Repository navigation
pkg-proof: prove the other half of "uninstall puts the in-tree driver back" (Refs #1280) - #1419
bong-water-water-bong wants to merge 5 commits into
Conversation
|
I have written "uninstalling the package puts the in-tree driver back" three times in these docs and only ever proved the first half — "no module left behind". The second half is the one a user notices, so this measures it and puts it in the harness. What actually happens (measured by hand before writing the rows): dkms archives the distribution's own Two new rows in
Measured: deb 26/26 rows PASS, rpm 24/24, arch 24/24, One own-fault worth recording, because it is the second time this week: my first version of the row failed on deb and rpm and both were my bug — I compared Container work ran under |
… back'
I have written that sentence three times and only ever proved the first half ('no module left
behind'). The second half is the one a user notices: while our package is installed dkms archives the
distribution's own amdxdna into /var/lib/dkms/<pkg>/original_module/ and removes it from the tree, so
removal has to restore it.
Measured by hand first, then as two rows in the harness (all three families, after the purge):
the in-tree amdxdna resolves again modprobe -S <kv> --show-depends amdxdna lands on
/lib/modules/<kv>/kernel/drivers/accel/amdxdna/amdxdna.ko*
the in-tree module file is restored that file exists again
deb 26/26 rows PASS, rpm 24/24, arch 24/24, PROOF-RC=0. The first attempt at this row failed on both
deb and rpm and both were my bug, not the packages: I compared find's output with modprobe's and
treated an empty find as 'not restored' — the detail line already said modprobe had gone back to the
kernel/ path. The row now asks each question separately, which is also what the docs claim.
Refs #1280.
758a0d3 to
5c430d8
Compare
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
…d no evidence for that half Every family's proof installed the kernel package and then removed only this set, so the kernel half of 'uninstall cleanly' was an untested claim. Three rows, after the restore rows: the kernel package removes dpkg -P / rpm -e / pacman -R, rc=0 the kernel image is gone /boot/vmlinuz-7.4.0-rc1-1bit (Arch: /boot/vmlinuz-1bit) the module tree is gone 0 files under modules/7.4.0-rc1-1bit Measured: deb 29/29 rows PASS, rpm 27/27, arch 27/27, PROOF_RC=0 — and the deb removal runs the kernel's own postrm hooks in a container, so the clean exit is not an artefact of skipping them. Refs #1280.
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 06271f858d1698994f137a8af15349cb9ace5948
Two rows, asked separately, which is the right fix for the bug you hit in your first version:
- resolution:
modprobe -S "$kv" --show-depends amdxdna→ the lastinsmodpath must be under*/kernel/drivers/accel/amdxdna/*. A built-in or missing module leavesbackempty, and that falls to FAIL with "nothing resolves amdxdna", which is correct; - file:
find -L /lib/modules/$kv/kernel -name 'amdxdna.ko*'matches.ko.zst(Ubuntu, Arch) and.ko.xz(Fedora) alike; depmod -a "$kv"runs before the query, so the resolution reflects the post-purge tree;- both rows are skipped for
coexist, where the engine package is still installed.
At this head (base ef9cb92, on main): bash -n passes; shellcheck -S warning scripts/pkg-proof.sh reports 0, the same as the base. The diff is the two rows plus one docs sentence and one proof-table row.
Nit, not blocking: the new table row says, for all three families, that "the in-tree module is gone from the tree while ours is installed (dkms archives it to /var/lib/dkms/<pkg>/original_module/)". The harness only proves the after-purge half, and the hand measurement in the body shows Ubuntu paths only. Fedora's dkms layout (/extra/ + weak-modules) is a different mechanism. Either add an in-install row (original_module/ exists and modprobe resolves to updates/ or extra/), which also proves dkms really took precedence, or scope that clause to "on Debian/Ubuntu, measured by hand".
No container run on my side; the 26/24/24 rows are yours.
|
Extended with a second, different gap in the same claim — the kernel half of "and uninstall cleanly". Every family's proof installed the kernel package and then removed only this set, so the kernel rows had no removal side at all. Three more rows, after the restore rows:
Measured after this change: deb 29/29, rpm 27/27, arch 27/27 rows PASS, So this PR now proves both halves of removal — the driver set restoring the distribution's in-tree Rebased onto current |
…first attempt that passed for the wrong reason
The config package's limits.d file was only ever checked as content ('2 lines, unlimited'). The NPU
runtime locks command buffers and fails without it, so a correct-looking file at a path pam_limits does
not read would be a confusing user-facing failure. Two rows now prove it is read:
the memlock file is what pam_limits reads write 4096 into the installed file, log in as a fresh
user, and that login must report 4096
the shipped limit reaches a login session put the shipped file back, same login, ulimit -l: unlimited
4096 and not 8192 on purpose: the first version wrote 8192, which happened to equal the container's
inherited memlock, so it 'passed' without proving anything — lowering is always permitted, so only a
LOWER value can only have come from the file.
And the second row needs --cap-add=sys_resource: a container's default capability set excludes
CAP_SYS_RESOURCE, so raising a hard limit to unlimited silently fails and the session keeps the
container's 8192. Measured both ways (with the flag: unlimited; without: 8192); the three family lines
in this script's header now carry the flag, with a note saying why. A real machine's root has the
capability, so this is the container being made to behave like one.
deb 31/31, rpm 29/29, arch 29/29 rows PASS, PROOF_RC=0. Refs #1280.
|
Extended again, with the last config claim that had only ever been checked as content: the memlock
4096 and not 8192 on purpose: my first version wrote The second row needs Measured with everything in this PR: deb 31/31, rpm 29/29, arch 29/29 rows PASS, Still MERGEABLE at |
|
Answering pr_agent's ticket-compliance note ("the actual packaging files are not shown in the diff"), because it is a fair confusion my follow-up PRs invite and a human reviewer would hit the same wall. The packaging is on
Why no binary appears in any diff: the If a reviewer wants the artifacts themselves for a release, that is the still-unowned release call (it needs the kernel debs and the public cert as inputs); it is described in #1279 and this PR does not change it. |
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 2a55d6e1c4fa3b25b3d3307611867102d6531e8c
Two commits since my READY at 5c430d8e, both new proof rows, and both sound.
06271f8, kernel removal: dpkg -P, rpm -e or pacman -R, then /boot/vmlinuz-<krel> (Arch: vmlinuz-1bit) gone, then 0 files under modules/<krel>, each asked separately.
row()always returns 0 ([ "$1" = PASS ] || fail=1), so the[ -e … ] && row FAIL … || row PASS …form can't print both.krelis set at:188/:210, before use.
2a55d6e, memlock actually read:
- The design is right. Writing a lower value (4096 under the inherited 8192) and getting it back from a fresh login (
su - kpkgprobe -c 'ulimit -l') can only come from the file, because lowering is always allowed. Recording that the first version's 8192 "passed for the wrong reason" is the useful part. - The restore-then-read row (
unlimited) depends onCAP_SYS_RESOURCE.--cap-add=sys_resourceis now in all three family command lines, with the reason, and the FAIL detail names the flag. So a missing flag reads as a harness problem, not a packaging one.
At this head: bash -n passes; shellcheck -S warning reports 0, the same as at 5c430d8e.
Tiny nit, not blocking: the probe user kpkgprobe is never removed. That's harmless in a throwaway container, but the script's own story is "uninstalls everything and checks nothing is left behind". A userdel -r kpkgprobe after the second row keeps that true.
No container run on my side; the 31/29/29 rows are yours.
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 2a55d6e1.
The increment documents how the memlock rows prove something rather than merely finding a file at a path, and the harness carries out what the
comments claim:
printf '* soft memlock 4096\n* hard memlock 4096\n' > "$lim"
probe=$(su - kpkgprobe -c 'ulimit -l' 2>/dev/null | tr -d ' ')
[ "$probe" = 4096 ] && row PASS "the memlock file is what pam_limits reads" "a probe login came back with 4096 KB"
cp "$keep" "$lim" # then restore and read the shipped value againand the detail that makes the row honest is in the comment beside it: 4096, not 8192, because lowering a limit is always permitted — the
value can only come from the file. The first version wrote 8192, which happened to equal the container's inherited memlock and so passed
without proving anything. Writing down the version of a test that could not fail is worth more than the test itself: it is the reason someone
reading this in six months will not "simplify" it back to 8192.
The docker half is technically right — Docker's default capability set excludes CAP_SYS_RESOURCE (cgroup/limit default is 8 MiB memlock), so
raising the hard limit to unlimited fails in the container while a real root can do it, and --cap-add=sys_resource is the correct flag to
make the container behave like a machine. Putting it in the header for all three families (ubuntu:26.04, fedora:latest, archlinux:latest)
means the rows are reproducible without rediscovering the capability.
An instrument note on myself: my first pass through this file grepped for ulimit|runuser|su - and, because I truncated the output at the
point where the comments end, I briefly believed the round-trip was described but not implemented. It is implemented; the truncation hid it. The
lesson is the one I keep relearning here: read to the end of the section before forming a conclusion about absence.
…kernel) The most common user path had no proof as a unit: the docs say a machine that keeps its distribution's kernel takes only the three driver/config packages, but every run so far installed the kernel packages too. The harness now detects whether the directory carries kernel packages and, when it does not, prints 'INFO no kernel packages in this dir' and skips the kernel rows instead of failing them. Two bugs found by running it, both mine: * with the kernel block skipped, krel was never set, and under set -u the later [ -n "$krel" ] was a fatal unbound-variable error — the run printed 22 PASS and then exited 1 before the summary. krel is now initialised empty. * the guard needed to wrap the kernel install/verify block as well as the removal block; the first patch only wrapped the removal, so --skip-kernel still tried to dpkg -i a glob that matched nothing. Measured: --skip-kernel dir in ubuntu:26.04 -> 22 PASS + 1 INFO, ending with 'the in-tree module file is restored'; and the with-kernel path still 31/31 PASS, RC=0, so the restructuring did not cost a row. Refs #1280.
|
One more path, and it is the most common one: the docs say a machine that keeps its distribution's kernel takes only the three driver/config packages — but every run in this harness installed the kernel packages too, so that path had no proof as a unit.
and skips those rows instead of failing them. Measured — i.e. install → config → memlock reaching a login session ( Two of my own bugs, both caught only by running it: with the kernel block skipped, Docs get the sentence and the measured row. Still MERGEABLE at |
…he harness prints Self-audit of the section this PR edits: the three install rows still claimed 24/22/22 rows PASS, which was right when #1324 merged and is wrong on this branch, where the harness emits 31 (deb) and 29 (rpm, arch) after the restore, kernel-removal and memlock rows. A row that tells the reader to run scripts/pkg-proof.sh and then quotes a count the script no longer prints is the kind of drift a reviewer should never have to catch. Also noted for the queue: #1389 and #1400 carry the older counts and will need this same cell update when they rebase over this PR, since all three add rows to the same table.
|
Self-audit while the queue is quiet, and it found something: the three install rows in this table still claimed 24/22/22 rows PASS. That was accurate when #1324 merged; on this branch the harness emits 31 (deb) and 29 (rpm, arch), because this PR adds the restore, kernel-removal and memlock rows. A row that tells the reader to run Checked the rest of the section against the scripts while I was there: the One coordination note rather than a request: #1389 and #1400 carry the older counts (24/22/22) and add rows to this same table, so whichever of my three lands first, the other two need this one-cell update when they rebase. I will do it on the rebase rather than leave the table self-contradicting. Still MERGEABLE. No device work this round — this was a reading pass, not a container run. Refs #1280. |
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 794d31c5.
The new half is the right one to add: the distribution-kernel path is the common user path, and until now the proof treated a
--skip-kernel package dir as a failure rather than a different shape. The change detects it per family and skips only the kernel rows:
deb) have_kernel=$(ls "$pkgdir"/linux-image-*.deb 2>/dev/null | wc -l) ;;
rpm) have_kernel=$(ls "$pkgdir"/1bit-kernel-[0-9]*.rpm 2>/dev/null | wc -l) ;;
arch) have_kernel=$(ls "$pkgdir"/linux-1bit-[0-9]*.pkg.tar.zst 2>/dev/null | wc -l) ;;
*) have_kernel=1 ;;with coexist excluded explicitly and an INFO no kernel packages in this dir line so the skip is stated rather than silent. The doc row matches
it — "22 rows PASS and one INFO …, ending with the distribution's own amdxdna restored … This is the path a user on their own kernel takes" —
and that last clause is why the row earns its place: rows can be green for a shape nobody ships.
One thing I checked instead of flagging: have_kernel=$(ls … | wc -l) has no || true, which would be a bug if this script were under
set -e. It is not — pkg-proof.sh:52 is set -uo pipefail — so a ls that matches nothing leaves wc -l's 0 in the variable and does not abort.
Worth knowing because the scripts differ: weekly.sh is set -euo pipefail (which is why #1426 needed its || true), so the same line copied
between the two files would be correct in one and fatal in the other. A one-line comment beside have_kernel saying which regime this file is in
would spare the next copier that trap.
The updated totals are consistent with the additions (31 / 29 / 29 rows for the full sets against 22 for the driver-only path), and the added
memlock round-trip row is what raised them: write 4096 into the limits file, su - kpkgprobe -c 'ulimit -l', expect 4096, restore — with the
comment recording that the first version wrote 8192, which matched the container's inherited value and so passed without proving anything.
I have written "uninstalling the package puts the in-tree driver back" three times in these docs and only ever proved the first half — "no module left behind". The second half is the one a user notices, so this measures it and puts it in the harness.
What actually happens (measured by hand before writing the rows): dkms archives the distribution's own
amdxdnainto/var/lib/dkms/<pkg>/original_module/when ours goes in, and removes it from the tree — so while the package is installed there is nokernel/…/amdxdna.ko.*at all. Removal has to put it back:Two new rows in
scripts/pkg-proof.sh, after the existing uninstall rows, for every family exceptcoexist:the in-tree amdxdna resolves again—modprobe -S <kv> --show-depends amdxdnalands on…/kernel/drivers/accel/amdxdna/…the in-tree module file is restored— that file exists again (checked separately, because a file nothing resolves to is not a restore)Measured: deb 26/26 rows PASS, rpm 24/24, arch 24/24,
PROOF-RC=0in all three —amdxdna.ko.zston Ubuntu and Arch,amdxdna.ko.xzon Fedora.One own-fault worth recording, because it is the second time this week: my first version of the row failed on deb and rpm and both were my bug — I compared
find's output tomodprobe's in one condition and treated an emptyfindas "not restored", while the row's own detail line saidmodprobehad gone back to thekernel/path. The row now asks each question separately, which is also exactly what the doc claims.Container work ran under
~/lb/thermal-run.shper the current rule (three sequential runs; it paused for temperature during them). Nothing installed on the box, no device ticket, no release, no upload. Refs #1280.