Repository navigation
xtask: package the binaries in a fixed order - #436
Open
rudycelekli wants to merge 1 commit into
Open
rudycelekli wants to merge 1 commit into
rudycelekli wants to merge 1 commit into
Conversation
`package` appended `bin/*` in `read_dir` order, which is whatever order the filesystem keeps (on ext4 that is hash order with a per-filesystem seed). The same binaries packed on two machines could therefore produce two archives with two sha256s. That breaks the promise in updater-design.md 搂16 that fixed mtimes make the same inputs produce the same archive, "so a rebuild can be compared against what shipped". The names were already sorted for version.toml. Now the archive order is sorted too. The test checks the entry order directly. Packing twice on one filesystem usually enumerates both directories alike, so a byte comparison alone would pass on the bug. Assisted-by: Claude:claude-opus-5-5
This branch has not been deployed
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
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.
What's wrong
updater-design.md搂16 says:xtask packagefixes the mtime, but it appendsbin/*instd::fs::read_dirorder, and that order belongs to the filesystem. On ext4 (the CI runners) it is htree hash order, seeded per filesystem, so the same staged binaries packed on two runners can produce two archives with two different sha256s. The name list written intoversion.tomlwas already sorted. The archive order was not. Also, no test inxtask/testspacks the same inputs twice, so the test the design doc mentions does not exist.Here is what packing eight stub binaries on
mainproduced on APFS:The fix
packagecollects thebin_dirpaths, sorts them, and then appends them.shippedis built in that order, so the separateshipped.sort()is no longer needed.--includeentries keep their command-line order, which the workflow already fixes.Test
binaries_are_packaged_in_a_fixed_order(inxtask/tests/artifact.rs) runs the realxtask packageon eight stub binaries. It checks that thebin/entries in the archive are in sorted order, and that packing the same files created in reverse order gives byte-identical archives. The order assertion is the one that catches the bug. Two directories on one filesystem usually enumerate the same way, so the byte comparison alone would pass onmain. The test comment says this.main: fails with the order shown above.xtask/src/main.rsfromorigin/mainand the named test failed. Then I put the fix back.Checks (macOS, aarch64)
cargo test -p xtask: 68 passed, 0 failed (41 + 6 + 18 + 3)cargo clippy -p xtask --all-targets -- -D warnings: cleancargo fmt -p xtask --check: clean馃 Generated with Claude Code