Repository navigation
fix(uninstall): undo the installer's PATH change and remove the running exe - #636
Conversation
juhovainio
left a comment
juhovainio
left a comment
There was a problem hiding this comment.
Reviewed this (diff-only, didn't run the e2e suite myself) — no blocking issues, it does what it says: undoes the installer's PATH edit and removes the running exe on uninstall. Left a handful of inline notes, mostly about edge-case consistency rather than correctness:
- A couple of spots where a failure partway through
execute_planbehaves differently depending on which step hit it — worth making consistent. install.ps1's PATH-record write and the record read inuninstall.rshave narrower safety margins than their Unix/byte-level counterparts elsewhere in this same change.- Some test duplication in
lifecycle_steps.rsthat could use a shared helper. - A possible Windows CI flakiness risk around the registry-mutating scenarios (not new, but this PR adds a second one).
- A duplicate atomic-write helper and a small messaging gap for
--keep-binaries, both just worth being aware of.
Also worth a quick look: the PR description attributes this to #616, but that issue doesn't actually describe the PATH-record/undo mechanism or exe removal — might be worth tightening that reference.
On the positive side: the byte-level profile editing (handles non-UTF-8 and CRLF), the symlinked-$HOME handling, and the then_xdg_gone fix (the old assertion was checking a path that never existed, so it always passed trivially — the new one actually tests something) all stood out as solid, deliberate work. Docs look fully in sync with the implementation.
None of this blocks merging as far as I can tell from the diff — just flagging for your call.
| )), | ||
| Ok(false) => {} | ||
| Err(error) => { | ||
| unreadable = true; |
There was a problem hiding this comment.
If a Unix profile candidate fails to read here (e.g. permission denied), unreadable gets set and a warning is queued, but the function just keeps going: it doesn't add this profile to edits, and leaves plan.path_record as None. A few lines up, execute_plan's own undo loop treats a comparable failure very differently — it bails out and keeps the binaries around for a retry. Here, the uninstall proceeds anyway, the binaries get removed, and the stale .rocm-cli-path-edit record is left on disk with nothing left around to ever retry it (no rocm binary remains to rerun rocm uninstall). Doesn't corrupt anything and a warning is printed, but it's inconsistent with the "a failure to undo stops uninstall" guarantee the module doc describes, and permission-denied profiles are realistic enough to be worth handling the same way as the execute-time case.
There was a problem hiding this comment.
Agreed. If the installer recorded writing to a profile, uninstall now plans to undo it even when the profile cannot be read. The review warns, and uninstall stops before removing anything. A profile that is only scanned for, not recorded, still just warns. Tests: a_recorded_profile_that_cannot_be_read_stops_before_anything_goes and an_unrecorded_profile_that_cannot_be_read_is_a_warning_not_an_absence.
| } | ||
|
|
||
| if let Some(exe) = &plan.self_delete { | ||
| lines.push(delete_running_executable(exe)?); |
There was a problem hiding this comment.
This is a bare ? — if delete_running_executable fails, every "removed X" line collected earlier in this same call is dropped instead of printed. Two branches above, the PATH-undo failure path explicitly reprints lines before bailing for exactly this reason. Low real-world frequency since self-replace is normally reliable, but worth the same treatment for consistency: print what was collected so far before returning the error.
There was a problem hiding this comment.
Fixed for every failure path: each line is now printed as its step finishes, so no failure can drop them (6670305f). Test: a_failure_after_the_path_undo_still_reports_what_was_done.
| # user PATH stays the user's. | ||
| $recordPath = Join-Path $Path ".rocm-cli-path-edit" | ||
| $utf8NoBom = New-Object System.Text.UTF8Encoding $false | ||
| [System.IO.File]::WriteAllText($recordPath, "user-path $Path`n", $utf8NoBom) |
There was a problem hiding this comment.
This overwrites .rocm-cli-path-edit with WriteAllText (truncate), whereas install.sh's record_path_edit appends with a dedup guard. A crash between the registry write above and this write leaves PATH modified with no record — though that degrades to the same "pre-change install, no record" case the PR's own "Known limits" section already discloses, so it's a narrow timing window reaching an already-acknowledged state rather than a new failure mode. Flagging for awareness; not blocking.
There was a problem hiding this comment.
Reordered: the record is now written before SetEnvironmentVariable. An interrupted install now leaves at worst a record of an entry that is not there. Uninstall reports that entry as no longer on the user PATH.
| home: Option<&Path>, | ||
| ) { | ||
| let record_path = bin_dir.join(PATH_EDIT_RECORD); | ||
| let record = match fs::read_to_string(&record_path) { |
There was a problem hiding this comment.
fs::read_to_string here will fail on a non-UTF-8 byte in the record (possible if $HOME contains non-UTF-8 bytes — install.sh writes it unescaped), silently treating the whole record as unreadable and leaving that install's PATH edit unaddressed. The profile-editing path elsewhere in this file (remove_profile_block_bytes) is deliberately byte-level for exactly this reason; this read isn't. Fails safe, narrow precondition, but worth the same tolerance for consistency.
There was a problem hiding this comment.
Fixed. The record is parsed as bytes, and its targets are kept as OsString. The folder spellings derived from it are compared as bytes too, so a non-UTF-8 HOME or install folder also matches. Tests: a_record_naming_a_profile_that_is_not_utf8_is_still_followed and an_install_folder_whose_path_is_not_utf8_is_still_found.
| } | ||
|
|
||
| #[when("the user uninstalls from the installed binary keeping the binaries")] | ||
| async fn when_uninstall_keep_binaries(world: &mut E2eWorld) { |
There was a problem hiding this comment.
when_uninstall, when_uninstall_keep, and this new when_uninstall_keep_binaries are three near-identical Command::new(&rocm)... blocks differing only in the args slice. A shared run_uninstall(world, extra_args) helper would cut roughly 45 duplicated lines. Separately, then_profile_kept (line 1286) duplicates then_single_path_marker's exact marker-count check under a different name. Cosmetic, not a correctness issue — just flagging for a cleanup pass.
There was a problem hiding this comment.
Done: a shared run_uninstall(world, extra_args) helper, and both marker steps now use assert_single_path_marker.
| And the record of the installer's PATH change is still in the install directory | ||
|
|
||
| @id:lifecycle-windows-uninstall-restores-user-path @lifecycle @requires-os:windows | ||
| Scenario: lifecycle-24 - Windows - uninstall takes the install directory back off the user PATH |
There was a problem hiding this comment.
This scenario and the pre-existing lifecycle-13 both capture/mutate/restore the single real HKCU\Environment\Path value on the Windows runner, with no serialization guard visible here. max_concurrent_scenarios defaults to 64 unless the lane has a GPU or shared-cache dirs configured. This PR doubles the collision surface on what looks like an existing (not new) hazard — couldn't confirm from here whether the actual Windows CI lane forces these serial. Worth checking if this (or lifecycle-13) ever flakes.
There was a problem hiding this comment.
Confirmed: the hosted Windows lane runs up to 64 scenarios at once. lifecycle-13 and lifecycle-24 are now @serial. Capturing and restoring the Path also keeps its registry type now; before, an expandable Path was flattened to REG_SZ.
|
|
||
| /// Replace `path`'s contents in one step, keeping its permissions, so an | ||
| /// interrupted write never leaves a truncated shell profile behind. | ||
| fn write_replacing(path: &Path, contents: &[u8]) -> Result<()> { |
There was a problem hiding this comment.
This is a second, independent atomic-replace-file implementation (temp-file + fs::rename, preserving source permissions) alongside the existing write_file_atomically family in therock.rs, which a prior commit (911a399) introduced specifically to consolidate atomic-write logic across the codebase. It does have a real justification the existing helpers lack — preserving the destination file's permission bits matters when rewriting a live shell profile, and the existing helpers don't do that. Just flagging so the duplication is a known, deliberate tradeoff rather than something that drifts further next time someone needs atomic writes.
There was a problem hiding this comment.
Keeping it separate on purpose. It must keep the profile's permissions, edit through a symlinked profile, and now create the new file with its final mode. write_file_atomically does none of these, and neither does #610's shared version. If #610 lands with a keep-permissions option, I will switch to it.
| #[arg(long)] | ||
| dry_run: bool, | ||
| /// Keep installed rocm-cli binaries. | ||
| /// Keep installed rocm-cli binaries, and the installer's PATH change. |
There was a problem hiding this comment.
--keep-binaries keeps the PATH record too (confirmed by the unit test that plan_path_edits is simply never invoked under this flag), but unlike the shared-install-folder case — which prints an explicit "PATH change left in place: ..." message — there's no equivalent message here telling the user the PATH change was left alone. Purely a messaging completeness gap, not a behavior bug.
There was a problem hiding this comment.
Fixed. The note now reads "binary removal disabled by --keep-binaries, so any PATH change the installer made stays too". The unit test and lifecycle-25 assert it together with the state that is kept.
jussielo-amd
left a comment
jussielo-amd
left a comment
There was a problem hiding this comment.
Reviewed the diff against origin/main (three-dot merge-base) on branch pr-636-review. No prior reviews or review comments existed on this PR before this one.
This is a careful, well-scoped fix. rocm uninstall previously left two things behind: the running rocm.exe on Windows (unlinkable while running) and the installer's PATH edit (shell-profile block on Linux, user PATH registry entry on Windows). The PR adds crates/rocm-core/src/user_path.rs for the Windows HKCU\Environment Path manipulation (preserves REG_SZ/REG_EXPAND_SZ type, broadcasts WM_SETTINGCHANGE) and substantially extends apps/rocm/src/uninstall.rs to record and undo the PATH edit, with self-replace handling the Windows self-delete.
What I checked:
- Full diff read through
user_path.rs,uninstall.rs(~960 lines), themain.rsplan-building changes,install.sh/install.ps1record-writing, and the Gherkin/step-definition additions intests/e2e-cucumber. - Built and ran the new unit tests locally: all 5
rocm-coreuser_pathtests and all 23apps/rocmuninstall::testspass (cargo test -p rocm-core user_path,cargo test -p rocm --bin rocm uninstall::). cargo clippy -p rocm -p rocm-core --all-targets -- -D warningsis clean.cargo xtask check-crate-edgespasses (no new first-party edge introduced;self-replaceis a Windows-only third-party dep, properly added toMANIFEST.md/THIRD_PARTY_NOTICES.txt/Cargo.lock).- Ordering/safety logic verified by reading and by test: PATH edits are undone before binaries are removed, and a failed PATH undo leaves the binaries and the
.rocm-cli-path-editrecord in place for a retry (apps/rocm/src/uninstall.rsexecute_plan), exercised bya_path_change_that_cannot_be_undone_stops_before_the_binaries_goand the matchinglifecycle-linux-uninstall-path-failure-keeps-binariese2e scenario. - Verified a user's own PATH line is never touched (only the installer's marked block, matched byte-for-byte with quote-boundary checks in
remove_profile_block_bytes) and that a profile reached via a symlink is edited at its real target while the link itself is preserved (a_linked_profile_is_edited_where_it_lives_and_stays_a_link). - Docs (
README.md,docs/manual-testing.md,docs/testing.md,docs/architecture.md,docs/engine-plugins.md) were updated in the same change to match the new behavior, including correctly reclassifyinguninstall.rs's extraction pattern now that it ownsPathEdit. - BDD coverage for the new user-observable behavior is thorough:
@id:lifecycle-linux-uninstall-full-purge(updated), plus four new scenarios (lifecycle-23throughlifecycle-26) covering shared-folder PATH retention, Windows user-PATH removal,--keep-binariesretention, and the undo-failure/retry path. The pre-existing XDG-purge check bug (asserting arocm-clisubdirectory that never existed) is also fixed as called out in the commit message. - Leak scan across the diff is clean (the only hits were the word "private" in
docs/architecture.mddescribing Rust visibility, not an actual leak).
I didn't find any blocking issues. A couple of very minor, non-blocking observations for awareness, not requesting changes: other_entries in uninstall.rs treats any non-rocm-cli file in the install folder as "another program" sharing it, which is a conservative/safe default but could occasionally over-trigger on stray files; and the Windows "on PATH but not recorded" informational message reads the current process's PATH env var rather than the persisted registry value, which only affects that FYI wording, not the actual undo logic.
Approving.
jussielo-amd
left a comment
jussielo-amd
left a comment
There was a problem hiding this comment.
Follow-up to my earlier approval on this PR: a second, independent pass surfaced issues I missed the first time, and after re-verifying each directly against the code (and reproducing two of them), I'm retracting the approval. Sorry for the churn — the latest review should be treated as the current state.
Two confirmed blockers, both in newly-added code in apps/rocm/src/uninstall.rs:
-
Lost progress output on any failure other than the PATH-edit-undo one.
execute_plan(lines 94-133) accumulates every "removed ..." line into a localVec<String>and only returns it to the caller on full success;uninstall()then doesfor line in execute_plan(&plan)? { println!("{line}"); }. Only the PATH-edit failure arm (lines 103-118) explicitly reprints the accumulatedlinesbefore bailing. Every other failure path — the non-binary removal loop (97-101), the binary removal loop (120-124), install-record removal (125-128), and the Windows self-delete (delete_running_executable(exe)?at 130-131) — propagates its error straight through?, which drops theVec<String>(and therefore every already-reported-success line, including a successful PATH undo) with nothing printed. I reproduced this: I added a temporary test that plans a real shell-profile PATH edit (which succeeds, restoring the profile) followed by a binary removal that's forced to fail (read-only bin directory), and confirmed the returned error contains no mention of the PATH-lines removal that had, in fact, already happened — there is no way for the caller to learn about it. Pre-PR, each line wasprintln!'d immediately as each item was removed, so this is a real regression, not pre-existing behavior. -
The "binaries were left in place" message doesn't account for already-removed state, and no scenario checks what actually happens to it. The non-binary actions (config/data/cache, lines 97-101) run before the PATH-edit-undo is attempted (103-118). On a plain
rocm uninstall --yeswith no--keep-*flags, if the PATH edit then fails, config/data/cache have already been permanently deleted, but the bail message only says "The ROCm CLI binaries were left in place." The newlifecycle-26scenario (tests/e2e-cucumber/features/install_lifecycle.feature) exercising this exact failure path checks that the binaries, profile, and record are preserved, but never asserts what happened to the isolated config/data/cache state, so this interaction is untested either way.
A security-relevant finding I independently reproduced, also in new code: write_replacing (apps/rocm/src/uninstall.rs lines 478-492), used to rewrite a shell profile after stripping the rocm-cli block, creates the replacement at default/umask permissions via fs::write and only applies the original file's mode afterward via fs::set_permissions. I verified with a standalone repro that a 0600 original file is briefly written back as 0644 before the mode fix lands — a narrow but real window where a profile holding a secret (e.g. an exported API key) is world/group-readable to another local user on a shared box.
A few lower-severity, non-blocking items worth tightening in the same area, for awareness rather than as hard blockers: other_entries (used to decide whether another program shares the install folder) re-derives "is this ours" from a hardcoded filename-prefix heuristic (is_rocm_install_entry_name) instead of consulting the authoritative .rocm-cli-manifest that install.sh/install.ps1 already write with the literal list of installed paths — a future binary name that doesn't fit the heuristic, or a third-party file that happens to, is misclassified either way; the "is this the running exe" check in main.rs (~17866, *path == current_exe) is a strict PathBuf equality, while this PR's own e2e step then_running_rocm_gone (tests/e2e-cucumber/tests/e2e/lifecycle_steps.rs) deliberately avoids that same comparison and matches by filename suffix instead, with a comment citing Windows long-vs-8.3-short-path spelling risk — the production check doesn't share that hedge, and a miss there would hit the output-loss bug above; and user_path::remove_user_path_entry's registry read-modify-write has no compare-and-swap (though this mirrors the same unguarded pattern install.ps1's own Add-InstallDirToUserPath already uses for writing the entry, so it's not a new category of risk this PR introduces).
Everything from my first review (test runs, clippy, crate-edges check, doc sync, leak scan) still stands — those were accurate, this is additive.
siloteemu
left a comment
siloteemu
left a comment
There was a problem hiding this comment.
🔴 Automated review · pr-review-watcher · 68dd067
This automation never files a GitHub approval, so no approving review will
appear here whatever the outcome — the merge decision stays with a human
reviewer.
Review — needs work
Full review of the whole change.
Implements what its description claims; it names #616, an open docs PR, as the intended behaviour, and leaves stopping background processes to #299. No ticket named.
Blocking
- new · A failure after the PATH undo no longer reports what was already changed —
apps/rocm/src/uninstall.rs:82-84,:94-134- On
prw-base,uninstall()printedremoved …after eachremove_path. Nowexecute_plansaves every line and prints them only if the whole plan succeeds. The one exception is the PATH-undo error branch (:107-109), which prints them before failing. - The
?on the config/data/cache loop (:98), the binary loop (:121), the record removal (:126) anddelete_running_executable(:131) all throw the saved lines away. - So when a binary removal fails (for example a running
rocmd.exeon Windows), the user sees onlyfailed to remove …. They are not told that their shell profile or user PATH was already edited, or that config, data and cache were already deleted. - Nothing tests a failure after the PATH step.
- Confidence 85 · logic · Fix: print each line as it is produced, as the base code did, or print the saved lines on every error exit.
- On
- new ·
keeping_the_binaries_keeps_the_path_changecannot fail on--keep-binaries—apps/rocm/src/uninstall.rs:686-704,apps/rocm/src/main.rs:17851- I replaced
if options.keep_binaries {withif false {inbuild_uninstall_plan, and the test still passed. - The reason: the test binary sits in a cargo
target/layout, sois_dev_binary_layoutskips binary and PATH planning however the flag is set. The record and profile checks see nothing either way. - The PR description lists
--keep-binariesunder "Unit tests cover", which this test does not. - lifecycle-25 does guard the flag, on the Linux
@lifecyclelane. - Confidence 100 · logic · Fix: test
--keep-binariesagainst a fixture install folder that holds a record and a marked profile (for example by factoring the gate so it takesbin_dir). Otherwise delete the test and its line in the description, and rely on lifecycle-25.
- I replaced
- new · No test depends on what
install.shwrites into the record —install.sh(therecord_path_editcalls afterappend_path_snippet),tests/e2e-cucumber/tests/e2e/lifecycle_steps.rs:390,:449,apps/rocm/src/uninstall.rs:231-251- The Linux lifecycle scenarios always write the block to
$HOME/.bashrc, and the install folder is<root>/install/bin, which is outsideHOME. - Uninstall always checks
.bashrcas a known profile, and always works out the literal path spelling itself. So neither theprofileline nor thepath-exprline frominstall.shis ever needed. Scenarios 09, 23, 25 and 26 only check that the file exists. - If either key in
install.shwere misspelled or dropped, every scenario would stay green. Two user-visible cases have no scenario, as AGENTS.md §3 requires:- a profile picked through
ROCM_CLI_SHELL_PROFILE, which only the record can find; - a block spelled
$HOME/…that uninstall cannot work out for itself.
- a profile picked through
- The unit tests write records by hand, so they cover the parsing side only.
- Confidence 85 · logic · Fix: add a lifecycle scenario that installs with
ROCM_CLI_SHELL_PROFILEpointing at a profile outside the known list, then uninstalls without that variable and checks the profile is restored.
- The Linux lifecycle scenarios always write the block to
- new · lifecycle-24 would pass even if the rest of the user PATH were destroyed —
tests/e2e-cucumber/tests/e2e/lifecycle_steps.rs:1322-1342,crates/rocm-core/src/user_path.rs:40-171- The step checks only two things: the install folder is no longer in the user
Path, and the output saysfrom your user PATH. - It would stay green if
remove_user_path_entryemptied the value, dropped other entries, or rewroteREG_EXPAND_SZasREG_SZ. The code promises to preserve all three ("keeps the value's registry type"), and the PR description says so too. - The registry function has no other test. The
#[cfg(test)]tests inuser_path.rscover only the string helper, and the Windows branch ofplan_path_editsnever runs on Linux. - Confidence 85 · logic · Fix: before the install, put an extra entry (one with
%VAR%) on the userPath. After uninstall, assert that it is still there, and that the value type is unchanged.
- The step checks only two things: the install folder is no longer in the user
- new · The branch conflicts with the base —
crates/rocm-core/Cargo.toml,docs/architecture.mdgit merge-tree prw-base HEADreports content conflicts in both files. The base is 19 commits ahead: it added a differentwindows-sysfeature and rewrote the architecture paragraphs this change edits.- After rebasing:
- re-add
Win32_UI_WindowsAndMessaging; - regenerate the lock, manifest and notices;
- re-check how the base's architecture text describes
uninstall.rs, since this change adds an owned type (PathEdit).
- re-add
- Confidence 100 · mechanical · Fix: rebase onto the current base and resolve.
Non-blocking
- new · The new section header names something no scenario covers —
tests/e2e-cucumber/features/install_lifecycle.feature:279# ── Uninstall: PATH changes and background processes ──sits over four scenarios, and none of them concerns background processes.- Confidence 85 · mechanical · Fix: drop "and background processes".
- new ·
other_entrieserrs toward removing the PATH change —apps/rocm/src/uninstall.rs:416-428- Three cases are treated as "no other programs here", so the PATH change is undone under a program that may still need it:
read_dirfails (it returns an empty list);- an entry cannot be read (
.flatten()drops it); - a file name is not UTF-8 (
filter_map(to_str)drops it).
- The same function handles unreadable profiles and the record file the other way, keeping the change.
- Confidence 70 · logic · Fix: use
to_string_lossy, and on a read error keep the change and add a warning.
- Three cases are treated as "no other programs here", so the PATH change is undone under a program that may still need it:
- new · The
install.shcomment says more than its check does —install.sh("Only the installer's own block is recorded")- The guard is a grep for the start marker, which also matches a block written for a different install folder.
append_path_snippetalready reportsunchangedin that case, so the record can name a profile andpath-exprthat are not this install's. Uninstall copes: it reports "no longer has the rocm-cli PATH lines".- Confidence 70 · mechanical · Fix: match the block for this folder's spelling, or reword the comment.
Decisions for the author
- Order of removal — tradeoff
- Config, data and cache are deleted before the PATH undo. When the undo fails, the error message and the README mention only that the binaries were kept.
- On Windows, a binary removal that fails (for example a running
rocmd.exe) now comes after the user PATH entry is already gone.rocm.exeis then left off PATH. A retry still works through the full path, because the record outlives the failure. - Undoing PATH first and deleting state last would make every failure fully retryable, but would delete state later.
- The record stays in a shared folder that has no uninstaller left — tradeoff
- The comment at
uninstall.rs:184-185says the record stays "so a later uninstall still knows". But the same run removesrocmandrocmd, so the record is only read again after a reinstall. Until then it is a stray file in the user's~/.local/bin. - The alternative is to remove it and rely on the scan of known profiles, which on Linux does not need the record.
- The comment at
- Windows upgrades from installs that predate this change — tradeoff
install.ps1records the entry only when it adds it. Rerunning the installer to upgrade an older install, as the README suggests, never records the entry, so uninstall never removes it. On Linux, the marked block is found anyway.- The PR states this limit. Confirm it is acceptable for the most common existing Windows users.
Positive signals
- Shell profiles are edited as bytes, with an atomic replace that keeps permissions, and a symlinked profile is edited where it points. Non-UTF-8 and dotfile-managed profiles therefore keep everything outside the block.
- The fixed XDG purge step replaces a check that could never fail: on
prw-base, it tested arocm-clisubdirectory that was never created.
Deployment notes
- Adds a Windows-only dependency,
self-replace1.5 (Apache-2.0), and theWin32_UI_WindowsAndMessagingfeature ofwindows-sysinrocm-core. - Both installers now write
.rocm-cli-path-editinto the install folder. Older installers do not write it.
What this covered
Read every changed file in full, together with the install.sh and install.ps1 PATH functions, build_uninstall_plan and its helpers, storage.rs's use of UninstallPlan, and the CI lanes that run the lifecycle features. Range: merge base 0d85577…68dd067e, compared with prw-base 8e2e890.
What ran:
- The
rocmuninstall unit tests (40 passed). - One mutation of the
--keep-binariesgate. - A read-only
git merge-treeagainst the base. - CI on 68dd067: every check passed or was skipped.
Not run:
- The Windows code paths (registry edit, self-delete) and the lifecycle e2e suite, which needs packaging and a Windows host. Those findings come from reading the code.
cargo xtask manifest --checkandtpn --check.
The design review (Step 7) was done by the driver from the workers' investigation, not by separate design workers. The four review passes, history, prior changes on earlier PRs, code comments, and the instruction file (AGENTS.md) all ran as independent workers.
This review was not reconciled against any discussion on the PR.
…ng exe `rocm uninstall` left two things of ROCm CLI behind: - On Windows it skipped the running rocm.exe, so the program that ran uninstall stayed in the install folder. - On every platform the installer's PATH change survived: the marked block install.sh appends to a shell profile, and the user Path entry install.ps1 adds. The installers now record the PATH change they made in .rocm-cli-path-edit, and uninstall undoes it: the marked profile block is removed (the profile reads back as it did before the install, and a profile kept as a link stays a link), or the entry is taken off the HKCU user Path with its registry type kept. A PATH line the user wrote is never touched, and the change stays when other programs share the install folder or with --keep-binaries. Marked blocks from installs that predate the record, or from an earlier install under another shell, are found and removed too. The PATH change is undone before the binaries go, so a failure leaves rocm and the record in place to retry. The running rocm.exe is deleted with self-replace, which moves it out of the install folder and deletes it once the process exits. Lifecycle scenarios cover the profile restore, the shared folder, --keep-binaries, the failed-undo retry, the Windows user Path, and the running rocm.exe. The XDG purge check now asserts the directories themselves are gone; it used to check a subdirectory that never existed. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
… runs Review follow-up. Order and reporting: - The installer's PATH change is now undone before anything is removed, so a change that cannot be undone leaves the config, data, cache, and binaries all in place, and the message says nothing else was removed. - Each step is printed as it finishes. A later failure (a binary Windows will not unlink, say) no longer hides the PATH undo and the removals that already happened. - A profile the installer recorded writing to that cannot be read is now planned as an undo, so the uninstall stops there before removing anything, instead of removing the binaries and stranding the record. Correctness: - The replacement profile is created with the original's permissions, never wider, instead of at the umask default and narrowed afterwards. A replacement left by an interrupted run no longer blocks the next. - The record and the folder spellings are read as bytes, so a home or profile path that is not UTF-8 is still matched. - Another program in the install folder is detected even when its name is not UTF-8, and a folder that cannot be listed keeps the PATH change. - The running rocm.exe is matched case-insensitively. - --keep-binaries now says the PATH change stays with the binaries, and a shared folder names the record it keeps. - install.sh records a profile only when it holds a closed marked block for this folder. install.ps1 writes its record before changing the user Path, so an interrupted install never leaves it unrecorded. Tests: - The --keep-binaries unit test now plans against an installed folder holding a record and a marked profile; it could not fail before. - lifecycle-24 adds an expandable %VARIABLE% entry to the user Path and checks uninstall keeps it and the REG_EXPAND_SZ type. The harness now captures and restores the user Path with its registry type, and the two scenarios that change it are @serial. - lifecycle-26 checks the isolated state survives the failed undo. - lifecycle-27 installs through a linked folder into a profile chosen with ROCM_CLI_SHELL_PROFILE, so only the installer's record can lead uninstall to the block. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
68dd067 to
6670305
Compare
|
Rebased onto main
@siloteemu (automated review)
@juhovainio: replied inline. The description no longer claims #616 describes this behaviour. Two smaller fixes came out of the Windows clippy run: Verified locally on the rebased head:
Deliberate type errors injected into the |
jussielo-amd
left a comment
jussielo-amd
left a comment
There was a problem hiding this comment.
Re-reviewed at 6670305f against the two confirmed blockers and the security finding from my prior review, plus the automated review's blockers.
Verified directly against the code (not just the reply text):
- Lost progress output:
execute_plannow takes areport: &mut dyn FnMut(String)callback and calls it per-step as each edit/removal completes, instead of buffering into aVecthat only got printed on full success. The PATH-undo loop runs first, before any removal. New testa_failure_after_the_path_undo_still_reports_what_was_doneforces a late binary-removal failure and asserts the PATH-undo and config-removal lines are still present in the output — confirmed this actually exercises the code path. - Permission window on profile rewrite:
create_replacementnow opens the temp file withcreate_newand sets the target mode directly viaOpenOptionsExt::mode, so the file never exists at umask-derived permissions before narrowing.the_replacement_profile_is_never_more_readable_than_the_originalpins this at the unit level, anda_replacement_left_by_an_interrupted_uninstall_does_not_block_the_nextcovers the stale-tempfile case. --keep-binariestest that couldn't fail: now callsplan_installed_binariesdirectly against a layout holding a real record and marked profile, rather than going throughis_dev_binary_layout's bypass. Asserts the flag both keeps the PATH edit/record and reports it in the skip note. Confirmed the test topology no longer depends on the dev-layout short-circuit the finding identified.- Branch conflicts with base: rebased onto
287e08a8;windows-sys's feature list carries both this PR'sWin32_UI_WindowsAndMessagingand whatever main had added, anddocs/architecture.mdreads clean. - Spot-checked the other siloteemu/juhovainio items addressed in the reply (case-insensitive running-exe match,
install.shrecord ordering beforeSetEnvironmentVariable, byte-level record parsing,other_entrieserring toward keeping the PATH change on an unreadable entry, lifecycle-27 and the@serialtags) — each matches what the reply describes.
The remaining items (manifest-vs-heuristic for other_entries, registry read-modify-write, the informational PATH note reading the process env) are reasonable trade-offs with stated rationale, not blockers.
CI is green across the board; only the MI300X hardware lane is still pending.
No further blocking issues from me. Approving.
#636 rewrote the e2e user-PATH restore to go through the registry directly so it keeps the value's REG_EXPAND_SZ/REG_SZ kind, while this branch had refactored the old `[Environment]::SetEnvironmentVariable` restore into a `write_user_path` helper. Kept #636's `restore_user_path` and `capture_raw_user_path` -- they are strictly more correct -- and kept `write_user_path` as a stage-only helper for `stage_user_path_for_test`, which deliberately wants the installer's own write path. Retargeted the staging capture at `capture_raw_user_path` for #636's new `Option<CapturedUserPath>` field type. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
Keep main's installer PATH undo and running-executable deletion (#636) together with the stop-before-remove gate: the stop pass and its gate run first, then execute_plan_with undoes the PATH changes and removes entries, reporting removed / failed and possibly partial / not attempted on a failure. Our lifecycle scenarios keep numbers 10-29; main's new ones become 30-34. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: fredespi <fredrik.espinoza@gmail.com>
4 participants
Summary
rocm uninstallleft two parts of ROCm CLI behind:rocm.exe, so the program that ran the uninstall stayed in the install folder.PATHchange survived. That is the marked blockinstall.shappends to a shell profile, or the userPathentryinstall.ps1adds.#616 (open) adds uninstall guidance to the README; this PR is the code change. It does not depend on #616.
Changes
<install dir>/.rocm-cli-path-edit:profileandpath-exprlines frominstall.sh, auser-pathline frominstall.ps1. Only a change the installer made itself is recorded, never aPATHline the user wrote.install.shadds lines instead of overwriting, so a reinstall from a different shell keeps the earlier entry.Pathentry fromHKCU\Environment, keeps the value'sREG_SZ/REG_EXPAND_SZtype, and broadcastsWM_SETTINGCHANGE.PATHchange is undone first, before anything is removed. If it can't be undone, or a profile the installer recorded can't be read, uninstall stops with nothing else removed, so you can fix it and retry. Each step is printed as it finishes, so a later failure still shows what was already done.install.shrecords a profile only when it holds a closed marked block for this folder.install.ps1writes its record before changing the userPath.--keep-binaries. The record stays too, so a later uninstall still knows about it. With--keep-binariesthe review says so; in a shared folder it names the record it keeps.rocm.exe. It is deleted withself-replace(Apache-2.0, Windows-only dependency). The crate moves the file out of the install folder and deletes it once the process exits.Stopping managed servers and the automation helper before removal is left to #299, which implements it. This PR doesn't depend on #299: Windows allows renaming a running executable, so the self-delete works without stopping those processes first.
Known limits
~/.local/binis often shared with other tools (uv, pipx, …). In that case thePATHchange is kept and the review names those tools. On many default Linux installs, uninstall will therefore report the change rather than undo it.Tests
Lifecycle scenarios in
install_lifecycle.feature:rocm.exethat ran uninstall is gone. Before this change it was skipped.PATH; another%VAR%entry and theREG_EXPAND_SZtype are kept.@serial, like lifecycle-13.--keep-binarieskeeps the change and says so. This only guards against a future regression: the old code never touchedPATH.PATHundo that fails removes nothing (binaries, record and isolated state all stay), and a retry completes.ROCM_CLI_SHELL_PROFILE, in an install folder reached through a link, is found and restored only through the installer's record.The XDG purge step used to check a subdirectory that never existed. It now seeds a file in each directory and asserts the directories are gone.
Unit tests cover:
--keep-binariesagainst an installed folder holding a record and a marked profile;Verification
Run locally on Linux (WSL2):
On the current head (rebased onto
287e08a8):cargo test -p rocm --bin rocm(1196 passed) and-p rocm-core --lib(666 passed).feature_naming;cargo check --test e2e;manifest --check.E2E_INCLUDE_LIFECYCLE=1 E2E_ONLY_LIFECYCLE=1 cargo xtask e2e: 13/13 Linux lifecycle scenarios pass (01–09, 23, 25–27), run on the head before the final rebase.tpn --check,check-crate-edges, shellcheck oninstall.sh, and a pwsh parse of the registry scripts pass.The full default e2e suite,
scripts/smoke_local.pyandcargo xtask coveragewere run on the earlier head, not re-run after the review fixes; CI runs them.Not verified locally: the Windows code paths (registry edit, self-delete, the case-insensitive exe match, lifecycle-22/24 including the expandable-entry check), which were only type- and lint-checked with
cargo clippy --target x86_64-pc-windows-gnu -- -D warnings. Deliberate type errors injected into thecfg(windows)code failed that check, so it really compiles them. They execute in the Windows lifecycle CI lane.