rock-core / autoproj

Rock (Robot Construction Kit) package-oriented build system
http://rock-robotics.org/rock-and-syskit/workspace
23 stars 21 forks source link

Support conditional dependencies in manifests #381

Closed pierrewillenbrockdfki closed 1 year ago

pierrewillenbrockdfki commented 2 years ago

Conditional dependencies would help solve the problem where a package can be built in multiple ways but depending on different sets of packages for each of those ways.

The actual problem here are packages that can be configured to compile either with qt4 or with qt5. Currently, a manifest would have to include both with <depend package="qt4" optional="1" /> <depend package="qt5" optional="1" />, which makes autoproj try to install both qt4 and qt5, if available, and it will try to build the package even when the correct qt(as configured) could not be installed.

Having autoproj install both qt4 and qt5 for a workspace that only actually requires qt4 is not desirable, and attempting a build that is destined to fail due to missing dependency is not desirable either.

In https://github.com/rock-core/package_set/pull/210#issuecomment-1199870798, @doudou proposed new manifest syntax: <depend ... if="..." />

This would probably be a good solution, so lets try to flesh it out a bit more.

Compatibility

Is compatibility with rosdep needed, that will parse the manifest.xml if given the opportunity, or has that already been dropped?

Older autoproj would interpret the <depend ... if="..." /> syntax like a normal always-on dependency, so is there merit to add a variant that older autoproj would always ignore?: <conddepend ... if="..." />

The Condition

The condition would have to be something that can be set by package-sets. Maybe support expressions.

I would try to have a ruby expression here with terms that are predefined from a per-package hash. These hashes could then be manipulated by package-sets.

Before i start implement something based on eval here, is there a better option?

g-arjones commented 2 years ago

You can actually do that already if you use ROS manifests (package.xml) instead.

See the manifest syntax See the expression syntax

The implementation follows ROS' REP-149.

I think that's something reasonable to have in autoproj's vanilla manifest as well and if you decide to implement it, please try to reuse as much code as possible.

pierrewillenbrockdfki commented 2 years ago

You can actually do that already if you use ROS manifests (package.xml) instead.

See the manifest syntax See the expression syntax

The implementation follows ROS' REP-149.

I think that's something reasonable to have in autoproj's vanilla manifest as well and if you decide to implement it, please try to reuse as much code as possible.

Thank you, that makes it trivial to implement. I could pretty much copy/paste directly from ros_package_manifest.rb.

I think this is going to work for the qt4/qt5 situation.

pierrewillenbrockdfki commented 1 year ago

The syntax change for <depend condition="..."/> has been merged. I actually don't think compatibility with older autoproj is needed, since autoproj is generally the last component that stops to be updated, long after packages have been pinned.