-
Notifications
You must be signed in to change notification settings - Fork 20
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
Reduce duplication between colcon-cargo and colcon-ros-cargo #21
Conversation
3cdfbf4
to
8f33ddb
Compare
I tested that the example from the README still works. |
@esteve There's also this PR. I'm not sure if it makes much sense for non-ROS packages to follow the https://www.ros.org/reps/rep-0122.html install space layout, but if it does, this would be a nice simplification. |
@nnmm I have to look more into this, but a priori |
This tries to make colcon-cargo a better base class for colcon-ros-cargo, so that both packages have fewer combined lines. As a concrete benefit, colcon-cargo now supports cleaning up the build directory.
@esteve I changed this PR to no longer use |
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.
@nnmm great work!
Thanks! Unfortunately, CI seems to have a hiccup. Do you know what's missing @esteve? |
@nnmm I've triggered CI again, but there seems to be a job stuck. I'll look into it later, but it doesn't seem to appear on the Github actions tab, though. |
@nnmm it's really weird, seems to be coming from this old CI configuration: but I don't understand why it's being triggered now |
@nnmm CI seems to be fixed now |
Cool! From renaming to |
This tries to make colcon-cargo a better base class for colcon-ros-cargo, so that both packages have fewer combined lines.
As a concrete benefit, colcon-cargo now supports cleaning up the build directory.
I changed the installation location for binaries to be
${prefix}/bin
, since I think that's a more standard location.