Preserve multiarch apt package names in pkg.installed#68933
Preserve multiarch apt package names in pkg.installed#68933TobiPeterG wants to merge 1 commit intosaltstack:3006.xfrom
Conversation
|
Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here's some information that may help as you continue your Salt journey. There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar. |
TobiPeterG
changed the title
|
Good news: I found a way to test this. Update: Running Update 2; Got it to upgrade and rehold the package as expected, but salt reports that the installation still fails. It looks like that's because apt reports the final name without the architecture suffix. I'm looking into this. Update 3: I think I actually fixed this now: |
Honor split_arch in aptpkg target parsing, keep arch-qualified package names through the pkg.installed preflight path, and normalize installed package lookups during final verification so held multiarch packages can be updated correctly with update_holds=True. Add regression tests and document the provider-specific naming caveat for held/unheld packages.
|
Alright, I pushed my changes now. I tested it manually, I see: on Ubuntu 24.04 when applying this state: With both packages held to lower versions. Please note that this PR also changes the behaviour of YUM a bit and how its name matching works, but this should not affect how it is used in any way. I hope this is fine. :) |
TobiPeterG
changed the title
| elif __grains__["os_family"] == "Debian": | ||
| cver = new_pkgs.get(pkgname.split("=")[0]) | ||
| else: | ||
| cver = new_pkgs.get(pkgname) |
There was a problem hiding this comment.
Why were these 2 conditions combined?
There was a problem hiding this comment.
I combined them because both branches were doing the same installed-package lookup pattern, and the new normalization fallback needed to apply to both. I kept the Debian-specific split("=")[0] behavior via the lookup_name variable, but merged the rest to avoid duplicating the fallback logic.
I can also split them again, then it would look like this:
elif __grains__["os_family"] == "Debian":
lookup_name = pkgname.split("=")[0]
cver = new_pkgs.get(lookup_name)
if not cver and "pkg.normalize_name" in __salt__:
normalized_name = __salt__["pkg.normalize_name"](lookup_name)
if normalized_name != lookup_name:
cver = new_pkgs.get(normalized_name)
else:
cver = new_pkgs.get(pkgname)
if not cver and "pkg.normalize_name" in __salt__:
normalized_name = __salt__["pkg.normalize_name"](pkgname)
if normalized_name != pkgname:
cver = new_pkgs.get(normalized_name)
if not cver and pkgname in new_caps:
cver = new_pkgs.get(new_caps.get(pkgname)[0])
So only the initial lookup key differs. Is this the prefered solution? :)
There was a problem hiding this comment.
Another option would be:
else
if __grains__["os_family"] == "Debian":
lookup_name = pkgname.split("=")[0]
else:
lookup_name = pkgname
cver = new_pkgs.get(lookup_name)
if not cver and "pkg.normalize_name" in __salt__:
normalized_name = __salt__["pkg.normalize_name"](lookup_name)
if normalized_name != lookup_name:
cver = new_pkgs.get(normalized_name)
if not cver and pkgname in new_caps:
cver = new_pkgs.get(new_caps.get(pkgname)[0])but I think it is fine to always split on the equal sign for both like your code is already doing since = is not a valid character in rpm names either. So I think what you have is ideal if both need to do this normalization.
I don't think yum/dnf supports multi architecture packages though, so it isn't clear from the issue/PR description why yum package logic is changing. Maybe you can update the descriptions to elaborate.
2 participants
What does this PR do?
Fixes
aptpkg.install()so explicit APT multiarch package names such as:amd64are preserved whensplit_arch=Falseis passed from the genericpkg.installedstate path.This makes the APT behavior similar to the existing YUM handling and fixes a case where
pkg.installed(update_holds=True)could fail to temporarily unhold packages that dpkg records as held with an architecture suffix.This PR also adds documentation notes for the provider-specific naming requirement around held/unheld multiarch APT packages.
What issues does this PR fix or reference?
Fixes #68932
Previous Behavior
For APT,
pkg.installedpassessplit_arch=Falseintopkg.install, butaptpkg.install()did not honor that when callingpkg_resource.parse_targets().As a result, an explicit package target like:
- libnvidia-cfg1-570-server:amd64could be normalized to:
libnvidia-cfg1-570-serverbefore APT hold handling ran.
If dpkg recorded the hold as:
then
update_holds: Truecould fail to match and unhold the package correctly.New Behavior
aptpkg.install()now honorssplit_arch=Falsewhen parsing install targets, matching the existing YUM pattern.This preserves explicit APT multiarch package names such as
:amd64in the install path, allowingupdate_holds: Trueto operate on the exact held package name reported by dpkg.Merge requirements satisfied?
Tests updated:
tests/pytests/unit/modules/test_aptpkg.pyValidation performed:
split_arch=FalsepathCommits signed with GPG?
No
Is signing the commits with GPG required?
I tried to test my changes locally on a Ubuntu system to confirm that the issue is properly resolved. However, I saw this message when I tried to install salt in editable mode:
ERROR: Project file:///home/user/Documents/salt uses a build backend that is missing the 'build_editable' hook, so it cannot be installed in editable mode. Consider using a build backend that supports PEP 660.Did I do something wrong?
This means that I don't have a real world proof that this code actually fixes the issue. However, I hope that A) the issue is easily reproducable and B) this code can be easily checked by someone with a working test setup.
Until this PR is proven to solve the linked issue, I will keep it in draft mode.
EDIT: I will change it to ready and add WIP as there are very few PRs in draft mode, but quite a few with the WIP tag. I hope this change is correct.
I'm also not too experienced in python and not very familiar with the code base, so I hope this is an acceptable solution. :)
I'm of course open for feedback. :)
PS: I hope I didn't miss a point in the contribution guidelines :D