jspricke / ros-deb-builder-action

Github Action to convert ROS packages to Debian packages
BSD 3-Clause "New" or "Revised" License
65 stars 24 forks source link

Pass $ROS_DISTRO to sbuild #3

Closed esteve closed 1 year ago

esteve commented 1 year ago

This passes the $ROS_DISTRO environment variable to packages built with sbuild

jspricke commented 1 year ago

Thanks, I assume you want this for https://github.com/autowarefoundation/autoware_common/blob/main/autoware_cmake/cmake/autoware_package.cmake#L38

I think that is the wrong approach and you should rather deprecate those defines as I don't find any users of them (for example there is no semantic behind ROS_DISTRO_ROLLING). The better way is to check version numbers of the specific packages if you have version specific code.

esteve commented 1 year ago

It's unlikely we'll change that code in autoware_package.cmake, but moreover ROS_DISTRO has been part of all the ROS distributions for a long time, so in that respect I don't see why ros-deb-builder-action shouldn't pass that environment variable. How downstream packages use that variable (correctly or incorrectly) is not part of ros-deb-builder-action goal, in my opinion.

esteve commented 1 year ago

As for who uses $ENV{ROS_DISTRO} in CMake, here's a list:

https://github.com/search?q=%24ENV%7BROS_DISTRO%7D&type=code

Which I'm not arguing whether is correct or not, but if these projects were to use ros-deb-builder-action, they wouldn't be able to do so.

jspricke commented 1 year ago

Looking at it again, it is actually a missing dependency: https://github.com/autowarefoundation/autoware_common/pull/142

jspricke commented 1 year ago

To give some context: bloom adds loading the setup.sh to every debian/rules so the variable is defined if ros_environment is installed.

jspricke commented 1 year ago

@esteve autoware_cmake is fixed in main, can you retrigger your action?

esteve commented 1 year ago

@jspricke thanks for your PR, but autoware_cmake is not fixed, the PR doesn't address the issue. I've triggered a run anyway, but it still failed. In any case, if ros-deb-builder-action strives to mimic (or at least be be consistent) with the behavior of the ROS buildfarm, it should pass $ROS_DISTRO to downstream.

The buildfarm does not make any decision how packages should use $ROS_DISTRO environment variable, even if they use it wrong, it does not teach any downstream packages how they should be built, if they fail, they need to be fixed and submitted again, that's it. So when ROS_DISTRO=rolling may become outdated, package authors will fix them in their own packages, that's it.

esteve commented 1 year ago

Closing in favor of https://github.com/autowarefoundation/autoware_common/pull/144