-
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
colcon and ROS2 support #361
colcon and ROS2 support #361
Conversation
a2e0541
to
6cf0e7d
Compare
(PR drafts do not work with Travis as expected: https://travis-ci.community/t/draft-pull-requests-not-being-built/2434) |
4e00253
to
78546a5
Compare
@130s, @miguelprada: do you have some time to review this? |
Addressed #360 |
64fd566
to
6bfcc4b
Compare
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.
Sorry for taking so long to respond. This is a very thorough work from your side and requires some effort to absorb. Even then, I haven't done an exhaustive review of all the changes to the script, but rather I've barely gone over the documentation and re-ran the same set of jobs I used to test your previous colcon
branch (all of which are ROS1 jobs, BTW, since I've yet to go past the ROS2 tutorials).
First of all, prerelease test is no longer failing on universal_robot
, nor am I getting the exec: eval: not found error
👍
Then, as expected, I've had to modify some the setups we use in our private repositories at work:
- Migrated from
CATKIN_CONFIG
toCMAKE_ARGS
, which was quite straightforward - Migrated from
BEFORE_SCRIPT
toAFTER_SETUP_UPSTREAM/TARGET_WORKSPACE
- I had to use both, since we also use
UPSTREAM_WORKSPACE=file
and need to process stuff in upstream ws as well - I failed to find a way to automatically determine which workspace my script should process, and ended up duplicating our custom CI setup script with the paths of upstream and target workspaces hardcoded. Is there currently any way to find the relevant workspace to act upon on each of these hooks? I'm not even sure relevant is well defined in this context; have you thought about this? Would it make sense to export the workspace currently being processed to the hooks' environments?
- I had to use both, since we also use
Both these cases are pretty well documented in the migration guide, though. Nice job.
There are still a couple of issues I haven't quite figured out yet:
- Clang format test errors due to the LLVM format test code have re-appeared (see this job). I've had a look to see whether I could figure out why, but I failed miserably...
- Default
ros:$ROS_DISTRO-ros-core
images do not contain an SSH client, causing cloning of private repositories in upstream workspace to fail. I've managed to work around this by runningapt-get install -y openssh-client
inBEFORE_SETUP_UPSTREAM_WORKSPACE
, but there must be a better way...
My last comment is with regard to eliminating the job control-related configuration that exists in the legacy version. I guess this may be related to introducing a new builder and the fact that variables existing in legacy may not directly map to it. However, we currently rely on these to avoid exhausting the limited resources the machine running our pipelines has at its disposal. I will need to think a bit more about this, and try to figure out whether we can achieve the same result by other means, but have you got any ideas yourself about how this feature could be introduced back?
This is all for now. Thanks one last time for all your work!
* refactored into separate functions * flexibile upstream and downstream workspaces * support for BUILDER
5f92742
to
797c8e0
Compare
+1 for merge (although I haven't reviewed after my previous approval). Sorry I've been aware of this but re-reviewing this would probably take good amount of time, and I doubt I can do that in next few weeks. The target branch is not the default so I think less impact to the users. And once merged in I hope I can contribute by using it as we're in need of some new features added. |
017b3b6
to
c7e2792
Compare
I just rebased the commits a little bit to reflect the latest changes. @130s: Thanks for your response! I will go ahead tomorrow :) |
c7e2792
to
84e65db
Compare
I will keep my |
target branch: colcon
This PR adresses #358 and #333
Most things should work as expected, but might require some config changes.
For the time being I had to remove the support for
CATKIN_CONFIG
because it would only apply for catkin-tools, but not colcon.Instead, I would like to implement more specific options (#360)
ToDos:
industrial_ci
a hybrid (catkin/ament) package