Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add design docs for versioning export #511

Draft
wants to merge 1 commit into
base: rolling
Choose a base branch
from

Conversation

Ryanf55
Copy link
Contributor

@Ryanf55 Ryanf55 commented Feb 23, 2024

This adds design docs for how ament packages can express their versioning, and versions of the dependents. It should give nice behavior when making ABI breaking changes if you use Semver.

Please provide feedback. The design docs here explain how it could work. This may require some prototyping to prove it can work.

Copied sections from #506. Read that ticket to get context around the use case and why this is desirable.

Signed-off-by: Ryan Friedman <ryanfriedman5410+github@gmail.com>
@cottsay
Copy link
Contributor

cottsay commented Feb 26, 2024

Hi, thanks for bringing this up. I have a few thoughts.

Regarding declaration of version compatibility of an installed package: This seems like a great feature. While the implementation in ament_cmake_core would be pretty straightforward, I would love to see this implemented in such a way that the source of the information was in the package.xml and wasn't CMake-specific. Two relevant points:

  1. The package version number embedded in the package version file is already sourced from the package.xml
  2. The v3 package.xml specification has an optional attribute for version (ABI) compatibility, so there is precedent for this type of information

While I don't believe we can use the existing package.xml compatibility attribute in this context (where I believe you essentially want to declare API compatibility), there may be grounds to add such information. This would also allow tools like colcon to identify version incompatibilities before the build even occurs, including problems among packages which don't involve CMake at all.

All this said, it is very common for a package to be compatible with more than one major version of a dependency, even under proper use of semantic versioning. This leads to an important question: how do we handle a conflict between what version compatibility is explicitly specified in a downstream package and what downstream compatibility a package expresses in an upstream package?


Regarding specification of version requirement of a dependency: This one makes a lot of sense to me, but is largely not a problem in the ROS package ecosystem due to the inherent assumption of ABI incompatibility. At least in the deb and RPM system packages, a package simply isn't available if it didn't successfully build, as would be the case if a package declared a build-time version requirement that wasn't satisfied.

That said, there's certainly an argument here to "do the correct thing" especially in the context of other build systems and deployments of ROS 2.

I'll also say that implementing this behavior doesn't look trivial at first glance.

@Ryanf55
Copy link
Contributor Author

Ryanf55 commented Feb 29, 2024

Thanks for the ideas. Here's some follow up questions:

  1. The package version number embedded in the package version file is already sourced from the package.xml
  2. The v3 package.xml specification has an optional attribute for version (ABI) compatibility, so there is precedent for this type of information

While I don't believe we can use the existing package.xml compatibility attribute in this context (where I believe you essentially want to declare API compatibility), there may be grounds to add such information. This would also allow tools like colcon to identify version incompatibilities before the build even occurs, including problems among packages which don't involve CMake at all.

  1. How do you envision dealing with the case that the CMakeLists.txt and package.xml have different versions? I was thinking of using the version in the package.xml, and then issuing a developer warning at configure time.
  2. How do you think we should deal with package.xml only supports MAJOR.MINOR.PATCH but CMake project supports <major>[.<minor>[.<patch>[.<tweak>]]].
  3. Is there an existing CMake function get the version from the package.xml?

All this said, it is very common for a package to be compatible with more than one major version of a dependency, even under proper use of semantic versioning. This leads to an important question: how do we handle a conflict between what version compatibility is explicitly specified in a downstream package and what downstream compatibility a package expresses in an upstream package?

That's a hard question; yea - ABI only breaks if you call the functions that changed ABI. That's why I suggested defaulting to AnyNewerVersion for now to have the smallest impact.


Regarding specification of version requirement of a dependency: This one makes a lot of sense to me, but is largely not a problem in the ROS package ecosystem due to the inherent assumption of ABI incompatibility. At least in the deb and RPM system packages, a package simply isn't available if it didn't successfully build, as would be the case if a package declared a build-time version requirement that wasn't satisfied.

That said, there's certainly an argument here to "do the correct thing" especially in the context of other build systems and deployments of ROS 2.

I'll also say that implementing this behavior doesn't look trivial at first glance.

Sounds like it might be useful to just try a simple prototype of "doing the right thing" and see if blows up. That's generally how I approach these sort of things, but that's more on my lack of experience. If you suspect it's going to be difficult and complicated, I'm happy to split the scope of this effort into two, and focus on the first part which seems more achievable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants