fix: gnome-autoar - remove flagged files - #18626
Conversation
There was a problem hiding this comment.
Pull request overview
Repackages gnome-autoar to remove encrypted test fixtures that block package signing.
Changes:
- Adds a deterministic source-repacking script.
- Replaces the upstream archive with the sanitized artifact.
- Refreshes generated sources, release, and lock metadata.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
base/comps/components.toml |
Moves the customized component out of the inline list. |
base/comps/gnome-autoar/gnome-autoar.comp.toml |
Configures the replacement source archive. |
base/comps/gnome-autoar/modify_source.sh |
Removes fixtures and matching test cases. |
locks/gnome-autoar.lock |
Refreshes the input fingerprint. |
specs/g/gnome-autoar/gnome-autoar.spec |
Bumps the rendered release. |
specs/g/gnome-autoar/sources |
Records the replacement archive checksum. |
Suppressed comments (1)
base/comps/gnome-autoar/modify_source.sh:129
- The checksum manifest names
${ORIGINAL_NAME}, but that path is the cached upstream tarball; the modified output is${ORIGINAL_NAME}.modified. As written,sha512sum -c "${ORIGINAL_NAME}.sha512"always checks the old archive against the new hash and fails. Point the manifest at the file whose hash was computed.
echo "${MODIFIED_SHA512} ${ORIGINAL_NAME}" > "${ORIGINAL_NAME}.sha512"
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| VERSION="0.4.5" | ||
| ORIGINAL_NAME="gnome-autoar-${VERSION}.tar.xz" | ||
|
|
||
| # Upstream Source0 (download.gnome.org) SHA512, from specs/g/gnome-autoar/sources. |
There was a problem hiding this comment.
Fixed. Clarified the comment so it's explicit that UPSTREAM_SHA512 is the pristine upstream download.gnome.org checksum used to verify the download before repacking — and explicitly not the modified/served hash that now lives in specs/g/gnome-autoar/sources. That was the only reference to specs/.../sources; I didn't find a second occurrence at line 129 (the tail of the script only computes and prints the modified hash + the upload command, which is correct).
| [[components.gnome-autoar.source-files]] | ||
| filename = "gnome-autoar-0.4.5.tar.xz" | ||
| hash = "855c016959c216b233c5a7c07c8a96f8beeb74b55154fdc054f67768c4b4935b7dc35d16dcd8aa8feee9b5ec283acaf92dd0ea686ebcd9a2a00cc6a2753da4a1" | ||
| hash-type = "SHA512" | ||
| origin = { type = "download", uri = "https://azltempstaginglookaside.blob.core.windows.net/repo/pkgs_modified/gnome-autoar/gnome-autoar-0.4.5.tar.xz/sha512/855c016959c216b233c5a7c07c8a96f8beeb74b55154fdc054f67768c4b4935b7dc35d16dcd8aa8feee9b5ec283acaf92dd0ea686ebcd9a2a00cc6a2753da4a1/gnome-autoar-0.4.5.tar.xz" } | ||
| replace-upstream = true |
There was a problem hiding this comment.
Verified via the modify_source.sh post-conditions and azldev comp render --check-only (no drift): the script checks the upstream SHA512 before editing, removes the encrypted fixtures, drops the three test_encrypted* cases and their g_test_add_func registrations, and asserts siblings (e.g. test_readonly_directory) survive. meson uses only selective -Werror= flags (no -Werror=unused-function and no blanket werror), so removing the cases doesn't break compilation. A full RPM/mock build isn't runnable in my environment; CI performs it.
bd9c314 to
6ccc9dd
Compare
| cd "${WORKDIR}" | ||
|
|
||
| echo "[1/6] Downloading ${ORIGINAL_NAME}" | ||
| [[ -f "${ORIGINAL_NAME}" ]] || curl -fsSL --retry 3 -o "${ORIGINAL_NAME}" "${UPSTREAM_URL}" |
There was a problem hiding this comment.
Fixed. The download now writes to ${ORIGINAL_NAME}.part and is renamed to the final name only on success, so an interrupted transfer can't leave a truncated file that the -f guard would silently reuse.
| ) | xz -T1 -9e > "${ORIGINAL_NAME}.modified" | ||
|
|
||
| MODIFIED_SHA512=$(sha512sum "${ORIGINAL_NAME}.modified" | awk '{print $1}') | ||
| echo "${MODIFIED_SHA512} ${ORIGINAL_NAME}" > "${ORIGINAL_NAME}.sha512" |
There was a problem hiding this comment.
Fixed. The sidecar now records the modified file's name (${ORIGINAL_NAME}.modified) written to ${ORIGINAL_NAME}.modified.sha512, so sha512sum -c verifies against the file that was actually hashed.
6ccc9dd to
6dc41e8
Compare
| EXTRACT_DIR="extracted" | ||
| rm -rf "${EXTRACT_DIR}" | ||
| mkdir "${EXTRACT_DIR}" | ||
| tar -xf "${ORIGINAL_NAME}" -C "${EXTRACT_DIR}" |
There was a problem hiding this comment.
Good catch. I pinned umask 022 before extraction so the recorded file modes no longer depend on the caller's environment. One caveat: the whole script is now Python and xz output still isn't guaranteed identical across liblzma versions, so the docstring no longer claims cross-toolchain byte-reproducibility — the artifact published to the lookaside (pinned by SHA-512 in specs/g/gnome-autoar/sources) is the source of truth the build downloads.
| TEST_FILE="tests/test-extract-unit.c" | ||
|
|
||
| SCRIPT_DIR="$(cd "$(dirname "$(realpath "$0")")" && pwd)" | ||
| REPO_ROOT="$(cd "${SCRIPT_DIR}/../../.." && pwd)" |
There was a problem hiding this comment.
suggestion(non-blocking): git rev-parse --show-toplevel would probably be more reliable here.
There was a problem hiding this comment.
Done — now uses git rev-parse --show-toplevel.
|
|
||
| SCRIPT_DIR="$(cd "$(dirname "$(realpath "$0")")" && pwd)" | ||
| REPO_ROOT="$(cd "${SCRIPT_DIR}/../../.." && pwd)" | ||
| WORKDIR="${REPO_ROOT}/base/build/work/scratch/gnome-autoar" |
There was a problem hiding this comment.
nit/suggestion(non-blocking): I'd probably pull the various instances of gnome-autoar into a variable like PACKAGE_NAME. Definitely non-blocking; take it or leave it.
There was a problem hiding this comment.
Done — added a PACKAGE_NAME constant that the paths, upstream URL and blob name all derive from.
| cd "${WORKDIR}" | ||
|
|
||
| echo "[1/6] Downloading ${ORIGINAL_NAME}" | ||
| if [[ ! -f "${ORIGINAL_NAME}" ]]; then |
There was a problem hiding this comment.
issue(non-blocking): This seems overly complicated. Is the entire point of this to prevent issues when calling this script simultaneously from two different shells? That doesn't seem worth guarding against.
Consider just bailing if the file exists or even moving on to the next step -- it will check the sha256 anyway, so you'll be guaranteed to have the right file.
There was a problem hiding this comment.
Simplified — dropped the .part/rename dance. It just bails if the file already exists, and the SHA-512 check rejects any partial or corrupt download on a re-run.
|
|
||
| echo "[5/6] Dropping encrypted test cases from ${TEST_FILE}" | ||
| export TEST_PATH="${EXTRACT_DIR}/${TOPDIR}/${TEST_FILE}" | ||
| python3 <<'PY' |
There was a problem hiding this comment.
issue(blocking): This is a lot of (embedded) python code to get rid of three test cases. Is there a reason we wouldn't use %meson_test --exclude test1 --exclude test2 --exclude test3? That's a simple overlay and gets rid of all of this, unless I'm missing something.
There was a problem hiding this comment.
The %meson_test --exclude route doesn't solve this one: the package-signing scan blocks on the encrypted input/arextract.zip files physically present in the .src.rpm, not on test execution. Excluding the tests would still ship the flagged fixtures in the source, so they have to be physically removed from the tarball (hence the repack). Dropping the three test cases is only so %check still builds once the fixtures are gone.
|
|
||
| echo "[5/6] Dropping encrypted test cases from ${TEST_FILE}" | ||
| export TEST_PATH="${EXTRACT_DIR}/${TOPDIR}/${TEST_FILE}" | ||
| python3 <<'PY' |
There was a problem hiding this comment.
issue(blocking): If we decide to keep this approach rather than %meson_test --exclude (see above comment), it makes more sense to me to have this entire script be python rather than over half of it be embedded python.
There was a problem hiding this comment.
Done — the whole script is now Python (modify_source.py); the shell wrapper is gone and the C-file edit is native Python instead of an embedded heredoc.
| modified tarball: ${WORKDIR}/${ORIGINAL_NAME}.modified | ||
| SHA512: ${MODIFIED_SHA512} | ||
|
|
||
| Upload (after \`az login\`): |
There was a problem hiding this comment.
question(blocking-on-answer): Does this actually work? I thought most people didn't have this entitlement and you could only use our pipelines to upload these files.
There was a problem hiding this comment.
You're right — most contributors don't have write access to azltempstaginglookaside; that az storage blob upload line is the maintainer/pipeline step, not something every contributor can run. I've reworded the script's printed guidance to say so (hand the file to someone with access, or use the lookaside-upload pipeline). The artifact for this PR is already uploaded and pinned by SHA-512 in specs/g/gnome-autoar/sources.
gnome-autoar ships three encrypted extract-test fixtures (tests/files/extract/test-encrypted*/input/arextract.zip) that fail the package-signing scan and block signing. %check runs %meson_test, so removing the fixtures also drops the three meson cases that read them (test_encrypted, test_encrypted_request_passphrase, test_encrypted_wrong_passphrase and their registrations); test-only, not shipped in any binary RPM. An azldev archive overlay cannot be used here because the tarball also ships an absolute-target symlink fixture (test-symlink-parent/reference/arextract -> /tmp) that azldev's overlay extractor rejects; instead modify_source.sh repacks the tarball out-of-band and it is served via origin=download.
6dc41e8 to
72895f8
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
Suppressed comments (1)
base/comps/gnome-autoar/modify_source.py:120
- Because Ruff enables all rules and does not ignore D103 (
ruff.toml:8-14), this newly added public function failsruff checkwithout a docstring.
def main() -> None:
| if not original.exists(): | ||
| # No atomic-rename dance: the SHA-512 check below rejects any partial or | ||
| # corrupt download, so a re-run simply re-fetches it. | ||
| run(["curl", "-fsSL", "--retry", "3", "-o", ORIGINAL_NAME, UPSTREAM_URL]) |
| def run(cmd: list[str], **kwargs) -> subprocess.CompletedProcess: | ||
| return subprocess.run(cmd, check=True, **kwargs) |
| return subprocess.run(cmd, check=True, **kwargs) | ||
|
|
||
|
|
||
| def sha512_of(path: Path) -> str: |
| xz = subprocess.Popen( | ||
| ["xz", "-T1", "-9e"], stdin=tar.stdout, stdout=out | ||
| ) | ||
| tar.stdout.close() # allow tar to receive SIGPIPE if xz exits |
Remove three encrypted extract-test fixtures from gnome-autoar and drop the three meson test cases that read them.
gnome-autoar ships three encrypted extract-test fixtures (
tests/files/extract/test-encrypted*/input/arextract.zip, password-protected zips). The package-signing scan can't inspect encrypted archives and rejects the .src.rpm, blocking signing. The files/cases are test-only and not shipped in any binary RPM.%checkruns%meson_test, so removing the fixtures also drops the three meson cases that read them (test_encrypted,test_encrypted_request_passphrase,test_encrypted_wrong_passphraseand their registrations).Why not an azldev archive overlay (as used for the other packages)? The tarball also ships an absolute-target symlink fixture (
tests/files/extract/test-symlink-parent/reference/arextract -> /tmp) that azldev's overlay extractor rejects while extracting the whole archive — before overlays apply. Instead,modify_source.shrepacks the tarball out-of-band and it's served viaorigin=download.azldev comp render --check-onlyreports no drift.