-
Notifications
You must be signed in to change notification settings - Fork 129
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 ROS-independent CI Builds #818
base: master
Are you sure you want to change the base?
Conversation
95af2d7
to
56dca0f
Compare
56dca0f
to
2f4c8ab
Compare
IMHO this is out of scope of industrial_ci. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to sacrifice the sanity checks for an edge case, which is not really supported..
@@ -172,7 +172,7 @@ function run_source_tests { | |||
|
|||
ici_step "setup_rosdep" ici_setup_rosdep | |||
|
|||
extend=${UNDERLAY:?} | |||
extend=${UNDERLAY:-} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This knocks out one of the sanity tests.
@@ -406,7 +406,7 @@ function ici_source_setup { | |||
local extend=$1; shift | |||
if [ ! -f "$extend/setup.bash" ]; then | |||
if [ "$extend" != "/opt/ros/$ROS_DISTRO" ]; then | |||
ici_error "'$extend' is not a devel/install space" | |||
ici_log "'$extend' is not a devel/install space; skipping source" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, this is an important sanity check to test against misspelled parameters.
export BUILDER=${BUILDER:-colcon} | ||
export ROS_VERSION=0 | ||
export ROS_VERSION_EOL=false | ||
export ROS_PYTHON_VERSION=${ROS_PYTHON_VERSION:-3} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are questionable defaults.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are your suggested alternatives? Python2 is EOL, so Python3 seems reasonable. colcon
is the only builder I am familar with that can build code such that a downstream ROS1 or ROS2 workspace can use it. The ROS version cannot be EOL since this is for a ROS-independent build. ROS_VERSION
is required by other parts of ICI, so anything < 1 seems okay
Maybe at the moment, but I'm proposing this PR to support these types of builds as a feature of At least for ROS-I Americas, I think ROS-independent builds are going to become more common as we separate the core functionality of our tools from the ROS interfaces that make them convenient to use. As stated in #817, I personally already have several ROS-I related projects that could leverage this type of build today, with more planned in the near future. We could certainly gather feedback from others if there is a question about general interest in this capability as a feature. In my opinion, there is a ton of value in the ICI pipeline: nested builds, docker image creation, testing, dependency installation, etc. I would rather not re-implement this whole pipeline elsewhere when it only requires very minor modifications to support the builds I am interested in. Regarding the sanity checks, I understand the concern. What would be an appropriate way to keep them while being able to keep these added features (i.e., no |
What is so bad about sourcing /opt/ros? |
#834 should be sufficient, it breaks ros_prerelease tests, but I will have to come up with a better implementation for those anyway |
Mostly because I want to do builds where ROS isn't actually installed, so there is no |
The whole point of colcon, ament, and vcs is they are separate from ROS. Perhaps an alternative method is to contribute the actions directly to those repositories?IE - VCS has a clone action, and colcon has a build action and a test action? |
Addresses #817 to allow CI builds that do not source
/opt/ros/<distro>
during the build.Here is an example CI run where this capability was used