search

borglab-launchpad / gtsam-packaging

repo for nightly gtsam builds
4 stars 1 forks source link

Push into Debian proper #2

Open dkogan opened 1 year ago

dkogan commented 1 year ago

Hi. Is there any reason you're building private packages instead of contributing to the main Debian (and by extension, Ubuntu) repo? That would make THIS work unnecessary. I'm about to file an ITP. Let me know if I'm missing anything. Thanks

dkogan commented 1 year ago

The ITP: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1031098

berndpfrommer commented 1 year ago

Sorry I didn't see this earlier. The reason we are building this in a private repo is because a) we have no idea what it entails to get the package pushed into Debian b) we are under the impression that the quality of our packaging is not up to the high standards required for getting GTSAM into a major distribution. I'd love to hear more about these two points. In particular related to b): we'd then have to strictly make sure we don't break the ABI on a new release, correct? How do we safeguard against that (and potentially other mistakes)?

berndpfrommer commented 1 year ago

@jlblancoc is actually more familiar with packaging than I.

dkogan commented 1 year ago

Hi. Thanks for replying. I'm now mostly done with the package:

https://salsa.debian.org/science-team/gtsam

There're a few loose ends to tie up, and it'll be ready to push into Debian. Currently the next Debian release is being prepared, and new packages aren't accepted. It'll open up in a few months, and I'll push it then.

You can break the ABI on a new release, if you really want to. You would then need to bump the ABI version in the SONAME of the library, and the package would be renamed accordingly. But if you can avoid breaking ABIs and APIs, that would be preferable, obviously.

There were a few patches needed to make the package work. These are here:

https://salsa.debian.org/science-team/gtsam/-/tree/master/debian/patches

Most of these tell the build to not use the bundled libraries, and to use the system instead. If you can add a config option to not touch any of the bundled stuff, that would be great.

Some other patches fix bugs: patch 0001. You want patch 0006 to use the new SPECTRA API. Patch 0007 removes the ceres autodiff. This doesn't exist in the new ceres; I think they removed it a long time ago.

I'll let you know when new packages can be pushed again. Let me know if you have any suggestions.

jlblancoc commented 1 year ago

@dkogan Thanks for the great work! We have been trying to push all the relevant changes upstream, but upstream's policy is not 100% compatible with Debian's, so you did very well in adding the patches, and problem solved.

I thought gtsam latest version didn't need boost at all, but I didn't check it for your packaged version 4.2a9, could you please check it?

As you said, Debian is in Freeze right now and will be for months, so we probably have time for a new gtsam release, 100% boost free. But anyway, having boost or not, it's better to have SOME gtsam version directly in Debian/Ubuntu.

FYI: I also pushed packaged versions of gtsam to ROS2 these past weeks, so they can reach users much faster (months) than thru the Debian -> Ubuntu pipeline (~ 1 year).

Do you need a sponsor for uploading to Debian, or are you a DD? :-)

dkogan commented 1 year ago

OK. Thanks for the notes. I'm making this package for a project, and will go back, and tie up all the loose ends (copyright, dependency tightening, etc) then. We're in a freeze, so there's no rush. If upstream makes a new release by then, I'll push that.

I'm a DD, and don't need a sponsor. One less thing to think about.

There's a cmake problem in the build that needs debugging. If you're adept in cmake, and can help debug, that would be really great. If you check out the sources from salsa, and try to build, cmake barfs:

-- Found pybind11: /usr/include (found version "2.10.0") CMake Warning (dev) at /usr/lib/cmake/pybind11/pybind11NewTools.cmake:220 (if): Policy CMP0057 is not set: Support new IN_LIST if() operator. Run "cmake --help-policy CMP0057" for policy details. Use the cmake_policy command to set the policy and suppress this warning.

IN_LIST will be interpreted as an operator when the policy is set to NEW.
Since the policy is not set the OLD behavior will be used.

Call Stack (most recent call first): wrap/cmake/PybindWrap.cmake:108 (pybind11_add_module) python/CMakeLists.txt:80 (pybind_wrap) This warning is for project developers. Use -Wno-dev to suppress it.

CMake Error at /usr/lib/cmake/pybind11/pybind11NewTools.cmake:220 (if): if given arguments:

  "NOT" "ARG_WITHOUT_SOABI" "AND" "NOT" "WITH_SOABI" "IN_LIST" "ARG_UNPARSED_ARGUMENTS"

Unknown arguments specified

Call Stack (most recent call first): wrap/cmake/PybindWrap.cmake:108 (pybind11_add_module) python/CMakeLists.txt:80 (pybind_wrap)

This has something to do with using the system pybind11:

https://salsa.debian.org/science-team/gtsam/-/blob/master/debian/patches/0003-Using-the-system-pybind11.patch

But I'm pretty sure the patch is right. If you don't get to it first, I'll mess with it later.

jlblancoc commented 1 year ago

https://salsa.debian.org/science-team/gtsam/-/blob/master/debian/copyright#L5-8

Well done!

I'm a DD, and don't need a sponsor. One less thing to think about.

Great!

I'm looking into the cmake issue (cloned with gbp), but found this: 

$ gbp buildpackage 
gbp:error: upstream/4.2a9+dfsg is not a valid treeish

Any alternative way or easy fix? I'm trying to build using "debuild" in the meanwhile, but it's a bit painful... also, I would prefer to have gbp right and run within a Debian/sid docker image...

dkogan commented 1 year ago

Thank you very much for looking into this. Here's how you can reproduce on Debian/sid:

That should be enough to see the problem. You can also strip all the Debian bits out of it. Instead of the "dpkg-buildpackage":

Thanks

jlblancoc commented 1 year ago

Great thanks, I'll try! I prefer trying the actual debian commands to reproduce all possible problems with fortify and other funny flags ;⁠-⁠)

jlblancoc commented 1 year ago

@dkogan the pybind11 error is easy to fix, look https://github.com/borglab/gtsam/pull/1487

If/when all those latest PRs are merged, you could delete the corresponding d/patches after the next gtsam release or if you wrap a snapshot.

dkogan commented 1 year ago

Thank you so much for fixing that!

I'm not looking at any of this at the moment; will get back to it in a week or two. When we're ready to push the package, we should use the latest release, whatever that is. If it includes the updates you just made, we'll drop the patches.

Oh, and if you want to contribute more directly to the Debian package, you can do stuff in the salsa.debian.org repo; I don't need to own it. In fact, you can take it over, and I can review and sponsor the upload, when it's ready. Let me know if you want to do that, and I'll get you started.

jlblancoc commented 1 year ago

Oh, thanks! But I'm probably into already too many projects to take it over... Still, I can, and will probably do, PRs to the salsa repo when needed.