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

Bump debhelper compat to 11 #643

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Bump debhelper compat to 11 #643

wants to merge 1 commit into from

Conversation

jspricke
Copy link
Contributor

@jspricke jspricke commented Jul 5, 2021

@jspricke
Copy link
Contributor Author

jspricke commented Jul 5, 2021

This enables parallel builds by default and more, see man debhelper for a list.

@nuclearsandwich nuclearsandwich self-assigned this Jul 12, 2021
@nuclearsandwich
Copy link
Contributor

This enables parallel builds by default and more, see man debhelper for a list

This is a potentially high-impact change. I don't think the ROS buildfarm is set up to handle parallel builds within the binarydeb jobs in a deterministic fashion.

From the debhelper(7) page in Buster:

Changes from v9 are:

dh_installinit will no longer install a file named debian/package as an init script.
dh_installdocs will error out if it detects links created with --link-doc between packages of architecture "all" and non-"all" as it breaks binNMUs.
dh_installdeb no longer installs a maintainer-provided debian/package.shlibs file. This is now done by dh_makeshlibs instead.
dh_installwm refuses to create a broken package if no man page can be found (required to register for the x-window-manager alternative).
Debhelper will default to --parallel for all buildsystems that support parallel building. This can be disabled by using either --no-parallel or passing --max-parallel with a value of 1.
The dh command will not accept any of the deprecated "manual sequence control" parameters (--before, --after, etc.). Please use override targets instead.
The dh command will no longer use log files to track which commands have been run. The dh command still keeps track of whether it already ran the "build" sequence and skip it if it did.

The main effects of this are:

With this, it is now easier to debug the install or/and binary sequences because they can now trivially be re-run (without having to do a full "clean and rebuild" cycle)
The main caveat is that dh_* now only keeps track of what happened in a single override target. When all the calls to a given dh_cmd command happens in the same override target everything will work as before.

Example of where it can go wrong:

  override_dh_foo:
    dh_foo -pmy-pkg

  override_dh_bar:
    dh_bar
    dh_foo --remaining
    

In this case, the call to dh_foo --remaining will also include my-pkg, since dh_foo -pmy-pkg was run in a separate override target. This issue is not limited to --remaining, but also includes -a, -i, etc.

The dh_installdeb command now shell-escapes the lines in the maintscript config file. This was the original intent but it did not work properly and packages have begun to rely on the incomplete shell escaping (e.g. quoting file names).
The dh_installinit command now defaults to --restart-after-upgrade. For packages needing the previous behaviour, please use --no-restart-after-upgrade.
The autoreconf sequence is now enabled by default. Please pass --without autoreconf to dh if this is not desirable for a given package
The systemd sequence is now enabled by default. Please pass --without systemd to dh if this is not desirable for a given package.
Retroactively removed: dh no longer creates the package build directory when skipping running debhelper commands. This will not affect packages that only build with debhelper commands, but it may expose bugs in commands not included in debhelper.

This compatibility feature had a bug since its inception in debhelper/9.20130516 that made it fail to apply in compat 9 and earlier. As there has been no reports of issues caused by this bug in those ~5 years, this item have been removed rather than fixed.

v11
Changes from v10 are:

dh_installinit no longer installs service or tmpfile files, nor generates maintainer scripts for those files. Please use the new dh_installsystemd helper.
The dh_systemd_enable and dh_systemd_start helpers have been replaced by the new dh_installsystemd helper. For the same reason, the systemd sequence for dh has also been removed. If you need to disable the dh_installsystemd helper tool, please use an empty override target.

Please note that the dh_installsystemd tool has a slightly different behaviour in some cases (e.g. when using the --name parameter).
dh_installdirs no longer creates debian/package directories unless explicitly requested (or it has to create a subdirectory in it).

The vast majority of all packages will be unaffected by this change.
The makefile buildsystem now passes INSTALL="install --strip-program=true" to make(1). Derivative buildsystems (e.g. configure or cmake) are unaffected by this change.
The autoconf buildsystem now passes --runstatedir=/run to ./configure.
The cmake buildsystem now passes -DCMAKE_INSTALL_RUNSTATEDIR=/run to cmake(1).
dh_installman will now prefer detecting the language from the path name rather than the extension.
dh_auto_install will now only create the destination directory it needs. Previously, it would create the package build directory for all packages. This will not affect packages that only build with debhelper commands, but it may expose bugs in commands not included in debhelper.
The helpers dh_installdocs, dh_installexamples, dh_installinfo, and dh_installman now error out if their config has a pattern that does not match anything or reference a path that does not exist.

Known exceptions include building with the nodoc profile, where the above tools will silently permit failed matches where the patterns are used to specify documentation.
The helpers dh_installdocs, dh_installexamples, dh_installinfo, and dh_installman now accept the parameter --sourcedir with same meaning as dh_install. Furthermore, they now also fall back to debian/tmp like dh_install.

Migration note: A bug in debhelper 11 up to 11.1.5 made dh_installinfo incorrectly ignore --sourcedir.
The perl-makemaker and perl-build build systems no longer pass -I. to perl. Packages that still need this behaviour can emulate it by using the PERL5LIB environment variable. E.g. by adding export PERL5LIB=. in their debian/rules file (or similar).
The PERL_USE_UNSAFE_INC environment variable is no longer set by dh or any of the dh_auto_* tools. It was added as a temporary work around to avoid a lot of packages failing to build at the same time.

Note this item will eventually become obsolete as upstream intends to drop support for the PERL_USE_UNSAFE_INC environment variable. When perl drops support for it, then this variable will be removed retroactively from existing compat levels as well.
The dh_makeshlibs helper will now exit with an error if objdump returns a non-zero exit from analysing a given file.
The dh_installdocs and dh_installexamples tools may now install most of the documentation in a different path to comply with the recommendation from Debian policy §12.3 (since version 3.9.7).

Note that if a given source package only contains a single binary package in debian/control or none of the packages are -doc packages, then this change is not relevant for that source package and you can skip to the next change.

By default, these tools will now attempt to determine a "main package for the documentation" (called a doc-main-package from here on) for every -doc package. If they find such a doc-main-package, they will now install the documentation into the path /usr/share/doc/doc-main-package in the given doc package. I.e. the path can change but the documentation is still shipped in the -doc package.

The --doc-main-package option can be used when the auto-detection is insufficient or to reset the path to its previous value if there is a reason to diverge from Debian policy recommendation.

Some documentation will not be affected by this change. These exceptions include the copyright file, changelog files, README.Debian, etc. These files will still be installed in the path /usr/share/doc/ package.
The dh_strip and dh_shlibdeps tools no longer uses filename patterns to determine which files to process. Instead, they open the file and look for an ELF header to determine if a given file is an shared object or an ELF executable.

This change may cause the tools to process more files than previously.

@jspricke
Copy link
Contributor Author

jspricke commented Jul 12, 2021 via email

@nuclearsandwich
Copy link
Contributor

My biggest concerns are cross-build resource usage and log output determinism.

We currently provision the build farm to run 4 executors (4 simultaneous Jenkins jobs) on a 4 core CPU VM instance, that is, 1 CPU core per executor, and if we enable the parallel features then there will be more contention between jobs running on the same instance resulting in less consistent and predictable build times. As you mention there are ways to influence the number of parallel jobs but we'd want to make sure that we employ these in our build farm configurations rather than in the bloom templates since otherwise the updated feature would not bring any benefit to others who want to opt in to the parallel builds.

I'm additionally concerned about build log output being out of order or inconsistently ordered in our build farm logs, thus making problems harder to diagnose.

The other changes do look like benefits on the surface. My experience tells me that any change, positive or not, usually comes with some consequences.

The separation between bloom execution time and package build time also makes this more challenging to partially roll out or roll back if needed. My favorite approach for things like this is to roll them out in stages. First trying it in Rolling and then extending it to other distributions as it proves itself. But that doesn't make it easy to deploy for your own distributions until the general rollout.

@roehling
Copy link

roehling commented Jul 12, 2021 •
edited
Loading

As you mention there are ways to influence the number of parallel jobs but we'd want to make sure that we employ these in our build farm configurations rather than in the bloom templates since otherwise the updated feature would not bring any benefit to others who want to opt in to the parallel builds.

I think this is a point in favor of bumping the compat level, because with Debhelper 10+ you can fully determine parallelism via the DEB_BUILD_OPTIONS environment variable, and with Debhelper 9, you need to patch the debian/rules files to use dh --parallel.

I'm additionally concerned about build log output being out of order or inconsistently ordered in our build farm logs, thus making problems harder to diagnose.

I agree that parallelism for the ROS build farm is likely more trouble than it is worth, considering that the workload if the builder is generally high enough to achieve parallelism by building multiple packages. But as outlined above, the compat bump gives more flexibility to others without sacrificing anything for the build farm.

The other changes do look like benefits on the surface. My experience tells me that any change, positive or not, usually comes with some consequences.

I think you overestimate the complexity of this migration, because unlike the vast Debian ecosystem, all ROS packages are built from the very restricted template files in bloom/generators/debian/templates. Most of the changes can be almost instantly ruled as as irrelevant because the affected dh_* helpers are not used at all. Looking at the remaining stuff:

  • the cmake buildsystem now passes -DCMAKE_INSTALL_RUNSTATEDIR=/run to cmake: harmless
  • dh_auto_install will now only create the destination directory it needs. Previously, it would create the package build directory for all packages. This will not affect packages that only build with debhelper commands (…): also harmless
  • dh_makeshlibs will now exit with an error if objdump returns a non-zero exit from analysing a given file: will only fail if a package ships with a broken ELF binary, which should be flagged as a bug anyway IMHO.
  • dh_strip and dh_shlibdeps no longer use filename patterns to determine which files to process. Instead, they open the file and look for an ELF header to determine if a given file is an shared object or an ELF executable. This would only have a visible effect if the package maintainer explicitly instructed cmake to output shared objects with a file name without .so in it, and in that case it would arguably do the right thing, i.e. add the required dependencies to the package metadata.

Personally, I would go even further and bump to compat level 12, at least for distributions that use Ubuntu 20.04 or later.
What will become more problematic is Debhelper 13, because it turns uninstalled files from debian/tmp into a hard error, and Debhelper 14, which will disable RPATH unconditionally for cmake and thus affect dh_auto_test runs.

@jspricke
Copy link
Contributor Author

jspricke commented Jul 16, 2021 via email

@mikepurvis
Copy link
Contributor

mikepurvis commented Jul 16, 2021 •
edited
Loading

It would probably be good to collect some statistics if the job tree is rather deep or wide and decide based on this.

Off the cuff, I would say that the public ros/rosdistro tends to be wide (vast numbers of unrelated packages which mostly just depend on a handful of msg and library packages), whereas private ones tend to be deep (substantial chains of functionality that roll up to a handful of metapackages defining a product, possibly less of an emphasis on composability by interface).

Now that said, we haven't used Bloom for years as needing to create tags to get binaries doesn't fit our development model. And I know some other groups have gone to similar approaches with containerization, bazel-based ROS builds, and so on. So it probably doesn't make too much sense to optimize Bloom for anything other than the needs of the OSRF-maintained build farm.

@jspricke
Copy link
Contributor Author

jspricke commented Jul 16, 2021 via email

@mikepurvis
Copy link
Contributor

Oh I don't doubt that it's usable (and indeed used) in private systems, but I guess another way to look at it is that the deeper your dependency tree is, the more you will run up against the limitations of not being able to do broad-scale integration testing ahead of merging or even tagging. That "one repo at a time" approach to prerelease testing hinges on a) the APIs of your dependencies being completely locked down and b) no feature or piece of functionality ever spanning multiple repos in a way that could require simultaneous updates.

Anyway, didn't mean to hijack here— despite that I believe ros/rosdistro to be largely wide rather than deep, I would still say it's probably worth turning on parallel builds be default for those cases where roscpp or a msg package does change, and you want to clear the initial dependencies as quickly as possible to get to work on the main compilations.

@jspricke
Copy link
Contributor Author

jspricke commented Jul 17, 2021 via email

@russkel
Copy link

russkel commented Mar 28, 2022

I am getting this when building on Ubuntu jammy:

dh binary -v --buildsystem=cmake --builddirectory=.obj-x86_64-linux-gnu
dh: warning: Compatibility levels before 10 are deprecated (level 9 in use)

This issue should probably be revisited.

@nuclearsandwich
Copy link
Contributor

This issue should probably be revisited.

Yep. It has been in my queue. It hasn't been a top priority item because things which are "deprecated" will remain present for the lifetime of Ubuntu 22.04. That isn't an excuse to wait until the last minute but we've been hitherto occupied with changes that are happening in 22.04 rather than after 22.04.

As previously discussed, the question is not whether to make the change. We must. But how to do it in a way that is well tested, minimally disruptive, and still satisfying to highly motivated bloom use cases like @jspricke's.

I'm pretty resistant to making the debhelper compat version a configurable setting as Bloom is meant to help package maintainers by trying to insulate them from the intricacies of packaging by adhering to a common set of practices that are mostly suitable for most packages.

It is possible right now for a package to change the debhelper compat version on a per-package basis using bloom's patch workflow.

I think this is a point in favor of bumping the compat level, because with Debhelper 10+ you can fully determine parallelism via the DEB_BUILD_OPTIONS environment variable, and with Debhelper 9, you need to patch the debian/rules files to use dh --parallel.

No disagreement that this is a better, more consistent interface, but that does not mean getting from here to there requires no engineering time compared with retaining the status quo (where it is neither parallel nor configurable by default).

The ROS build farm triggers all downstream jobs by default. Reducing the build time of single jobs by building in parallel

The right thing to do is measure and given the time there are dozens of different combinations and configurations I would love to test. But by my observation many ROS 2 packages are on the small side and the majority of the build time is spent configuring the environment rather than building the package. In-build parallelization will help some of the larger packages like rclcpp and rviz but my expectation is that it won't be worth the loss of deterministic build logs or multi-executor agent hosts. If we move to parallel builds on the build farm it would most likely be as part of a much larger effort to improve build farm throughput.

I think you overestimate the complexity of this migration, because unlike the vast Debian ecosystem, all ROS packages are built from the very restricted template files in bloom/generators/debian/templates. Most of the changes can be almost instantly ruled as as irrelevant because the affected dh_* helpers are not used at all. Looking at the remaining stuff:
attractive. I l
* the cmake buildsystem now passes -DCMAKE_INSTALL_RUNSTATEDIR=/run to cmake: harmless

* `dh_auto_install` will now only create the destination directory it needs. Previously, it would create the package build directory for all packages. This will not affect packages that only build with debhelper commands (…): also harmless

* `dh_makeshlibs` will now exit with an error if objdump returns a non-zero exit from analysing a given file: will only fail if a package ships with a broken ELF binary, which should be flagged as a bug anyway IMHO.

* `dh_strip` and `dh_shlibdeps` no longer use filename patterns to determine which files to process. Instead, they open the file and look for an ELF header to determine if a given file is an shared object or an ELF executable. This would only have a visible effect if the package maintainer explicitly instructed `cmake` to output shared objects with a file name without `.so` in it, and in that case it would arguably do the right thing, i.e. add the required dependencies to the package metadata.

Thanks for taking the time to review the changelogs and share your summary! It's great to have additional eyes on these things.

I think you overestimate the complexity of this migration...

I'm not so much estimating the complexity but rather the small, subtle, changes which may not even be captured in the changelog documentation. This is not meant to disparage the efforts of the Debian project and team, who in general produce fantastic change logs and documentation, but to acknowledge that when working with complex software at this scale there's almost always "something" that wasn't anticipated. This isn't an urgent change that needs to be made because the status quo is unsound and as such I'd like to be deliberate enough that we don't upset that stability.

Personally, I would go even further and bump to compat level 12, at least for distributions that use Ubuntu 20.04 or later.

I find this idea appealing because it pushes us to start being more proactive

What will become more problematic is Debhelper 13, because it turns uninstalled files from debian/tmp into a hard error, and Debhelper 14, which will disable RPATH unconditionally for cmake and thus affect dh_auto_test runs.

Bloom was originally written with debian/compat at 7, updated to 9 in #277 (then conditionally in #279) which was 2014 and then not changed since then. It would be nice to have a strategy for reviewing and phasing in new compat versions as they become available so we do the research and testing more frequently and while eyes are on a new platform, rather than making a change that would retroactively affect earlier platforms, which is one concern about rolling out compat 11 on all platforms now.

What will become more problematic is Debhelper 13, because it turns uninstalled files from debian/tmp into a hard error, and Debhelper 14, which will disable RPATH unconditionally for cmake and thus affect dh_auto_test runs.

As @jspricke mentioned above, I'm not sure the that change in debhelper 13 affects bloom while we're still producing only a single package per source package. If we ever do implement package splitting handling that will be one of things we have to do to bring that up.

The RPATH restrictions concern me slightly more since there are ROS packages using RPATH even outside of testing binaries which could be affected. However, the release engineer in me agrees with the change and so while I'm apprehensive I'm also a little eager :)

I think the next steps for this change are:

  • Decide which distributions it should apply to by default (package maintainers can always patch to override)
  • Update ros_buildfarm to add --no-parallel or --max-parallel=1 (easier to override) to DEB_BUILD_OPTIONS by default and plumb a config option to change them.
  • Use a development release of bloom to run the rosdistro migration tools on Rolling or a subset of it and build the world on a test build farm to check for package soundness.
  • Revisit based on test results the decisions about applicable distributions.
  • Deploy in a released version of bloom.

Not listed above is buying @jspricke a 🍺, mostly because we shouldn't wait until the end to do so.

@Aposhian
Copy link

I'm pretty resistant to making the debhelper compat version a configurable setting as Bloom is meant to help package maintainers by trying to insulate them from the intricacies of packaging by adhering to a common set of practices that are mostly suitable for most packages.

That's what default config is for, right? Purposefully not making things configurable just decreases your user base. For example, I would like to use bloom generators to work with ROS packages, even though I don't want to use the gbp release process, but things like this make me want to get away from a dependency on bloom as soon as I can (or hard fork it).

@jspricke
Copy link
Contributor Author

I would like to use bloom generators to work with ROS packages

This is actually not that hard, see my minimal example here: ros-industrial/industrial_ci#697 (comment)

I actually think it would be easier if the buildfarm does the blooming and accepts tagged upstream repos instead of gbp releases. That way it would be easy to bump the compat version and test it or do other fixes. I did that for the last big buildfarm I build and it worked really well.

@Aposhian
Copy link

Yes, I guess I should rephrase: I am already using the bloom generators in my custom build, but wishing I could configure the debhelper compat version without a fork.

It is possible right now for a package to change the debhelper compat version on a per-package basis using bloom's patch workflow.

@nuclearsandwich could you elaborate on this? I'm having trouble finding how to do this in the docs

@jspricke
Copy link
Contributor Author

configure the debhelper compat version without a fork.

See my script linked above ;).

@Aposhian
Copy link

Oh, I missed the key line you were referring to. You are just overriding it in the file after the fact. I suppose that is simple enough. Thanks!

echo 11 > debian/compat

@jspricke
Copy link
Contributor Author

just to say, nowadays we prefer a Depends: debhelper-compat (= 13) in debian/control and no debian/compat but that's not that important.

@jspricke jspricke mentioned this pull request Jan 9, 2024
Open
@Timple
Copy link
Contributor

Timple commented Apr 2, 2024

By now the version might as well be 12: https://packages.ubuntu.com/search?suite=all&searchon=names&keywords=debhelper

@jspricke
Copy link
Contributor Author

jspricke commented Apr 2, 2024

Well, 13: https://manpages.debian.org/unstable/debhelper/debhelper-compat-upgrade-checklist.7.en.html#v13

This was referenced Jun 10, 2024
Open
Closed
@cottsay cottsay mentioned this pull request Dec 9, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@nuclearsandwich nuclearsandwich

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@jspricke @nuclearsandwich @roehling @mikepurvis @russkel @Aposhian @Timple