Repository navigation
test(archive-props): keep shifted symlinks pointing where they did - #660
Conversation
`entries_without_top_level_symlink` moves every relative ComfyUI entry under `a/`, but left the generator's relative link targets as they were, so `../outside` written one level deeper resolved to `dest/outside` and no longer escaped. A moved symlink's relative target now gains a `../`, keeping the "link, then write through it" escape the property is meant to exercise, and a unit test checks the resolved target is unchanged by the move. Also share one `ESCAPES_ONLY` between the uv and ComfyUI properties, and say on `CaseOutcome::tree` that its `shared_inodes` is always empty because `run_case` reports those as escapes. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
jussielo-amd
left a comment
jussielo-amd
left a comment
There was a problem hiding this comment.
Reviewed. No correctness bugs found — I traced moved_one_level_down's relative-symlink-target compensation against lexical path-resolution semantics and cross-checked it against the tar crate's actual resolution behavior (symlink targets resolve relative to the link's own directory; hardlink targets resolve relative to the destination root, so correctly leaving them untouched is also correct). Moving an entry under a/ increases its containing directory's depth by exactly one regardless of original nesting, so the single ../ compensation holds in all cases.
Ran the full test suite locally, including the new moving_an_entry_down_keeps_its_symlink_target_in_place test and the entries_without_top_level_symlink-driven property test (512 cases, hardlink shapes reached ~11% of the time) — all passing.
Two minor, non-blocking cleanup notes left as inline comments below.
| let moved: Vec<Entry> = entries.into_iter().map(moved_one_level_down).collect(); | ||
| for entry in &moved { | ||
| if matches!(entry.kind, Kind::Symlink(_)) | ||
| && !entry.name.starts_with(b"/") |
There was a problem hiding this comment.
Non-blocking: this absolute/re-rooted guard (!name.starts_with(b"/") && !name.starts_with(b"@")) is now duplicated between moved_one_level_down (around line 3142) and this symlink-filter loop. If the re-rooting convention ever changes (e.g. a new sentinel prefix), it'd be easy to update one copy and miss the other. Might be worth factoring into a shared helper.
There was a problem hiding this comment.
Agreed. The absolute/re-rooted guard should live in one place. I will move it into a shared helper in a follow-up PR rather than re-spin this approved one.
| b"a", | ||
| ] { | ||
| let moved = moved_one_level_down(Entry { | ||
| name: b"l".to_vec(), |
There was a problem hiding this comment.
Non-blocking: this test only exercises a top-level link name (l -> a/l), never one already nested in a subdirectory (e.g. sub/l). So it can't by itself prove the doc comment's "exactly one ../, regardless of original nesting depth" claim — only the probabilistic property test would catch a depth-dependent regression here, and only non-deterministically. A second case with a pre-nested name would pin that down.
There was a problem hiding this comment.
Fair. Only the property test reaches a nested link today. The follow-up will add a direct sub/l case, so the "exactly one ../ whatever the depth" claim has a deterministic test.
juhovainio
left a comment
juhovainio
left a comment
There was a problem hiding this comment.
Reviewed this (test-only follow-up to #559) with a full fan-out correctness pass plus a separate Standards/Spec pass. No blocking findings on either.
Independently traced the ../-compensation math across all 6 relative-target cases and confirmed it correctly restores the original resolved location in every one. Cross-checked impact across the other crates that use archive_props.rs (uv.rs, therock.rs, lemonade's install.rs) and confirmed none of them share this shifted-symlink pattern, so this is correctly scoped to comfyui.rs alone. The ESCAPES_ONLY dedup is clean. Also checked this against #559's own review thread: 3 of the 4 follow-up items are fully implemented here, and the 4th (reusing running_as_root()) is explicitly declined with cross-crate #[path]-inclusion reasoning that I verified is actually correct by grep, not a shortcut.
One non-blocking tightening worth doing at some point, not blocking this PR: moving_an_entry_down_keeps_its_symlink_target_in_place only pins one branch of each of the two absolute-name/absolute-target guard conditions explicitly. The missing branches are still exercised probabilistically by the existing 512-case property sweeps, so actual regression risk is low, but two more one-line assertions would make the dedicated unit test deterministic instead of relying on that backup coverage.
Approving - nothing here blocks merge.
|
Thanks. Agreed: the follow-up already promised for jussielo's notes (sharing the guard, adding a nested |
|
Follow-up is in #684, with a correction. The nested |
Follows up #559 (jussielo-amd's notes).
a/, used to pull their relative symlink targets one level back inside the install. A newmoved_one_level_downhelper adds a compensating../, so../outsidestill escapes.moving_an_entry_down_keeps_its_symlink_target_in_placechecks that the resolved target is the same before and after the move, and fails without the fix.ESCAPES_ONLY: now defined once, inarchive_props.rs.CaseOutcome.treedoc: now saysshared_inodesis always empty there, because those entries are moved intoescapes.running_as_root()not reused:archive_props.rsis#[path]-included into three crates, andcrate::openmpidoesn't resolve outside rocm-core. The umask still has to be read from/procanyway.Verified locally:
rocm --bin rocm(1096),rocm-core(622) ande2e-cucumberlib, plusfeature_namingx86_64-pc-windows-gnuThis is test-only, so no scenario.