Only re-download and re-extract archive inputs when they changed - #604
Draft
leoschwarz wants to merge 2 commits into
Draft
Only re-download and re-extract archive inputs when they changed#604leoschwarz wants to merge 2 commits into
leoschwarz wants to merge 2 commits into
Conversation
Preparing a `bfabric_resource_archive` input re-downloaded the archive and re-extracted every entry on every run. The "for caching" comment beside the kept `<name>.zip` was never true: `fetch_to_path` always transfers into a fresh `<dest>.part` sibling and then renames it, so rsync never has a local basis file. Reuse the kept archive when its md5 matches the resource checksum, and extract only the entries whose local copy is missing or differs. Zip stores a CRC-32/ISO-HDLC per entry, which `zlib.crc32` computes directly, so a modified extracted file is still repaired even when the archive itself is unchanged -- the case that makes a naive "archive unchanged, skip everything" wrong. The size check runs first, so same-length files are the only ones read. `_download_file` passed `checksum=None`, which made the spec's `check_checksum` a no-op in the prepare path; it now forwards the checksum so the archive is verified as documented. `inputs check` gains the matching per-entry verification via the shared `_planned_entries`, so it cannot drift from what a prepare would do, and it now derives the cached archive from the last path component -- it looked for `input/input/name.zip` when the input filename contained a subdirectory (#323). Closes #458
…tory `_clean_input_files` called `path.unlink()` for every resolved input, so for an archive input -- whose path is the extracted *directory* -- `inputs clean` aborted (EISDIR on Linux, EPERM on macOS) and removed nothing. It also left the cached archive behind. That was harmless while every prepare re-downloaded, but now that a prepare reuses the archive it matters: a clean followed by a prepare would have rebuilt from the stale copy. Both the extracted tree and the cached archive are now removed, via the `archive_cache_path` helper the prepare and integrity paths share. Guard the removal against a path that is not strictly inside the working directory: a `filename` of "." resolves to the working directory itself and one containing ".." can point outside it, and recursing there would delete the whole chunk folder (`inputs.yml` included) or a sibling. The check is lexical rather than via `resolve()`, so a symlinked input (`link: true`) still gets cleaned.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
bfabric_resource_archiveinput to reuse the already downloaded archive when its checksum still matches, and to extract only the entries whose local copy is missing or differs (size, then CRC32) — a repeated prepare now does no transfer and no extraction when nothing changed, while a modified or deleted extracted file is still repaired.inputs checkto verify an archive input's extracted files too, not just the downloaded archive.BfabricResourceArchiveSpec.check_checksumto take effect: the downloaded archive is verified against the resource checksum instead of being accepted unchecked.inputs cleanto remove an archive input's extracted directory and its cached archive, instead of aborting on the directory and removing nothing.inputs checkto locate the cached archive correctly when an input filename contains a subdirectory (it looked forinput/input/name.zip).Closes #458
🤖 Prepared with assistance from Claude Opus 5 via Claude Code.