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

Adding new repository for indexing: acado #27907

Closed
wants to merge 1 commit into from
Closed

Conversation

Andor233
Copy link

@Andor233 Andor233 commented Jan 6, 2021

No description provided.

@Andor233 Andor233 requested a review from sloretz as a code owner January 6, 2021 09:39
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

In all previous ROS distributions, this package was released from the upstream repository, namely https://github.com/tud-cor/acado.git . Can you please work with @RonaldEnsing to get this released from that repository?

@Andor233
Copy link
Author

I will create a pull request

@Andor233
Copy link
Author

@clalancette what's your opinion
tud-cor/acado#1

@gavanderhoorn
Copy link
Contributor

gavanderhoorn commented Jan 18, 2021

So we're not actually using Acado any more at CoR (or at least not with ROS), so we don't have an interest in maintaining the ROS release.

The easy way out would be to just accept the PR here, update the release and source repository.

And then wait for the next iteration of this process, where someone else wants to get an updated release of Acado into ROS, but @Andor233 is not around any more for whatever reason.

To improve continuity and make maintenance of such third-party releases a shared effort, would it perhaps be an idea to create a ros-thirdparty-releases organisation where release repositories for these kinds of packages would be hosted? The owners of that organisation would then always be able to grant access to new maintainers.

I realise this adds to the ever increasing maintenance overhead of all these packages, but might actually be worth it.

(and in this case I believe a new source repository is not needed: the patch to the CMakeLists.txt and the addition of the manifest could be part of the release repository, with the release simply tracking upstream's master).

@RonaldEnsing
Copy link
Contributor

I agree with @gavanderhoorn that something like a ros-thirdparty-releases may be preferred over the current process.

@Andor233
Copy link
Author

I agree too.
Actually, there is many of this kind repo in ROS, if we can find some way to add a package.xml and modify cmakelists.txt which like vcpkg is doing, I think everything will be better. It can be published to every ROS version (because these repos are ROS independent) rather than I need to make a new repo and use bloom to do release manually. Everything will be more automatic.

vcpkg is a good example I think.

@gavanderhoorn
Copy link
Contributor

if we can find some way to add a package.xml and modify cmakelists.txt which like vcpkg is doing, I think everything will be better.

you should be able to add those as patches in your release repository.

It can be published to every ROS version (because these repos are ROS independent) rather than I need to make a new repo and use bloom to do release manually.

just a note: this is not what I'm suggesting, and would also not be a desirable situation.

Tbh the packaging situation on Windows is not something I believe we should try to duplicate on Linux.

@clalancette
Copy link
Contributor

(and in this case I believe a new source repository is not needed: the patch to the CMakeLists.txt and the addition of the manifest could be part of the release repository, with the release simply tracking upstream's master).

I think this is the easiest path forward. If we could simply track https://github.com/acado/acado and patch in the package.xml in the release repository, that would be a lot better than what we have now. I guess it will still need a new release repository (since it seems like CoR isn't willing to host anymore), but that is a lot easier.

@wjwwood
Copy link
Member

wjwwood commented Feb 3, 2021

@clalancette what should we do here? Wait for an update to the pull request or close it?

@Andor233
Copy link
Author

Andor233 commented Feb 5, 2021

if we can find some way to add a package.xml and modify cmakelists.txt which like vcpkg is doing, I think everything will be better.

you should be able to add those as patches in your release repository.

It can be published to every ROS version (because these repos are ROS independent) rather than I need to make a new repo and use bloom to do release manually.

just a note: this is not what I'm suggesting, and would also not be a desirable situation.

Tbh the packaging situation on Windows is not something I believe we should try to duplicate on Linux.

Why not? I mean all the ROS independent package, the only thing changed is the system version( like Glibc, GCC) It should be all the same for every ROS version.

@cottsay cottsay added the noetic Issue/PR is for the ROS 1 Noetic distribution label Feb 10, 2021
@sloretz
Copy link
Contributor

sloretz commented Mar 4, 2021

I'm not familiar with how vcpkg does things, but it sounds like there's consensus on needing to modify the CMakeLists.txt and add a package.xml. It looks like the preferred way to do this is by patching it in the release repo rather than forking. There's some documentation on how to do that here.

http://wiki.ros.org/bloom/Tutorials/ReleaseThirdParty#Modifying_the_Release_Repository

@Andor233 I think I can make a release repo for acado on the ros-gbp org if you'd be willing to make a release from there.

@gavanderhoorn
Copy link
Contributor

Some days ago #28441 was merged, which introduced an acado_vendor package.

Source repository: autowarefoundation/autoware.auto/acado_vendor.

It was only released for ROS 2 though.

@jacobperron
Copy link
Contributor

Funnily enough, acado_vendor just pulls in a fork that contains a package.xml and modifies the CMakeLists.txt

https://gitlab.com/autowarefoundation/autoware.auto/acado_vendor/-/blob/main/CMakeLists.txt#L19

I'm not sure how feasible it is, but I think it would be better if the vendor package could pull in the original repository directly and apply necessary patches there (or just use a patched release repository as suggested above).

@clalancette
Copy link
Contributor

There's been no motion on this for a while, so I'm going to close this out. I think the way forward here is a new release repository that patches in the package.xml and CMakeLists.txt as necessary. Please feel free to request a new release repository from us, and we're happy to create it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
noetic Issue/PR is for the ROS 1 Noetic distribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants