-
Notifications
You must be signed in to change notification settings - Fork 360
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
Enhance requires with version information from the build root. #2372
base: master
Are you sure you want to change the base?
Enhance requires with version information from the build root. #2372
Conversation
231eb85
to
369bf9b
Compare
I just want to mention that:
|
Understood. I don't think this PR is the right way to solve this problem. It's just a quick mock-up; I expect to throw it away and do something "the right way" later. My hope is that we can discuss whether rpm should attempt to provide minor-version information for dependencies that don't provide versioned symbols. |
A different approach, which wouldn't require calling into
This would have the following advantages:
|
Looking at the contents of /usr/lib64 on my local system, I see some cases that look like developers not versioning their so at all (e.g. libasprintf.so.0.0.0, and 144 others). But that's about 7% of all shared object files. Generally: that sounds like it'd be a major improvement, with coverage of > 90% of a typical system's dependencies. I like it, but I would hope that we could try to address the remainder, further up the stack in Fedora. Possibly: either tooling to detect shared objects that don't increment even their so version and raise an issue with the upstream project, or by modifying dnf so that its default behavior is to update dependencies on update or install transactions. |
Oh, one more caveat: if the shared library uses symbol versioning, exclude it from the new generator.
I'd be very much enforcing this as a general rule. Maintainers are supposed to express package dependencies, so that updates and fresh installs "just work". If we started enforcing automatic upgrades of all dependencies even without package requirements would mean that routine installs of leaf packages would pull in updates of hundreds of other packages. In many scenarios this would be very annoying and constraining. I think in the particular case of shared libraries, maintainers expect that the built-in generators handle this for them, but in reality those generators only works for some subset of libraries. If you can make it work for another (large) subset of packages, things will just improve overall. There might be some cases which are not handled, but I think it's better to handle the 90% in a way that has a good technical underpinning, than try for a more questionable approach in an attempt to cover "everything". |
The whole point of sonames is that it's NOT tied to versions but the soname abstraction, and this kind of dependency breaks that point. |
Understood. This mock-up only applies the externally-enhanced version info to shared objects that don't provide versioned symbols, and any future work would do the same. |
I think "the point" is that binaries should not need to re-link when a shared object is updated with a backward-compatible version. However, both the soname abstraction and rpm's current handling of it are currently slightly broken, in that there is a real existent dependency on a minimum version within the major version expressed by the soname abstraction, which rpm doesn't detect at all, and which ld.so detects by reporting unresolved symbols at startup and failing to launch a binary. Because rpm currently only tracks the major-version of the soname, it's possible for rpm (and dnf) to install a package set that doesn't actually satisfy the underlying dependencies. I provided an example in https://lists.fedoraproject.org/archives/list/[email protected]/thread/WE5UTPZ6EIMTSNUX6ILODAHRYNH6J6TM/ (see the second reference in the original email). The dependency on the updated version exists regardless of whether rpm knows about it. By improving rpm's expression of dependencies, we can make the soname abstraction more reliable. |
Yup, the soname abstraction and the libtool style major.minor.micro version in the filename don't exactly meet in practise. So reading through the comments more throroughly, turning the version in the filename (or symlink) into a version as suggested by @keszybz. That'd be enforcing the libtool style versioning, something which people rely on but nothing in the tooling actually cares. And yes, libraries doing symbol version need to be excempt from that. |
Oh and BTW, even for a proof-of-concept, you really need to start with the internal dependency generator. In the external one, the whole world-iview is upside down, and starting there is likely to run into conceptual problems elsewhere. |
I can think of one real-world case this would have broke: the SDL -> sdl12-compat transition. While now the version numbers are ahead, they weren't initially. This would have caused way too much pain to deal with. |
Would that have required Fedora to do more than either bump the so version or, worst case, manually add a Provides: line to the spec file? |
I'm going to start work on an implementation of this soon. I've been looking over elfdeps.c and thinking through implementation details, yesterday. One thing that comes to mind is that while the Provides: change would be backward compatible, the Requires: change would not. New packages would be generated with Requirements that existing packages wouldn't satisfy, which would require a "rebuild the world" deployment that obsoletes most existing rpm packages. I'm sure there will be push-back against that. Deployment of that change in a distribution might be slightly less painful if done in phases, where enhanced Provides is enabled first, and Requires in some later release. ...another thing to think about, I guess. |
The version scheme was changed to match SDL 1.2 later to deal with weaknesses in the Debian's packaging system. It was originally not that way. Bumping the soversion would have been a big deal, since it's an ABI break. I'm not really sure this is a good idea. |
I'm curious... do you have a reference that describes those weaknesses? (I have very little experience with dpkg)
I'm not suggesting bumping the major version. |
Ah, I misremembered when it came to where the weakness was. It had to do with how the Steam Runtime "judges" library versions: libsdl-org/sdl12-compat#53 (comment) |
That's the annoying thing with any dependency improvements, you need to do them in phases - provides pre-populated preferably on a mass-rebuild and only then enable the require-side. So there will need to be some kind of disable/enable control for the thing. |
I've started work on elfdeps.c... |
398827f
to
41da0a2
Compare
This version seems to work as intended. It's more complex than I'd like, but still probably better than parsing the output of ldd.
|
Well... almost as intended. Those two are unnecessary since the libraries provide versioned symbols, and shouldn't have additional version information. I'll figure out how to detect and skip that case. In the mean time, opinions on whether the rest of the approach is acceptable are welcome. |
If we assumed that the SHT_GNU_verneed header appeared before SHT_DYNAMIC, then when processing the latter, we could loop over the existing ei->requires and look for elements that start with the same filename, and skip the libtool version lookup if one is found. The only problem is that I don't know if that order is guaranteed. |
|
0af21b5
to
2075c5e
Compare
The last commit includes macros that can be used to enable the fallback dependency versions, which are off by default to provide a phased rollout of the feature. |
tools/elfdeps.c
Outdated
filename = dest; | ||
} | ||
// Start from the end of the string. Verify that it ends with | ||
// numbers and dots, preceded by ".so.". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a purely stylistical nit, //
comments do not belong in rpm coding style. Always use /* ... */
and for multiline comments,
/*
* ...
* ...
*/
/me realizes this isn't even explained in contributing.md, sigh...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll follow up on each of those items tomorrow. Thanks for the feedback!
These tests are not compatible with fakechroot, for two reasons. While fakechroot supports dlmopen(), it will convert relative paths to absolute paths, which prevents library path searching. The test could be run from the data directory so that the relative path's absolute path expansion was correct, and dlmopen() will succeed. However, linkmap->l_name will have the real path, not a chrooted path, and when that is passed to readlink(), fakechroot will try to expand a path that already includes the chroot prefix. So, even when the library can be loaded with dlmopen(), the symlink can't be resolved to find the path that contains the version.
e8c9844
to
7445008
Compare
@pmatilai , is there anything I can do to move that conversation forward? I've written a description of these changes in the form of a design proposal: https://docs.google.com/document/d/1tnEdxsh95iGkuedJ0VCWXFOawxlrD4xIqMiDqVESk9c/edit?usp=sharing ... and in the form of a Fedora change proposal: https://docs.google.com/document/d/1PcnuoW_DlE4TRqcMJPNqcp0YktO7DX34YobslnT68TI/edit?usp=sharing (Both are open for comments until spam becomes a problem.) |
from the symlink target, generate ".elf-version/<so>" files containing the value of the _elf_so_version macro. These files are then used by the elfdeps tool to generate the provides and requires.
I've added a commit to illustrate a slightly different approach. Code quality and error handling need more work, and there are no new tests, but I'd like to ignore that for the moment and discuss only whether this approach is worth developing further. This change adds another file to packages that provide ELF shared objects, and that file contains their version. By default, the version is that of the RPM package, but package maintainers could override the macro so that the maintainers of alternate library implementations can coordinate an ABI version between packages whose version does not match. This would also support a gradual rollout. Worst-case, it's still possible that any given package might not have enhanced "requires" information until two mass rebuilds were done, but the macros can be enabled at the same time and enhanced information will be added as it becomes available. One of the SUSE list members mentioned that they want to continue to be able to use LEAP packages as dependencies, and I think this approach addresses that concern. The trade-off is the proliferation of small hidden text files in the lib directories. Briefly, for example, the libnghttp2 package currently includes:
The previous version of this PR would read the version "14.24.3" from that symlink and change one of the capabilities the package provides from This version of the PR would add a file, |
Is it about using library packages from Leap (old) on Tumbleweed (new)? If so, I'd argue that is simply invalid and basically what this approach should protect against. |
@mlschroe , thoughts on this? |
It's not necessarily invalid. In Fedora, packages that fail to build from source are eventually retired along with all of their dependencies, and (basically) everything is rebuilt for every release, so we wouldn't expect old libraries to mix with new applications, there. But if SUSE doesn't retire those packages, then they could continue using an older library package in a new build root, when they build a new application that depends on an old library. In the previous version of this PR, the new binary would express a dependency that the library's package doesn't provide. In this version, packages won't express a versioned dependency on libraries that don't provide it, because they won't have a ".elf-version" file. |
For Leap vs. Tumbleweed there's a ~6y gap for some packages so I don't expect any compatibility there. What can happen is that e.g Leap 15.6 with this PR builds an application against a library from 15.5 built without this PR. That still needs to be installable, either using your |
/me nods With the .elf-versions file approach, it would be. The library from 15.5 wouldn't have the .elf-versions file, so any application built in a 15.6 build root where that package was present would simply continue to express the same type of un-versioned dependency that it does today. |
The other thing I'm wondering is whether we can (or should) label the .elf-version files, so that package managers can skip installing them, the same way that documentation is not installed in container images (RPMTRANS_FLAG_NODOCS). The .elf-version files aren't needed at runtime, they're only needed in build roots. |
It's arguably package metadata and doesn't belong in the filelist at all if possible IMO |
Generally, I agree, but I'm operating on the principal that elfdeps shouldn't access the RPM DB during the build. (And if we did access the DB for metadata, I'm not sure how we'd allow packagers to override the version. Would we need to introduce a new field?) |
One possible disadvantage: you wouldn't be able to e.g. |
That's currently possible and can lead to various subtle runtime failures instead. |
I think it's important to differentiate the real binary dependencies from RPM's knowledge of those dependencies. In Fedora 40, it was safe to downgrade xz because libsystemd had been built before xz 5.6. If it had been built after, that might not have been true. xz 5.6 introduced at least one new symbol: But as you point out, if this change was already deployed, then downgrading xz would also downgrade everything that was built against xz-5.6, which would have avoided any users mistakenly breaking everything linked to a hypothetical libsystemd that required symbols in 5.6. So as I see it, the xz downgrade is a really good illustration of why this change is needed. We were lucky that we were able to safely downgrade that package. We should not rely on luck. |
As far as I know, the blocking issue here is simply a decision about where to get the version of the library. Among others, options include: 1: the rpm version of the package that owns the library. Not a good solution because I think the maintainers don't want elfdeps to access the RPM DB during the build, and because it'd make interchangeable library implementations much harder. 2: a version string derived from the target of the symlink that ELF files refer to. This requires some coordination for interchangeable libraries. It also requires a two-phase rollout in which packages "provide" new capabilities and then later "require" those new capabilities. And once that two-phase rollout is complete, old builds (which might be present in SUSE) can't be used any more. 3: a version string stored in a text file, defaulting to the version of the package that provides the library, but optionally defined by the packagers. Requires no coordination upstream, and supports organic rollout. 4: the same version stored somewhere else. Maybe in an extended attribute. Do extended attributes work properly in environments like mock container builds? 5: do what Debian does, and make package maintainers maintain versioned symbol lists. This supports interchangeable library implementations, and organic roll-out. It also requires a lot of effort on package maintainers' parts, and I expect that the implementation would be significantly more work than the other options, unless we adapted their dep generator scripts. |
Periodic check in: Please let me know if I can do anything to move this forward. |
For shared objects without versioned symbols, this additional information allows rpm to track minor-version dependencies.
The initial version of this PR updates the external dependency generator, so it will only affect packages that set
%global _use_internal_dependency_generator 0
, so it's mostly here to facilitate discussion and testing.