Skip to content

test(release): remove the dead INSTALL_DIRS derivation and pin the survivor - #453

Open
kragent66-glitch wants to merge 1 commit into
lacs-project:mainfrom
kragent66-glitch:fix/dead-install-dirs-derivation
Open

kragent66-glitch wants to merge 1 commit into
lacs-project:mainfrom
kragent66-glitch:fix/dead-install-dirs-derivation

Conversation

@kragent66-glitch

Copy link
Copy Markdown
Contributor

Summary

tests/release/install-paths.test.sh derived INSTALL_DIRS twice, three lines apart, and the first derivation was both unused and wrong.

Line 54 read one physical line with sed, which stops at the backslash and loses $(SUDOERS) and $(HELPERS). Line 56 then overwrote it unconditionally with the awk that reads the whole logical line. Nothing read the first value — and ShellCheck at --severity=warning, which is what e2e / scripts-lint runs, does not report an unused assignment — so it sat there looking authoritative above the reader that works. The next person to read those four lines sees two spellings of one idea and a coin flip over which to delete; pick wrong and the check stops covering $(HELPERS), which is the path #301 is about.

This removes the dead derivation and adds the assertion that says why the survivor is the one that survived.

Two changes beyond the deletion, both in service of that assertion:

  • The derivation moves into install_dirs_vars(), so the fixture exercises the real reader instead of a copy of it. Extracting it is what makes the new assertion mean anything.
  • The comment that sat between the two derivations now sits above the awk it describes.

The sed at line 48, which extracts PREFIX from a genuinely single-line assignment, is untouched.

Related Issue

Closes #449

Validation

  • Tests added or updated
  • Documentation updated if behavior changed — nothing to document: one dead line removed, the surviving derivation extracted, one assertion added
  • Security impact considered — the assertion guards the coverage Fedora Atomic: make install has never completed, and it fails halfway #301 depends on ($(HELPERS)), so deleting the wrong reader can no longer silently disable that check
  • Trust boundary preserved (daemon remains the only privileged executor) — no daemon, IPC, or polkit surface touched
  • CI passes — not yet run

Clean run, byte-identical at 30b9e31 (the commit the issue measured against):

$ bash tests/release/install-paths.test.sh
install-paths: KNOWN GAP HELPERS defaults to /usr/lib/sysknife and is not redirected on rpm-ostree (#301)
install-paths: INSTALL_DIRS covers all 7 recipe destinations; 7 defaults checked against the ostree overrides
rc=0

Mutation, using the method from the issue — copy the Makefile, tests/e2e/provision.sh and this test into a scratch tree, then swap the surviving awk for the line-54 sed:

$ bash /tmp/skip449/tests/release/install-paths.test.sh
install-paths: a line-continued INSTALL_DIRS lost $SUDOERS; the reader stopped at the backslash
install-paths: a line-continued INSTALL_DIRS lost $HELPERS; the reader stopped at the backslash
install-paths: daemon-install writes into $(HELPERS) but INSTALL_DIRS omits it, so the preflight never checks it
install-paths: daemon-install writes into $(SUDOERS) but INSTALL_DIRS omits it, so the preflight never checks it

install-paths: 4 failure(s)
rc=1

The two daemon-install writes into … lines are the failure the issue predicted from the clean run. The two lost $… lines are the new assertion naming the variables the bad reader dropped, which is what makes the mutation legible without diffing the two readers by eye.

ShellCheck, the same invocation CI uses:

$ shellcheck --severity=warning tests/release/install-paths.test.sh
(no output)

Notes for Reviewers

  • cargo nextest run --workspace --locked: NOT RUN. This box has no glib-2.0 development headers, so the workspace does not compile here — glib-sys fails at its pkg-config probe with exit code 101. That is pre-existing and unrelated to this diff, which touches no Rust code. I did run the build step that e2e / scripts-lint actually performs, cargo build -p sysknife-cli --locked, and it passes here (Finished dev profile in 1m 38s).
  • The assertion is deletion-safe by design: it fails if the surviving reader is replaced by one that stops at the backslash, and passes when the survivor is kept. It pins the property rather than the file's shape.
  • AI assistance: drafted with Hermes (an AI agent) against the no-dead-code rule in CLAUDE.md and the AI section of CONTRIBUTING.md. Every command output quoted above comes from a run reproduced on 30b9e31, not from the draft.

…rvivor (lacs-project#449)

tests/release/install-paths.test.sh derived INSTALL_DIRS twice, three lines
apart, and the first derivation was both unused and wrong. The `sed` read one
physical line, so it stopped at the backslash and lost $(SUDOERS) and
$(HELPERS). The `awk` on the next line overwrote it unconditionally, and
ShellCheck at --severity=warning does not report an unused assignment, so the
dead line sat above the reader that works, looking authoritative.

Deleting a line is not the deliverable. The derivation moves into
install_dirs_vars() so a fixture can exercise the real reader rather than a copy
of it, and the new assertion fails naming the variables a reader that stops at
the backslash loses.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

install-paths.test.sh computes INSTALL_DIRS twice and throws the first answer away

1 participant