-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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 pip specific content contribution rosdep rules for Gentoo #29909
Conversation
lgtm |
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.
A few minor rewording changes.
Co-authored-by: Chris Lalancette <[email protected]>
Co-authored-by: Chris Lalancette <[email protected]>
Did this PR also mean to add a note about pip in the REVIEW_GUIDELINES (line 63)? Maybe something like, for Gentoo entries, check that it does not use pip, and if it does, refer to CONTRIBUTING. Related, but not to this PR, I did a search in rosdep/python.yaml and found that we do have like a dozen gentoo+pip entries, like https://github.com/ros/rosdistro/pull/29426/files . I don't know if we can/want to do anything about them. |
I initially thought to do that. But then I realized that all the distro related rules are here in the CONTRIBUTING document and that these rules are directly linked in the REVIEWING section. There's several other rules per distro that are at the same level. This one is new but doesn't seem to have any distinction compared to all the others. And duplicating the requirements in a second location I think will lead to more maintenance or inconsistency in statements.
A PR to clean them up/remove them would be great, it's mostly a matter of having the time to clean it up. We can definitely remove them as they're not expected to work and as such won't break people to remove them and we don't need a sunsetting process. |
I could do that PR. But does that mean we also want to create ros/ros-overlay tickets for all of them? |
Great. Ticketing it over there too would be great. To be quicker maybe just a single ticket with each of the dependencies you cleared as a checkbox instead of opening a bunch of tickets. |
Fixes: #29908
@allenh1 From your suggestion.