engine: resources: packagekit: Use signal info for installed state#950
engine: resources: packagekit: Use signal info for installed state#950karpfediem wants to merge 1 commit into
Conversation
9288ffd to
ce437fa
Compare
PackageKit resolve results expose package state in the Package signal
info argument:
Package(info, package_id, summary)
The package_id is only the package identifier:
name;version;arch;data
mgmt was using the package ID data field to decide whether a package was
installed. That is backend-specific. Debian's apt backend can report an
installed package as:
Package(1, "qemu-utils;...;amd64;auto:debian-stable-main", ...)
while Fedora's dnf backend can report:
Package(1, "bash;...;x86_64;installed:anaconda", ...)
In both cases info=1 is PK_INFO_ENUM_INSTALLED. The difference is only
the backend-specific package ID data field.
This caused already-installed Debian packages to be classified as absent,
so mgmt repeatedly applied the package resource instead of converging.
Preserve the PackageKit signal info value while resolving packages and use
it for installed-state checks. ResolvePackages still returns package IDs;
the retained signal info is only used internally where mgmt needs package
state. There is no fallback to package ID data parsing for installed state.
Co-authored-by: Codex GPT 5.5 <noreply@openai.com>
ce437fa to
60802bd
Compare
|
PS: Without this patch ... how did this ever work on Debian??? Because I know it did. |
Did some digging with help of AI: PackageKit's aptcc backend used to emit installed packages as Then in 2022 it changed apt package IDs to encode apt install mode instead:
So Debian did work, at least with older PackageKit. I also believe it worked even on bookworm before. In any case, the fix now is to just use the Package(info, package_id, summary) signal as it is intended in the current PackageKit. |
|
Nice! Just needs the fixups I mentioned in my review before merge. |
| return obj.Info == packageInfoInstalledValue | ||
| } | ||
|
|
||
| func (obj *Conn) resolvePackagesWithInfo(ctx context.Context, packages []string, filter uint64) ([]resolvedPackage, error) { |
There was a problem hiding this comment.
Should return ([]*resolvedPackage, error)
| }, true | ||
| } | ||
|
|
||
| func (obj resolvedPackage) installed() bool { |
There was a problem hiding this comment.
ptr for the method receiver please
| return packageIDs, nil | ||
| } | ||
|
|
||
| func newResolvedPackage(body []interface{}) (*resolvedPackage, bool) { |
There was a problem hiding this comment.
please add a short docstring here and below.
Let's also move this to the bottom right after func (obj *Conn) PackagesToPackageIDs
The rough convention I have for ordering is main struct with all it's methods, secondary structs with their methods, pub helper funcs, then private helper funcs.
I might have gotten this wrong or made exceptions in some places, it's not a hard rule, but just some attempt at organization.
| if len(body) < 2 { | ||
| return nil, false | ||
| } | ||
| info, _ := body[0].(uint32) |
There was a problem hiding this comment.
please pull out the other values from the comment below and the doc comment, eg:
// format is: name;version;arch;data
and
//pkg_int, ok := signal.Body[0].(int)
If you don't want to use them, at least write the code but leave commented.
We need to check "ok" in all cases too!
| Newest bool | ||
| } | ||
|
|
||
| type resolvedPackage struct { |
There was a problem hiding this comment.
docstring plz here and for the fields.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| packageIDs := make([]string, 0, len(resolved)) |
|
|
||
| // PackageKit's Package signal uses sequential PkInfoEnum values, not the | ||
| // bitfield values used by the PkInfoEnum constants above. | ||
| const packageInfoInstalledValue uint32 = 1 |
There was a problem hiding this comment.
Are there other values here? Add them if they exist. Move up right above type PkError struct { just under the typedef consts. (But with the other consts.)
|
PS: I am stupid and forgot to click "submit review" I thought it happened with the comment. In any case, sorry about that. Patch LGTM, just needs a few fixups mentioned above. |
Fix installed-state detection for the PackageKit package resource.
PackageKit resolve results expose the package state (installed/absent, or other classification) in the Package signal
infoargument:The
package_idis only the package identifier:mgmt was using the package ID
datafield to decide whether a package was installed. That is backend-specific and breaks with theaptbackend (Debian, ...).Observed installed package data:
In both cases
info=1isPK_INFO_ENUM_INSTALLED.For an available package that is not installed (e.g.
sl) it shows 2:See the enum source in PackageKit
The package ID
datafield still shows a difference between backends:The apt backend intentionally uses
manual:/auto:package ID data for installed packages. So the portable state source is the PackageKitinfoenum, not the package ID string.Minimal reproduction
On Debian with
qemu-utilsalready installed, upstream mgmt repeatedly applies the package because it classifies the apt package ID as not installed:Fix
Preserve the PackageKit signal
infovalue while resolving packages, and use that value for installed-state checks.ResolvePackagesstill returns package IDs as before; the retained signal info is only used internally where mgmt needs package state.There is no fallback to package ID
dataparsing for installed state.Tests
The unit test covers the new condition directly:
Focused test run:
Manual smoke runs with the patched binary: