REP 149: conditional dependencies

[dirk-thomas]

The draft for REP 149 contains multiple different parts. In order to not mix the discussion on those topics in a single ticket this issue is meant to focus on discussing the introduced conditional dependencies represented by the condition attribute.

[NikolausDemmel]

Please excuse my ignorance. I have not followed the discussion, but only just now read the current draft. Some feedback:

1. Some bits that should maybe be red, but arent: a) the condition attribute in build_depend b) the section on Environment Variables.
2. The current description of the condition attribute does not make it clear what kind of variables we are talking about here? I.e. how do these variables get their value? Environment variables?
3. If now environment has been source some tools might require that the necessary information is being specified explicitly when being invoked. --> 2 typos now --> no, source --> sourced.
4. The section about environment variables mentions that something like ROS_VERSION should be exported by roslib / rcl. The sentence above from that section mentions that this would only work once an environment has already been sourced. This would mean that a from-source build in a fresh environment would only work if you manually set all variables used by all conditions in all package files. The description of the condition attribute should somehow mention or reference this fact. I.e. it should maybe be encouraged to be limited its use to common variables like ROS_VERSION or ROS_DISTRO.
5. I guess it would be possible to make the tools processing package.xml smarter. For example it is conceivable that catkin_tools parses the package.xml once in the fresh environment, extracts a dependency tree without the conditional dependencies, starts building packages and reevaluates the package.xml from some package in an environment which sources the env hooks of all its unconditional dependencies, thus updating its dependecy tree. But it sounds like this gets really messy, and also tools that previously didn't care about the environment hooks like rosdep would now get much more complicated.

[dirk-thomas]

1. Some bits that should maybe be red, but arent: a) the condition attribute in build_depend b) the section on Environment Variables.

Thanks for pointing it out. Fixed in 672acb2.

2. The current description of the condition attribute does not make it clear what kind of variables we are talking about here? I.e. how do these variables get their value? Environment variables?

It is up to the software evaluating the condition what to pass in. E.g. ament_tools passes all environment variables in.

3. If now environment has been source some tools might require that the necessary information is being specified explicitly when being invoked. --> 2 typos now --> no, source --> sourced.

Thanks for pointing it out. Fixed in 392285d.

4. The section about environment variables mentions that something like ROS_VERSION should be exported by roslib / rcl. The sentence above from that section mentions that this would only work once an environment has already been sourced. This would mean that a from-source build in a fresh environment would only work if you manually set all variables used by all conditions in all package files. The description of the condition attribute should somehow mention or reference this fact. I.e. it should maybe be encouraged to be limited its use to common variables like ROS_VERSION or ROS_DISTRO.

Restricting the use to common variable might indeed be a good idea. For a from-source build it doesn't necessarily mean that you have to define those manually. The tools building a workspace usually don't consider the dependencies from the manifests beyond deciding the topological order. If additional dependencies are declared (which should be ignored due to their condition) that would only affect the order in case those dependencies actually exist which is often not the case.

5. I guess it would be possible to make the tools processing package.xml smarter. For example it is conceivable that catkin_tools parses the package.xml once in the fresh environment, extracts a dependency tree without the conditional dependencies, starts building packages and reevaluates the package.xml from some package in an environment which sources the env hooks of all its unconditional dependencies, thus updating its dependecy tree. But it sounds like this gets really messy, and also tools that previously didn't care about the environment hooks like rosdep would now get much more complicated.

The extra logic necessary for the conditions is certainly making various tools more complicated. Sadly we couldn't come up with an easier design proposal to achieve the goals describe by the use cases.

[NikolausDemmel]

The current description of the condition attribute does not make it clear what kind of variables we are talking about here? I.e. how do these variables get their value? Environment variables?

It is up to the software evaluating the condition what to pass in. E.g. ament_tools passes all environment variables in.

I think the REP should mention this, at least as an example. Maybe something like "Tools may populate the values for these variables in different ways, but typically they are evaluated as environment variables."

The section about environment variables mentions that something like ROS_VERSION should be exported by roslib / rcl. The sentence above from that section mentions that this would only work once an environment has already been sourced. This would mean that a from-source build in a fresh environment would only work if you manually set all variables used by all conditions in all package files. The description of the condition attribute should somehow mention or reference this fact. I.e. it should maybe be encouraged to be limited its use to common variables like ROS_VERSION or ROS_DISTRO.

Restricting the use to common variable might indeed be a good idea. For a from-source build it doesn't necessarily mean that you have to define those manually. The tools building a workspace usually don't consider the dependencies from the manifests beyond deciding the topological order. If additional dependencies are declared (which should be ignored due to their condition) that would only affect the order in case those dependencies actually exist which is often not the case.

I might be wrong, but I think that for catkin_tools at least in the default setting for build-isolation, it builds every package in a separate devel space, and it sources only exactly those devel spaces of packages that the current package depends upon, so there determining the exact set of dependencies is critical for build success. If this breaks the from-source build of desktop_full with catkin_tools, it needs to be communicated, but of course it would be nice if the workflow at least from desktop_full could stay the same without defining additional environment variables / command line parameters a-priori.

Maybe the REP should not restrict the use of variables, but simply encourage self-restriction. It could be still worth to restrict it for packages in desktop-full, to make the from-source build workflow work as it does now.

Do we expect this to be used for system dependencies? We could also encourage to avoid using conditions for system dependencies, otherwise rosdep might not work for clean from-source builds, at least not with the current workflow.

I guess it would be possible to make the tools processing package.xml smarter. For example it is conceivable that catkin_tools parses the package.xml once in the fresh environment, extracts a dependency tree without the conditional dependencies, starts building packages and reevaluates the package.xml from some package in an environment which sources the env hooks of all its unconditional dependencies, thus updating its dependecy tree. But it sounds like this gets really messy, and also tools that previously didn't care about the environment hooks like rosdep would now get much more complicated.

The extra logic necessary for the conditions is certainly making various tools more complicated. Sadly we couldn't come up with an easier design proposal to achieve the goals describe by the use cases.

I don't really have a better suggestion, except for maybe limiting the usage to a fixed list of variables, such that the various tools have chance to use some other means to provide these values, e.g. command-line parameters, some other heuristics to determine what the user likely wants. If the tools are kept general enough, e.g. always falling back to environment variables, then people can still do whatever they want with this internally, even if it is prohibited for packages in desktop_full.

[dirk-thomas]

Maybe something like "Tools may populate the values for these variables in different ways, but typically they are evaluated as environment variables."

👍 I added this in bc54d4a.

I might be wrong, but I think that for catkin_tools at least in the default setting for build-isolation, it builds every package in a separate devel space, and it sources only exactly those devel spaces of packages that the current package depends upon, so there determining the exact set of dependencies is critical for build success. If this breaks the from-source build of desktop_full with catkin_tools, it needs to be communicated, but of course it would be nice if the workflow at least from desktop_full could stay the same without defining additional environment variables / command line parameters a-priori.

Might it fail? Yes, it could in very specific cases. But even if the build tool "ignores" the condition attribute it would "just work" in a lot of cases. Why? Because extra dependencies should not do any harm. And the build tool doesn't install them if they are missing but only considers what is already available in the workspace.

Do we expect this to be used for system dependencies? We could also encourage to avoid using conditions for system dependencies, otherwise rosdep might not work for clean from-source builds, at least not with the current workflow.

I can see use cases where that would be definitely be beneficial. E.g. neither of the following packages would be necessary which only exist to map dependencies differently for different ROS distros: gl_dependency, qwt_dependency, webkit_dependency.

rosdep should be able to work just fine once it has been updated to support the information provided in format 3. If you haven't sourced any environment you already need to pass the ROS distro name explicitly using --rosdistro=.... That will provide enough information to define the ROS_DISTRO variable as well as the ROS_VERSION variable which will be known for each ROS distro.

[NikolausDemmel]

Might it fail? Yes, it could in very specific cases. But even if the build tool "ignores" the condition attribute it would "just work" in a lot of cases. Why? Because extra dependencies should not do any harm. And the build tool doesn't install them if they are missing but only considers what is already available in the workspace.

Fair enough. Including additional dependencies when in doubt sounds reasonable for a build tool like catkin, I agree. For other tools I don't know, but you guys will probably figure out a sensible way to handle the default cases for bloom and friends.

I don't want to dwell on this too much and also not overcomplicate things. Maybe we could just add something like the following to the "condition" tag description:

Package maintainers are encouraged to keep the range of variables used in conditions minimal. For most use cases ROS_VERSION and ROS_DISTRO should suffice. See "Environment Variables" below.

[dirk-thomas]

you guys will probably figure out a sensible way to handle the default cases for bloom and friends.

bloom will know which ROS distro to release in and based on that also the ROS version. Assuming the conditions only use that it can decide on which dependencies to use and which ones to skip.

Maybe we could just add something like the following to the "condition" tag description:

Package maintainers are encouraged to keep the range of variables used in conditions minimal. For most use cases ROS_VERSION and ROS_DISTRO should suffice. See "Environment Variables" below.

I think this is a good idea. I will bring it up in the next meeting and see if we can add that. Thank you for all the feedback.

[NikolausDemmel]

Thanks Dirk!

[dirk-thomas]

We talked about the possible environment variables and lean toward only to recommend using a specific list of "known" environment variables. The reasoning is that tools need to support setting them / passing them potentially as command line arguments. Beside ROS_VERSION and ROS_DISTRO we also consider having a variable ROS_PYTHON_VERSION but since the scope and exact usage of that isn't decided yet we will probably not add it to the current draft of the REP. I will update the REP with this soonish.

[JWhitleyWork]

@dirk-thomas - I figured this would be a good place to discuss the following required changes to the XSD regarding this REP:

Currently, package_common.xsd defines the type "DependencyType" which covers all depend_ tags. Because package_common.xsd is included in all 3 package_formatN.xsd files, modifying the definition in package_common.xsd would apply to all 3. I'm sure this was done for convenience and ease of maintenance. Should I move separate definitions of "DependencyType" into all 3 package_formatN.xsd files or just apply these changes across all 3 from package_common.xsd? Technically, the tools which allow each of the versions of the XML files should prohibit the use of this attribute on earlier versions.

[dirk-thomas]

Should I move separate definitions of "DependencyType" into all 3 package_formatN.xsd files or just apply these changes across all 3 from package_common.xsd?

Adding the attribute to older formats is not desired. Please refactor the definition from he common file into each version specific file and then change the format 3 case.

[dirk-thomas]

Beside ROS_VERSION and ROS_DISTRO we also consider having a variable ROS_PYTHON_VERSION but since the scope and exact usage of that isn't decided yet we will probably not add it to the current draft of the REP. I will update the REP with this soonish.

The environment variable ROS_PYTHON_VERSION has been added to the REP a while ago in #172. Therefore I am going ahead and close this ticket.