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

Support for version metadata of dependencies in the ros package descriptor #39

Closed
murphm8 opened this issue Aug 30, 2018 · 7 comments
Closed
Labels
enhancement New feature or request

Comments

@murphm8
Copy link
Contributor

murphm8 commented Aug 30, 2018

Hi @dirk-thomas,

I'm interested in adding support for reading the version requirement metadata from package.xml and adding it to the descriptor. The package manifest format supports equality operators rep-0127.

Currently, descriptor.dependencies[<stage name>] returns a list of strings which maps to package names Code Link. My first thought would be to update dependencies[<stage name>] to be a list of Dependency objects that contain name and version attributes.

This seems like a possibly breaking change to the existing build sequencing code.

  • What is your policy on breaking changes like this? Could I update the relevant packages along with this one and bump their version requirements in setup.cfg?
  • Do you think there is a better way include this information in the package descriptor?

Thanks for your input!
Matt

@murphm8 murphm8 changed the title Support for versions of dependencies in the ros package descriptor Support for version metadata of dependencies in the ros package descriptor Aug 30, 2018
@dirk-thomas dirk-thomas added the enhancement New feature or request label Aug 30, 2018
@dirk-thomas
Copy link
Member

dirk-thomas commented Aug 30, 2018

I'm interested in adding support for reading the version requirement metadata from package.xml and adding it to the descriptor.

That sounds like a good idea.

My first thought would be to update dependencies[] to be a list of Dependency objects that contain name and version attributes.
This seems like a possibly breaking change to the existing build sequencing code.

Storing a dependency object (here) sounds like the right away. I am not sure how the data is currently being accessed in the various locations. The less "breaking" a patch is going to be the easier it would be to apply it across multiple packages.

What is your policy on breaking changes like this?

If it is necessary to introduce this feature I am fine with it. We can update version numbers, minimum required versions, and extension point versions where necessary.

Could I update the relevant packages along with this one and bump their version requirements in setup.cfg?

Generally we should first get the patches ready and then determine how to address the versioning (when it is clear what actually is a breaking change).

One question will be what the consequence should be when a version requirement is not satisfied. In general the user should have the ability to continue if desired. So is the goal to print a warning message if the version range is not matching?

That being said maybe there is a way to "inject" this through a separate package so that colcon-core doesn't need to know about dependency version conditions... 🤔

@murphm8
Copy link
Contributor Author

murphm8 commented Aug 31, 2018

I can't think of a way of injecting this functionality, but I think it will work without colcon-core knowing about versions. If colcon-core defines a base DependencyDescriptor, another package could define a subclass that includes version information, only packages that care about the version information would create/use that class.

For ROS packages specifically I think colcon-ros could define a ROSDependencyDescriptor that would include metadata from the ROS ecosystem. I could imagine another package augmentation which updates the ROSDependencyDescriptor with metadata from rosdep about how to fetch the dependency, or the ROSDependencyDescriptor creating that metadata itself based on the name.

I do wonder if versioning is a low level enough thing that it should be included in core? I'm not sure why core would care about it.

@dirk-thomas
Copy link
Member

If colcon-core defines a base DependencyDescriptor ...

That sounds like the right direction. I don't think extensions need to use subclasses though. As the PackageDescriptor the DependencyDescriptor could provide a generic metadata property to store arbitrary other information.

how to fetch the dependency

This specific feature is explicitly out of scope of what colcon(-core) aims to provide / support (see https://colcon.readthedocs.io/en/latest/developer/design.html#explicitly-out-of-scope).

@murphm8 How about my other question:

what the consequence should be when a version requirement is not satisfied.

@murphm8
Copy link
Contributor Author

murphm8 commented Aug 31, 2018

what the consequence should be when a version requirement is not satisfied.

I'm not sure on this one. How does colcon know what version of a particular dependency is installed? Should colcon-ros include the package version from the package.xml in the metadata too?

It could then give a warning if there is a mismatch between the version in the workspace and a declared dependency. For the build though, what actually has vision of all the packages in the workspace? I am grappling with this on my extensions also. Right now, my verb passes in the names of all the packages in the workspace to the task extensions, but that doesn't feel right.

EDIT: Whoops accidentally clicked comment and close.

@murphm8 murphm8 closed this as completed Aug 31, 2018
@murphm8 murphm8 reopened this Aug 31, 2018
@dirk-thomas
Copy link
Member

How does colcon know what version of a particular dependency is installed?

colcon has no awareness about any software / packages outside the workspace. I would also consider that out scope for the tool. Even the ROS package manifest doesn't contain the necessary information to know what kind of package a specific rosdep dependency key represents.

For the build though, what actually has vision of all the packages in the workspace?

After all packages have been discovered it should be possible to check if the version dependencies between them are satisfied. A verb has knowledge about all packages it is supposed to operate on. A task on the other hand only operates on a single package and gets the names and paths of its dependencies in order to use them (e.g. to setup the environment accordingly).

Maybe a package selection extension could perform the version checking. It already has access to all packages and their descriptors / decorators?

@dirk-thomas
Copy link
Member

The version information of dependencies has been added to the descriptor in #40. Is there anything else in this ticket which should be done or is it "addressed"?

@murphm8
Copy link
Contributor Author

murphm8 commented Oct 24, 2018

This ticket is complete, all the requirements have been addressed. Thanks!

@dirk-thomas dirk-thomas removed the in progress Actively being worked on (Kanban column) label Oct 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

No branches or pull requests

2 participants