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

feat: add open3d vendor package #23

Closed
wants to merge 1 commit into from

Conversation

wep21
Copy link

@wep21 wep21 commented May 22, 2022

Currently, open3d apt package is not available. I added a vendor package to build open3d from src.

Signed-off-by: Daisuke Nishimatsu <[email protected]>
@wep21
Copy link
Author

wep21 commented May 22, 2022

@Matnay @chadrockey @SteveMacenski Could someone review this PR?

@SteveMacenski
Copy link
Member

We get notifications @wep21, you do not need to tag the maintainers

@SteveMacenski
Copy link
Member

SteveMacenski commented May 23, 2022

This looks reasonable, but I'm not sure that it should be added here. Someone can easily build / install open3D on their system, I'm not sure why we need a vendorization package; it still doesn't help us release this work to apt to be installable by users. What problem is this solving?

It also adds an additional burden on us here to dictate which specific version of open3D should be used (which can/will change when open3D is eventually released). That could cause some real headaches later if we're not tightly synced with Open3D's roadmap

@wep21
Copy link
Author

wep21 commented May 24, 2022

What problem is this solving?

To release open3d_conversions (or any pakcages depending on open3D), we need to resolve these dependencies
via rosdep(apt). However, open3D is not available via apt, so I added a vendor package as a workaround until it is available via apt.

@SteveMacenski
Copy link
Member

That's not a workable solution if we don't know what versions are planned for release for Open3D in the future to make sure its the same or at minimum API/ABI compatible. If we release this and then open3D releases something different, we're going to be in for a world of hurt of breakages for users. Do you have insight in the open3D project of the roadmap / planned versions for release?

@SteveMacenski
Copy link
Member

But @nkhedekar what do you think?

@wep21
Copy link
Author

wep21 commented May 24, 2022

If we use the vendor package, we can control the version of Open3D here. If there is a new release in Open3D, we can update the version of Open3D in the vendor package and the API/ABI in the packages which depends on the vendor package. (But the users need to recognize which version of open3d_vendor is compatible with the version of Open3D.)

@SteveMacenski
Copy link
Member

I don't think its the role of a conversion package to dictate what version of open3D ROS users should be having available to them. I think that's more up to the open3D maintainers to decide for a given OS version what they want to support and we fall in line.

If there's a new open3D release and our released package is not ABI/API compatible with it, we're in trouble because we cannot re-release for a given distribution a new version that would break existing users.

@SteveMacenski
Copy link
Member

I don't think this is a bad idea to get open3D out sooner than later, but it needs to be made in conjunction with open3D maintainers. We need to know their plans so that we can use the same version as they plan to release or at least make sure its ABI/API compatible.

@wep21
Copy link
Author

wep21 commented May 25, 2022

Okay. I will ask open3D maintainers first.

@SteveMacenski
Copy link
Member

#25 (comment) supersedes - I think Foxy just won't have a binary release but OK for users to clone and build

@wep21 wep21 deleted the open3d-vendor branch June 2, 2022 00:01
@wep21 wep21 restored the open3d-vendor branch June 4, 2022 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants