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.

Sign up for GitHub

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

Jump to bottom

Added Draco to base.yaml #38056

Merged
merged 10 commits into from
Jul 27, 2023
Merged

Added Draco to base.yaml #38056

merged 10 commits into from
Jul 27, 2023

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Jul 24, 2023

As part of the effort to port point_cloud_transport ros-perception/point_cloud_transport#1 from ROS to ROS 2, we want to include draco as a dependency. It has binaries for some platforms

@ahcorde
Added Draco to base.yaml
19cda03 
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
@ahcorde ahcorde self-assigned this Jul 24, 2023
@ahcorde ahcorde requested a review from a team as a code owner July 24, 2023 15:37
@github-actions github-actions bot added the rosdep Issue/PR is for a rosdep key label Jul 24, 2023
This was referenced Jul 24, 2023
Merged
Merged
@ahcorde
Fixed
0dc8ab8 
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
@peci1
Copy link
Contributor

peci1 commented Jul 24, 2023

Wouldn't this interfere with #37378 ?

@cottsay
Copy link
Member

cottsay commented Jul 24, 2023

I agree that it would as-is, but the key should probably be libdraco-dev instead of draco anyway, in which case there would be no collision.
The linters picked up a few things as well.

ahcorde added 2 commits July 24, 2023 17:59
@ahcorde
Fixed
40f1514 
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
@ahcorde
Fixed
b851f8c 
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
cottsay
cottsay reviewed Jul 24, 2023
View reviewed changes
rosdep/base.yaml Outdated Show resolved Hide resolved
ahcorde added 3 commits July 24, 2023 18:13
@ahcorde
Fixed
23b4b85 
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
@ahcorde
Fixed
d5c01e5 
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
@ahcorde
Fixed
be36b51 
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
cottsay
cottsay reviewed Jul 24, 2023
View reviewed changes
Copy link
Member

@cottsay cottsay left a comment

Choose a reason for hiding this comment

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

Rather than calling out the platforms which DO work, we should call out the platforms which DON'T work so that we don't need to update the rules with each new release.

rosdep/base.yaml Outdated
Comment on lines 3096 to 3097
'*': null
'9': [draco-devel]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'*': null
'9': [draco-devel]
'*': [draco-devel]
'8': null

Copy link
Member

Choose a reason for hiding this comment

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

This suggestion has not been addressed yet.

rosdep/base.yaml Outdated Show resolved Hide resolved
@ahcorde
Fixed
2818a16 
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
@ahcorde ahcorde requested a review from cottsay July 25, 2023 17:07
cottsay
cottsay requested changes Jul 25, 2023
View reviewed changes
rosdep/base.yaml Outdated
Comment on lines 3096 to 3097
'*': null
'9': [draco-devel]
Copy link
Member

Choose a reason for hiding this comment

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

This suggestion has not been addressed yet.

@ahcorde
Fixed
799cf38 
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
@ahcorde ahcorde requested a review from cottsay July 25, 2023 18:11
cottsay
cottsay approved these changes Jul 25, 2023
View reviewed changes
Copy link
Member

@cottsay cottsay left a comment

Choose a reason for hiding this comment

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

Thanks for iterating. I think I'd still like to see the key called libdraco-dev to match the Debian package name and also allow for splitting the dev/runtime dependencies down the road, but this is acceptable as-is.

@cottsay cottsay requested a review from nuclearsandwich July 25, 2023 18:50
@ahcorde
Fixed
9a7766e 
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
@ahcorde
Copy link
Contributor Author

ahcorde commented Jul 25, 2023

Thanks for iterating. I think I'd still like to see the key called libdraco-dev to match the Debian package name and also allow for splitting the dev/runtime dependencies down the road, but this is acceptable as-is.

Added libdraco-dev

@ahcorde ahcorde merged commit e92d197 into ros:master Jul 27, 2023
@ahcorde ahcorde deleted the ahcorde/base/draco branch July 27, 2023 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peci1 peci1 peci1 left review comments

@cottsay cottsay cottsay approved these changes

@nuclearsandwich nuclearsandwich Awaiting requested review from nuclearsandwich

Assignees

@ahcorde ahcorde

Labels
rosdep Issue/PR is for a rosdep key
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ahcorde @peci1 @cottsay