Skip to content
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

Backport some colcon features #369

Merged
merged 10 commits into from
May 22, 2019

Conversation

mathias-luedtke
Copy link
Member

@mathias-luedtke mathias-luedtke commented May 14, 2019

Partial backport of #361

In addition it should simplify the review.

CoS (Condition of Satisfaction, suggested by @130s)

@mathias-luedtke
Copy link
Member Author

Due to the many renames it is best to review each commit individually.

Copy link
Member

@miguelprada miguelprada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've gone over the proposed changes and, even if I don't fully grasp every little change, nothing stands out for me that could potentially break anything.

Tested in universal_robot and ur_modern_driver, and also in some of our most convoluted internal pipelines (both before and after switching from CATKIN_CONFIG to CMAKE_ARGS), and every job passed.

For my part, it's ok to merge this.

@mathias-luedtke
Copy link
Member Author

@130s: Can you test /review it as well?
These fixes will end up in master/legacy.

@130s
Copy link
Member

130s commented May 19, 2019

(CoS suggested is moved to the OP)

@mathias-luedtke
Copy link
Member Author

Indigo is now EOL.

@mathias-luedtke
Copy link
Member Author

mathias-luedtke commented May 21, 2019

@130s: Can we merge this? Indigo jobs started to fail as a result of ros/rosdistro#21284.

@130s
Copy link
Member

130s commented May 22, 2019

I confirmed ABI feature issue is fixed with this PR 130s/openni2_camera#2 (comment)

Thanks for the great changes and backporting them!

@130s 130s merged commit 31d8443 into ros-industrial:master May 22, 2019
@130s
Copy link
Member

130s commented May 22, 2019

Tagged 0.8.0 mainly in a desire to clearly label a change where a ROS distro is now EOLed but still supported by ici.

@miguelprada
Copy link
Member

FYI, I just found a small regression introduced in this PR related to an arguably non-standard use-case (i.e. see #232).

Previously the export command here was masking the (error) return value of catkin_topological_order, and the execution continued even with an empty workspace. After bdd783f, this error is caught and the job fails.

I don't think this should be acted upon, since that use case is not very common (and I'm no longer doing that myself). Furthermore, I'm not sure that trick would make sense anymore when #361 is merged. I wanted to leave this comment here just for future reference, in case I stumble into this ever again.

@mathias-luedtke
Copy link
Member Author

Yes, this slipped through.
We could fix this on demand.
With #361 this problem does not exist anymore, you can just specify TARGET_WORKSPACE.
In addition colcon works with empty source spaces as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants