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

Add logic for excluding group workaround dependencies #155

Merged
merged 1 commit into from
Jun 5, 2024

Conversation

cottsay
Copy link
Member

@cottsay cottsay commented Apr 24, 2024

See ros-infrastructure/catkin_pkg#369

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@cottsay cottsay added the enhancement New feature or request label Apr 24, 2024
@cottsay cottsay added this to the 0.4.2 milestone Apr 24, 2024
@cottsay cottsay self-assigned this Apr 24, 2024
Copy link
Contributor

@nuclearsandwich nuclearsandwich left a comment

Choose a reason for hiding this comment

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

As I understand it, this isn't conditional is that colcon-ros already supports group dependencies (within the workspace, which is the only context in which it supports any dependencies) and so it should already not need this workaround.

If that's correct then we're all good. If I'm mistaken and there's a different reason it's not conditional I'd prefer another crack at reviewing after being corrected on the rationale.

@cottsay
Copy link
Member Author

cottsay commented Apr 24, 2024

colcon-ros already supports group dependencies (within the workspace, which is the only context in which it supports any dependencies) and so it should already not need this workaround

Correct.

@cottsay
Copy link
Member Author

cottsay commented Apr 25, 2024

We're going to need to block this change for now. Turns out that we actually have some messed up group unrolling in ROS 2 and this change exposes those bugs.

Who knew that manually curating a list of group members in downstream packages would be error prone?

I'll probably end up enabling this eventually as a feature flag once that change lands.

@cottsay
Copy link
Member Author

cottsay commented Apr 25, 2024

We're going to need to block this change for now.

After thinking about it, we actually don't. This doesn't change behavior in any manifests which aren't annotated with the workaround skipping logic, so a long as we fix the group unrolling at the same time, we'll never regress anything.

@cottsay cottsay merged commit 6575f2a into master Jun 5, 2024
17 of 20 checks passed
@delete-merged-branch delete-merged-branch bot deleted the cottsay/groups-workaround branch June 5, 2024 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

2 participants