First, sorry if I posted this to the incorrect category
I wondered why (simple) ROS packages wouldn’t be available as a single git branch for many different ROS distributions, even ROS1 and ROS2 ones.
Recently, I managed to get a simple package building locally without issues in both ROS1 and ROS2 environments which, I think, it’s great regarding maintainability in the long term.
One key requisite for this is having a package.xml that would be parsed differently for ROS 1 & 2, and package version=3 provides such a great feature:
<export>
<!-- Other tools can request additional information be placed here -->
<build_type condition="$ROS_VERSION == 2">ament_cmake</build_type>
<build_type condition="$ROS_VERSION == 1">catkin</build_type>
</export>
However, in the ROS1 & ROS2 build farms, these are the only environment variables defined:
ROS_PYTHON_VERSION=3
ROS_DISTRO=rolling
I could “hack” my code to check for every possible combination of ROS_DISTRO for ROS1/2, but I would propose, instead, to make the ROS_VERSION variable visible to the build farm instances, if possible. I was really surprised not to see it already defined… what’s the rationale of not providing it?
I feel like this walks a line between a good question on ROS Answers and a topic for the Buildfarm category here on Discourse. I’m not a moderator here but perhaps one will consider moving this to that category.
When posting about build farm builds, it helps to link to examples of which builds you’re asking about.
The build farm runs builds from source: configured by “source build” files which generate the “*dev” and “*pr” jobs, release builds: configured by “release build” files which generate the “*src” (for source packages) and “*bin” binary jobs, and documentation builds: configured by “doc builds” and generating “*doc” jobs.
For release builds, the package dependencies are handled when bloom is run and the job scripts use the generated metadata only. Data from the package.xml is, at that point, only used to generate the relationships between packaging jobs. This function is responsible for generating the context that package conditions are evaluated in for release and it defines ROS_VERSION, ROS_DISTRO, and ROS_PYTHON_VERSION.
For source builds, rodsep resolution happens ahead of time as part of the task generation step.
Those builds use this context for evaluating package conditions which automatically defines ROS_VERSION and ROS_DISTRO but only inherits ROS_PYTHON_VERSION from the environment if it is defined (as an aside, I think that ROS_PYTHON_VERSION ought to be defined regardless of environment as well and will open a PR suggesting as much).
So if there’s a place where package conditions aren’t working properly due to ROS_VERSION missing, there is possibly a situation where conditions are either not being properly resolved or where the conditional context is not being configured as expected.
If your package further requires the environment and/or CMake build variables to be available in the actual build or runtime context, it is recommended that your package depend on the ros_environment package which makes those variables available to downstream packages via environment hook scripts.
A link to a specific build which demonstrates the difference between expected and actual behavior would be welcome. And would help to determine whether this should move to an issue on the ros_buildfarm scripts or elsewhere.
(Sorry for the formatting, writing on move on the phone).
After your reply, I now understand that the missing ros_version variable happens at an earlier stage, before invoking cmake, since as you can see, neither catkin nor colcon can interpret the conditional attributes of the package.XML, while they do when invoked locally.
Independent of this first issue, the same env variable would be really useful too inside the cmake script to know if catkin or ament should be used, but the error seen in the linked logs above come from an earlier stage…
Perhaps it’s an issue with the build farm, since I can see :
Where setup.sh is invoked, but still ros_version is not defined afterwards?? Perhaps it’s only defined in setup.bash? You probably understand much easily what’s wrong or what I am misunderstanding. Thanks!
I’ll want to confer with some of my colleagues but I think that either
build farm jobs should be equipped to handle ROS 1 / ROS 2 conditional packages up to the configure stage (i.e. cmake configuration should be able to run even if it fails) and thus we need to make some changes, either in the buildfarm scripts or the build tools (where will depend on further review).
In documentation discussing same-branch ROS 1 and ROS 2 packages it should point out that all such packages must have a build dependency on ros_environment.
I’m not sure which answer is “correct”. My Monday morning attitude says that requiring ros_environment is going to be better overall since in my opinion build scripts and tools should only special-case up to the actual invocation of cmake configure if they’re going to special case at all (I think it’s possible that the current behavior is just the result of conditions not being applied or respected uniformly) and should be totally hands off after that. I think it is possible to write a CMakeLists.txt (example below) that doesn’t rely on explicit ROS_VERSION conditionals but I’m not sure whether that’s a recommended workflow either.
find_package(ament_cmake)
if (ament_cmake_FOUND)
message(STATUS "Configuring as ROS 2 package")
else()
find_package(catkin REQUIRED)
message(STATUS "Configuring as ROS (1) package")
endif()
Baking these sorts of variables into the buildfarm configuration/deployment/bootstrap is definitely doable. But in general we have worked hard to avoid embedding this sort of special logic into the buildfarm and instead focus on making sure that it’s captured in the packages themselves. The main reason for this is that the buildfarm is only one of the ways that people build ROS packages. Developers actively build it from source. In addition we have people building for many other platforms and binary packages on using other infrastructure such as OpenEmbedded, Gentoo, Debian/Ubuntu upstream and Conda. As well as creating specific systems for building tarballs and deploying within companies or setting up Dockerfiles.
The more content that we require on the system to be in the bootstrap setup the harder it is for things to build consistently across all these different platforms. If instead we package the environment settings as a platform independent dependent package that all developers can include in their standard pipeline then it can be consistent for everyone. The easiest example would be that if we add this extra variable on the buildfarm and all the CMake logic depends on it, then if someone checks it out from source and doesn’t set that variable appropriately then their source build will not work, and they won’t have a clue why. Whereas if you declare a dependency on ros_environment and it’s not present, the build process should give you warnings and or errors of missing dependencies. And if we make it part of the boostrap and people build their packages to rely on it, we will have to keep using it in the future for backwards compatibility potentially indefinitely as it would be very hard to know when all CMake code is no longer reliant on this specific boostrapping variable.
We should definitely do this anywhere ROS_VERSION is documented to say that to use this parameter you have to use ros_environment as a build dependency. Otherwise it will lead to lots of bugs of undeclared dependencies that work most of the time due to implicit installation from other dependency trees.
This is something that can be valuable in certain situations where a package might be used outside of a ROS distribution. And cares about the availabilty of certain build tools but not necessarily the other characteristics potentially implied by the ROS_VERSION. However, this is probably something that we want to leave to the more advanced user and not be high in the recommendations in the documentation.
I just wanted to share my findings on @nuclearsandwich 's idea of a “pure cmake” detection of ros1/2: unfortunately, find_package(ament_cmake) cannot be used since that .cmake requires cmake_minimum_required(VERSION 3.8) (due to usage of IN_LIST), whereas ros1 repos normally require cmake 3.1, and adding 3.8 to all packages shouldn’t be an option (would make builds in the farm for older distros to fail, etc.).
However, that idea is still probably the less intrusive since it doesn’t require introducing new environment variables in the farm builds, which doesn’t seem desirable per @tfoote arguments.
Any ideas on any other, simple cmake package that should be always present in ros2 only so could be used as a “marker” for ROS2 identification?
Can you clarify? If it’s not found then it won’t be required. If it’s installed you should have a high enough version of CMake.
This might make sense as a feature request for ros_environment to set a CMAKE variable that you can leverage after declaring a dependency on it in addition to the shell environment variables.
Sorry, I just realized that even u18.04 bionic provides cmake 3.10 (>3.8), so it wouldn’t be a problem to declare cmake_minimum_required(VERSION 3.8).
Anyway, I meant that find_package(ament_cmake QUIET) would fail with cmake_minimum_required(VERSION 3.1), if the package is found, due to too-old cmake policies enabled by default.
On the main topic of this post: the key was adding a package.xml-level dependency on ros_environment. With that, ROS_VERSION is defined and correctly handled by catkin, colcon, and of course, we can use it within cmake via $ENV{ROS_VERSION}, so my original question is solved. Probably mentioning ros_environment in the docs (if not done already and I missed it?) would be great.
Thanks!
