Remove amdgpu-install as an install method - #181
Conversation
rominf
left a comment
There was a problem hiding this comment.
The apps/rocm/src/main.rs half of this is clean — the per-ecosystem conversion is faithful rather than copy-pasted (rpm --import vs curl | gpg --dearmor + signed-by=, autorefresh=0/type=rpm-md only where zypper wants them), dead helpers were removed rather than orphaned, and the tests were strengthened to assert the actual repo-file body and target path. I also confirmed the removal is complete: no stale amdgpu-install references anywhere in apps/, crates/, docs, tests, CI, or completions.
Three things in the diagnose.rs/fix.rs half I'd like resolved first.
1. fix-12 no longer fixes the state it diagnoses
check_12 fires on KEYWORDS_DPKG_BROKEN (half-configured, dkms .*failed, dpkg: error, sub-process /usr/bin/dpkg returned). The old remedy ran amdgpu-install --uninstall + apt autoremove --purge, which actually removed the broken package.
The new remedy (crates/rocm-core/src/diagnose.rs:1063-1085, mirrored in crates/rocm-core/src/fix.rs:290-295) only moves repo files aside, refreshes metadata, and runs rocm install driver. Moving a repo definition file doesn't touch a package recorded half-configured in dpkg's status DB, or a failed DKMS build under /var/lib/dkms/amdgpu/ — so the apt-get install amdgpu-dkms inside rocm install driver hits the same interrupted-dpkg error. The recipe's own summary says it will "clear the half-configured state", which it no longer does.
Suggest restoring a state-clearing step before the reinstall (sudo dpkg --configure -a or sudo apt-get -f install, plus an explicit purge of the broken driver package), in both the dynamic Fix and the static FixRecipe so they don't drift.
2. Destructive sudo mv is emitted ready-to-run, against the file's own precedent
diagnose.rs:1074-1078 pushes format!("sudo mv {r} {r}.bak") uncommented, for every entry in rocm_repos_seen. But the sibling checker in the same file, check_7_stale_repos (diagnose.rs:747-751), deliberately emits the identical command commented out with a guard — # sudo mv {r} {r}.bak # quarantine, do not delete yet — and this PR's own static twin at fix.rs:291 also comments it out.
That guard matters because rocm_repos_seen isn't provenance-checked: examine.rs:947-966 scans the apt/yum/zypp repo dirs and adds any filename containing rocm, amdgpu, or radeon. A distro-shipped, mirror, or hand-added repo file gets swept in and the user is handed a ready-to-paste command to disable it.
Suggest commenting these out exactly as check_7 and fix.rs do, or restricting the loop to the markers rocm-cli actually writes.
3. The new provenance wording isn't accurate
examine.rs:47-48 now reads "Repo files dropped by rocm-cli's repo-native package-manager install flow". rocm-cli writes only three of the seven — /etc/apt/sources.list.d/amdgpu.list (main.rs:2812), /etc/yum.repos.d/amdgpu.repo (main.rs:2908), /etc/zypp/repos.d/amdgpu.repo (main.rs:3002). The other four (rocm.list, radeon.list, and the two rocm.repos) are legacy/manual-doc names rocm-cli never creates; the previous comment was accurate for them. The claim propagates into user-visible text at diagnose.rs:1063 and fix.rs:288, which is partly what makes (2) read as safe.
Suggest wording it as what the constant actually models — AMD repo files written either by rocm-cli's install or by the documented manual/legacy setup, whose presence marks a package-manager-repo-managed ROCm.
Non-blocking
fix-3is now self-contradictory. This PR updateddiagnose.rs:505to "rerunrocm install driver --dkms" but the static twin atfix.rs:109still says "rerun with--no-dkms" — a flag that doesn't exist anywhere in the CLI.- The refresh chain hides the diagnostic it needs to show.
diagnose.rs:1079-1082/fix.rs:293:sudo apt update 2>/dev/null || sudo dnf clean all 2>/dev/null || sudo zypper refresh 2>/dev/null. On a Debian host with broken repo state — exactly when this fires —apt updatefails with stderr discarded, then falls through to two commands that don't exist, also silenced. This also undercuts the recipe's own note ("if the package-manager update/refresh warns it will remove unrelated packages, stop"), since the warning was just sent to/dev/null.distro/distro_idis available and themvstep above is already personalized per-host, so branching to one correct command with visible output would be safer and more consistent. - The
verifystring no longer verifies.diagnose.rs:1093/fix.rs:299grepsrocm examine --jsonforrocm_install_method|rocm_version— both fields are always present, so the grep succeeds whether or not the fix worked. Only the trailingrocminfo | head -n 5carries signal. "apt"/"dnf"are now near-unreachable. Inexamine.rs:921-941the marker loop runs first and unconditionally sets"repo-native"; the dpkg/rpm probes only runif is_empty(). Any repo-installed ROCm has a marker, so those two values only surface when the repo file was deleted but the package remains. Pre-existing ordering, but the rename makes it more visible — worth deciding whether they still earn their place.- Test gap.
probe_rocm_installhas no unit tests, and there's no coverage forcheck_12/fix-12. The renamed sentinel values and the new SLES zypp marker paths ship untested, while themain.rsside has 19 solid driver-plan tests. A table-driven test over the marker → method mapping would be proportionate.
Worth a conscious call
The rocm examine --json value rename (amdgpu-install → repo-native, tarball-or-other → runfile-or-tarball) is a silent breaking change to a machine-readable interface. Nothing in-tree consumes it, but there's no schema or deprecation window, so any external tooling string-matching the old values breaks. Probably worth a line in the PR body or release notes.
CI
E2E tests (Strix Halo, Ubuntu) looks unrelated to this change. The job reports 0 unexpected failure(s) — all 7 failing scenarios are declared xfail, and the non-zero exit comes solely from 3 XPASS entries (chat/serve scenarios that were expected to fail on that host but passed). None of them touches driver install, the examine install-method field, diagnose, or fix; the two examine/diagnose scenarios in that suite both pass. The same job failed the same way on #177 with a superset of the same XPASSes, on a diff that also doesn't touch driver install, and it's green on this PR's merge base. Looks like host-state-dependent expectation drift — a re-run should clear it, and pruning the stale expectations.toml entries belongs in its own PR.
E2E tests (GPU) was still running when I looked; worth confirming it lands green.
Verified locally on the branch: cargo fmt --check clean, cargo clippy --workspace --all-targets clean, cargo test -p rocm 388 passed.
|
Thanks for the thorough review — addressed all three blocking points plus the non-blocking ones that were straightforward, in 0c10f5d.
Non-blocking, also fixed:
Left as-is (per your own framing as non-blocking / worth-a-conscious-call, not requiring a code change):
|
rominf
left a comment
There was a problem hiding this comment.
Re-reviewed 231c5f78..7518b1ba. All three blocking items are genuinely fixed — I went looking for superficial fixes and didn't find any:
- fix-12 state clearing —
diagnose.rs:1090-1101now emitsdpkg --configure -a/apt-get install -f/purge(or the dnf/zypper equivalent) before quarantine and reinstall, and the static twin atfix.rs:293-296lists all three labeled blocks. The newrepo_native_broken_fires_and_clears_state_before_reinstalltest pins the ordering. sudo mv— now the commented# sudo mv {r} {r}.bak # quarantine, do not delete yetform, byte-identical to thecheck_7precedent rather than an approximation of it.- Provenance wording —
examine.rs:44-49names all seven markers and gets the 3-written / 4-legacy split right. I checked it against the actual write sites (main.rs:2812,:2908,:3002) and there are no others.
The non-blocking items from last round are also handled: the --no-dkms twin contradiction, the 2>/dev/null silencing (the "stop if the refresh warns" note is reachable now), and the verify grep. On that last one — grep -q '"rocm_install_method": "repo-native"' was the easiest thing to get subtly wrong, so I verified the colon-space form actually matches: main.rs:1832 uses to_string_pretty, and Examination has no rename_all or skip_serializing_if, so that literal spacing is what's emitted.
Extracting repo_native_method_from_markers as a pure function to make the marker logic testable without touching the filesystem was the right call, and the reverse-the-input-order test is a nice touch.
One thing that needs doing before merge
DCO. The merge commit 7518b1ba is signed but has an empty Signed-off-by trailer, which is why Commit signatures + sign-off is red — both of your real commits (231c5f78, 0c10f5d5) are fine. Rebasing onto fresh main instead of merging fixes it and avoids recreating the problem.
windows-build-and-test is unrelated: only lifecycle-windows-http-install failed, on a loopback-HTTP error occurred while sending the request, with 98/99 steps passing. Same signature as the request-fragmentation flake addressed in #139, and green on main. Worth a re-run so the PR isn't sitting red. E2E tests (GPU) landed green, as did the other four E2E lanes.
On repo_pkg_family
The doc comment at diagnose.rs:1049 says the repo-dir family is known "with certainty" and treats distro_id as the weaker signal. Preferring the repo dir for the primary path is defensible — it's direct evidence of which package manager's repo config these commands act on, whereas distro_id reaches the same answer via a lookup table. But the certainty claim doesn't hold, and the ordering introduces a failure mode distro_id wouldn't have:
rocm_repos_seen isn't curated. examine.rs:974-990 reads all three repo dirs and pushes any filename containing rocm/amdgpu/radeon. A leftover /etc/yum.repos.d/rocm.repo on a Debian host — image layer, or someone once installed yum — makes repo_pkg_family return "dnf" and hands the user sudo dnf remove -y amdgpu-dkms on Ubuntu. The apt → dnf → zypper order also silently picks apt when two families are present rather than noticing the contradiction.
The more actionable half is the fallback, though — the one branch where distro_id is the only signal is the least careful code in the function. diagnose.rs:1066-1070 is a hardcoded list with _ => "apt" and no ID_LIKE; ID_LIKE isn't merely unconsulted, it's never parsed (examine.rs:425-426 captures only ID and VERSION_ID). Meanwhile openmpi.rs:917 in this same crate already has a tested resolve_package_manager(os_id, id_like) that handles derivatives, amzn included. So an Amazon Linux host with no ROCm repo file is told to run apt-get purge.
Suggestions, in order of value:
- Reuse
resolve_package_managerfor the fallback, and parseID_LIKEintoExaminationso it can. That branch is wrong today. - Reword the comment — the repo dir tells you a file exists under that dir, not that the dir's package manager manages ROCm. Something like "prefer the family implied by the repo dir the marker files live under, since that's the config these commands act on" says the same thing without the certainty claim.
- Optionally treat a repo-dir/
distro_iddisagreement as worth surfacing rather than silently resolving.
Bounded blast radius either way: fix-12 is auto_applicable: false with runner: None, so these are printed for a human to paste, not executed.
Smaller things
- The "run only the matching block" guard covers only the first triplet.
fix.rs:293puts it above the removal commands, but the three refresh commands atfix.rs:298-300are equally mutually exclusive and sit past an intervening comment line with no repeated guard. - One new test is near-vacuous.
repo_native_broken_does_not_fire_on_non_repo_native_installs_without_symptom(diagnose.rs:1774) uses an empty symptom, which already forceskw_score = 0, so the score is 0 regardless of install method — the method gate the name describes is never exercised. Passing the samedpkg: error processing package amdgpu-dkmssymptom as the positive test with a non-repo-native method would actually probe it. Worth doing becausescore += kw_scoreis unconditional and only the +20 bonus is method-gated, so the check can still fire — that behaviour is worth pinning down. - The static fix-12 recipe still has no test. It was half of what the first finding asked for, and
fix.rs's test module only covers catalog shape. A shape assertion mirroring the diagnose.rs one would stop someone re-enabling a livemvor droppingdpkg --configure -a. --jsonrename. Recorded in your comment, but the PR description is the durable record and doesn't mention it yet — there's no CHANGELOG in the repo, so that's the only place it'll survive.
Pre-existing, not yours, but adjacent: fix-3's verify is rocminfo | head -n 20 in diagnose.rs:510 vs head -n 5 in fix.rs:119.
Verified locally on 7518b1ba: cargo fmt --check clean, cargo clippy --workspace --all-targets clean (sources touched first, since post--- args aren't fingerprinted), cargo test -p rocm 388 passed.
amdgpu-install is being retired; legacy amdgpu-install/ packages on repo.radeon.com are frozen and won't move to the new amdrocm-* multi-arch/multi-version scheme, so rocm-cli flows depending on it break on ROCm 10.0. Convert the DNF and zypper driver install plans to the same repo-native flow apt already uses: import the AMD GPG key and write a package-manager repo file pointing at repo.radeon.com/graphics/..., then install amdgpu-dkms straight from the package manager instead of downloading and running the amdgpu-install RPM. Follow through on the three downstream consumers: - examine.rs: rename the install-method marker set and reported value from "amdgpu-install" to "repo-native" (this also fixes a pre-existing mislabeling bug, since rocm-cli's own apt-based install dropped the same marker files and was misreported as amdgpu-install), add SLES zypp repo paths, and rename the undetectable runfile/tarball fallback bucket to "runfile-or-tarball". - diagnose.rs: drop the dead amdgpu-install upstream-tracker arm, repoint stale-repo/kernel-mismatch fix guidance at `rocm install driver`, and rework the fix-12 broken-installer check to detect "repo-native" and produce a package-manager-agnostic reset+reinstall fix. - fix.rs: rewrite the static fix-12-installer recipe to match. Signed-off-by: Juho Vainio <juho.vainio@amd.com>
- Restore the state-clearing step (dpkg --configure -a / apt-get -f install / dnf|zypper remove amdgpu-dkms) before quarantining repos and reinstalling, in both check_12 and the static fix-12 recipe. - Comment out the repo-quarantine `mv` commands instead of emitting them ready-to-run, matching check_7's precedent; rocm_repos_seen isn't provenance-checked so a swept-in unrelated repo file must not be handed to the user as a one-paste disable command. - Fix REPO_NATIVE_INSTALL_MARKERS' doc comment: only amdgpu.list/ amdgpu.repo are written by rocm-cli's own install; the rest are legacy/manual-setup names it never creates. - Replace the silent apt||dnf||zypper refresh chain with a single command picked via the host's detected package-manager family (from rocm_repos_seen, falling back to distro_id), so errors are visible instead of redirected to /dev/null. - Tighten fix-12's verify string to actually assert repo-native rather than grep for fields that are always present. - Fix fix-3's stale --no-dkms reference to the real --dkms flag. - Add unit tests for repo_pkg_family, the check_12/fix-12 command shape, and a table-driven test over the marker -> install-method mapping (probe_rocm_install's core logic split into a pure, testable function). Signed-off-by: Juho Vainio <juho.vainio@amd.com>
repo_pkg_family's distro_id fallback was a hardcoded, ID_LIKE-unaware list defaulting unknown distros to apt, so an Amazon Linux host (or any RHEL rebuild) with no ROCm repo file was handed apt-get commands. Reuse openmpi's resolve_package_manager for the fallback instead, and parse /etc/os-release ID_LIKE into Examination (serde-skipped, since it's not part of the examine.py wire contract) so it's available. Also extend the fix-12 "run only the matching block" guard to the refresh command triplet, which was equally mutually exclusive but unguarded, and add coverage for the previously near-vacuous repo-native install-method test and the static fix-12 recipe shape. Signed-off-by: Juho Vainio <juho.vainio@amd.com>
7518b1b to
7e16a06
Compare
…amdgpu
apt/dnf/zypper driver plans generated a baseurl under repo.radeon.com/graphics/{rocm_version}/, which only carries the userspace graphics/mesa/llvm stack and never ships amdgpu-dkms. The kernel driver package lives in the separately-versioned repo.radeon.com/amdgpu/{driver_version}/ tree per AMD's current package-manager install docs. Point all three driver plans there and switch the version placeholder from ROCM_CLI_AMDGPU_VERSION (a ROCm version) to ROCM_CLI_AMDGPU_DRIVER_VERSION, defaulting to AMD's published 'latest' alias so no version-mapping table is needed.
Signed-off-by: Juho Vainio <juho.vainio@amd.com>
|
Follow-up commit (28fe71e) on this branch: Fix: repointed all three driver plans at Re: the |
There was a problem hiding this comment.
Pull request overview
Removes amdgpu-install as a supported/recognized ROCm installation method and standardizes Linux driver installation around “repo-native” package-manager flows, updating examine/diagnose/fix behavior accordingly.
Changes:
- Switches Linux driver install planning away from
amdgpu-installRPM/bootstrap and toward writing repo definitions + installingamdgpu-dkmsdirectly (APT/DNF/Zypper). - Updates
rocm examineinstall-method detection/markers and reported values (including SLES/zypp markers and an internalID_LIKEcapture for package-family fallback). - Updates
rocm diagnose+rocm fixguidance/recipes to removeamdgpu-installreferences and keep thefix-12-installerID stable under the new method naming.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
apps/rocm/src/main.rs |
Reworks driver install plan generation to repo-native repo-file creation and direct amdgpu-dkms installs; updates related tests/strings. |
crates/rocm-core/src/examine.rs |
Renames/expands marker detection to “repo-native”, adds zypp paths, and updates fallback install-method labeling. |
crates/rocm-core/src/diagnose.rs |
Removes the amdgpu-install tracker arm and rewires fix-12-installer diagnosis logic to the repo-native method with package-family-aware commands. |
crates/rocm-core/src/fix.rs |
Updates the static fix-12-installer recipe and related verification text to match repo-native guidance. |
crates/rocm-core/src/openmpi.rs |
Exposes package-manager resolution helpers for reuse by diagnosis logic. |
Suppressed comments (2)
apps/rocm/src/main.rs:3002
- The repo
baseurlline is single-quoted, which prevents${ROCM_CLI_AMDGPU_DRIVER_VERSION:-latest}from expanding. This would create a zypp repo file whose baseurl literally contains${...}, which zypper cannot use.
"printf '%s\\n' '[amdgpu]' 'name=amdgpu' 'baseurl={}' 'enabled=1' 'autorefresh=0' 'type=rpm-md' 'gpgcheck=1' 'gpgkey=https://repo.radeon.com/rocm/rocm.gpg.key' | sudo tee /etc/zypp/repos.d/amdgpu.repo >/dev/null",
apps/rocm/src/main.rs:2908
- The repo
baseurlline is single-quoted, which prevents${ROCM_CLI_AMDGPU_DRIVER_VERSION:-latest}from expanding. This would create a.repofile whose baseurl literally contains${...}, which dnf cannot resolve.
"printf '%s\\n' '[amdgpu]' 'name=amdgpu' 'baseurl={}' 'enabled=1' 'priority=50' 'gpgcheck=1' 'gpgkey=https://repo.radeon.com/rocm/rocm.gpg.key' | sudo tee /etc/yum.repos.d/amdgpu.repo >/dev/null",
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| DriverCommandPhase::Prepare, | ||
| &format!( | ||
| "printf '%s\\n' 'deb [arch=amd64 signed-by=/etc/apt/keyrings/rocm.gpg] https://repo.radeon.com/graphics/{repo_version_expr}/ubuntu {codename} main' | sudo tee /etc/apt/sources.list.d/amdgpu.list >/dev/null" | ||
| "printf '%s\\n' 'deb [arch=amd64 signed-by=/etc/apt/keyrings/rocm.gpg] https://repo.radeon.com/amdgpu/{repo_version_expr}/ubuntu {codename} main' | sudo tee /etc/apt/sources.list.d/amdgpu.list >/dev/null" |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Suppressed comments (7)
apps/rocm/src/main.rs:2812
- The repository entry is single-quoted, so the shell never expands
${ROCM_CLI_AMDGPU_DRIVER_VERSION:-latest}. The file receives that literal text andapt-get updatecannot resolve the intended repository URL. Double-quote the generated entry so the override/default is expanded when the command runs.
"printf '%s\\n' 'deb [arch=amd64 signed-by=/etc/apt/keyrings/rocm.gpg] https://repo.radeon.com/amdgpu/{repo_version_expr}/ubuntu {codename} main' | sudo tee /etc/apt/sources.list.d/amdgpu.list >/dev/null"
apps/rocm/src/main.rs:3002
- The single-quoted
baseurlprevents${ROCM_CLI_AMDGPU_DRIVER_VERSION:-latest}from expanding, leaving an unusable literal URL in the zypp repo file. Use double quotes for this argument.
"printf '%s\\n' '[amdgpu]' 'name=amdgpu' 'baseurl={}' 'enabled=1' 'autorefresh=0' 'type=rpm-md' 'gpgcheck=1' 'gpgkey=https://repo.radeon.com/rocm/rocm.gpg.key' | sudo tee /etc/zypp/repos.d/amdgpu.repo >/dev/null",
crates/rocm-core/src/fix.rs:306
rocminfo | headreturns the status ofhead, so this verification may succeed even ifrocminfois absent or fails to access the GPU. Checkrocminfodirectly instead of masking its status through a pipeline.
verify: "rocm examine --json | grep -q '\"rocm_install_method\": \"repo-native\"' && rocminfo | head -n 5",
crates/rocm-core/src/diagnose.rs:1135
- The final pipeline masks
rocminfofailures because a successfulheaddetermines the pipeline status in the default shell. The verification can therefore return success whenrocminfois missing or cannot initialize a GPU. Runrocminfodirectly (output can be redirected) so its exit status is checked.
verify: "rocm examine --json | grep -q '\"rocm_install_method\": \"repo-native\"' && rocminfo | head -n 5".to_owned(),
crates/rocm-core/src/diagnose.rs:1126
- This command does not reinstall anything: without
--dkms,build_driver_install_plancreates no commands, and without--yes,install_driverreturns the plan without executing it (apps/rocm/src/main.rs:2199-2203,2777-2833). Show the review command and the explicitly approved execution command instead.
This issue also appears on line 1135 of the same file.
commands.push("rocm install driver".to_owned());
crates/rocm-core/src/fix.rs:301
- The advertised reinstall is a no-op because
rocm install driverwithout--dkmsgenerates only a non-mutating preflight plan, and execution additionally requires--yes. The manual recipe should include the actual approved install command.
This issue also appears on line 306 of the same file.
"rocm install driver",
crates/rocm-core/src/diagnose.rs:1115
rocm_repos_seenis broader than the repo files written by the driver installer:probe_rocm_installadds every filename containingrocm,amdgpu, orradeon(examine.rs:981-995). This loop therefore tells a normal repo-native user to quarantinerocm.list/rocm.repotoo, butrocm install driveronly recreatesamdgpu.list/amdgpu.repo; ROCm userspace updates remain disabled afterward. Limit this reset to the files the driver flow owns.
for r in &e.rocm_repos_seen {
commands.push(format!(
"# sudo mv {r} {r}.bak # quarantine, do not delete yet"
));
| &format!( | ||
| "sudo dnf install -y {}", | ||
| amdgpu_install_rpm_url(&repo_version_expr, &version_id, distro) | ||
| "printf '%s\\n' '[amdgpu]' 'name=amdgpu' 'baseurl={}' 'enabled=1' 'priority=50' 'gpgcheck=1' 'gpgkey=https://repo.radeon.com/rocm/rocm.gpg.key' | sudo tee /etc/yum.repos.d/amdgpu.repo >/dev/null", |
rominf
left a comment
There was a problem hiding this comment.
Reviewed at head 28fe71ef. The conversion to a repo-native flow is the right direction and the removal is clean — no dead helpers, no orphaned env vars, no stale references anywhere in the tree. Two blocking issues before merge.
Blocking
1. Single-quoting prevents version expansion — dnf/zypper repo files get an invalid baseurl
apps/rocm/src/main.rs:2905-2911 (dnf) and :2999-3005 (zypper) emit:
printf '%s\n' '[amdgpu]' ... 'baseurl=https://repo.radeon.com/amdgpu/${ROCM_CLI_AMDGPU_DRIVER_VERSION:-latest}/rhel/9.7/main/x86_64' ... | sudo tee /etc/yum.repos.d/amdgpu.repo
The baseurl=... token is a single-quoted shell word. These commands execute through sh -c (crates/rocm-core/src/runtime.rs:165), which does not expand ${VAR:-default} inside single quotes even when the variable is exported. The written file therefore contains a literal ${ROCM_CLI_AMDGPU_DRIVER_VERSION:-latest}, the repo cannot resolve, and the following install -y amdgpu-dkms fails.
This is new for dnf/zypper: the base revision built these URLs with unquoted interpolation (sudo dnf install -y {url}), which expands correctly. The move to printf | tee introduced the quoting. The apt path at :2812 has the same defect, but that one is pre-existing — worth fixing all three together given this PR makes the repo-native flow the supported path.
Suggested fix: resolve the version in Rust (std::env::var("ROCM_CLI_AMDGPU_DRIVER_VERSION"), defaulting to latest) and emit a fully literal URL, rather than relying on shell expansion surviving the quoting.
Note that no current test catches this — the 19 driver-plan tests assert the rendered plan contains the literal ${ROCM_CLI_AMDGPU_DRIVER_VERSION:-latest} string, so they pin the bug rather than the resulting URL. A test that pipes one generated command through sh -c and asserts the resolved baseurl would close the gap.
2. fix-12's remediation step is a no-op
crates/rocm-core/src/diagnose.rs:1126 and crates/rocm-core/src/fix.rs:301 both end with bare rocm install driver. That command reinstalls nothing:
dkmsis a plain#[arg(long)]defaulting tofalse(main.rs:528)- the apt/dnf/sles plans only populate
commandsand setmutating: dkmsinsideif dkms { ... } install_driver()returns early withexecuted: falsewhen!plan.mutating(main.rs:2188-2205)
A user following fix-12 verbatim gets a plan reporting no execution commands and "DKMS was not requested; this is a non-mutating preflight plan." The codebase gets this right elsewhere — main.rs:2272 and check_3 both say rocm install driver --dkms. Suggest rocm install driver --dkms --yes in both twins.
Non-blocking
check_12's_ => {}arm.diagnose.rs:1092-1124:repo_pkg_familycan return"pacman"viaresolve_package_manager, but the state-clearingmatchhas no pacman arm while the refreshmatch's_arm still pushessudo apt update. Low reachability, but an explicit "unsupported package manager" note would beat emitting an apt command on Arch.repo_native_broken_does_not_fire_on_non_repo_native_installs_without_symptom(diagnose.rs:1791) is near-vacuous —keyword_scoreshort-circuits on an empty symptom, so the assertion holds regardless ofrocm_install_method. The install-method gate the name claims to pin is never exercised.- Quarantine loop scope.
diagnose.rs:1106-1116iterates all ofrocm_repos_seen(any filename matchingrocm/amdgpu/radeon), butrocm install driveronly recreatesamdgpu.list/amdgpu.repo. A user who un-comments the suggestedmvfor a hand-addedrocm.listloses ROCm userspace updates permanently. Mitigated by the lines being emitted commented out; a one-line note distinguishing the file rocm-cli writes from legacy names would close it. verifymasks exit status.rocminfo | head -n 5reportshead's status. Only ever printed, never executed, so this is recipe quality rather than a functional gate. Related pre-existing drift: fix-3'sverifyuseshead -n 20indiagnose.rs:510buthead -n 5infix.rs:119— predates this PR, worth a drive-by while the file is open.- zypper trust path.
:3009runsinstall -ywithgpgcheck=1relying on therpm --importat:2997. Since stdin isStdio::null()this fails fast rather than hanging, but--gpg-auto-import-keyswould be more robust.
Design notes
- The apt/dnf/sles triplication now argues for a shared repo-file builder — the quoting bug above landed identically in two of the three call sites, which is direct evidence the duplication is costing correctness. A helper centralizing the quoting decision once would have prevented it. The apt-only pinning file, per-distro kernel-header prepares, and the SLES
SUSEConnectprelude should stay distinct. - Three separate notions of "which package manager does this host use" now coexist (
resolve_package_manager,build_driver_install_plan's distro match, and the newrepo_pkg_family). This PR partially reconciles two of them, which is the right call; the third still lacks a pacman branch. - 15 fix-ids have hand-maintained
diagnose.rs/fix.rstwins with no cross-file test — the fix-3 drift above is live proof review alone doesn't catch it. A single parity test over the id set andverifystrings would be cheap. - The default moving from pinned
7.2.4to floatinglatestis a real reproducibility tradeoff, defensible given it avoids maintaining a ROCm-to-driver version table, but worth stating as a deliberate decision.
Verification
I confirmed the --json rename has no in-tree consumers: a full-tree grep at this head for amdgpu-install, amdgpu_install, tarball-or-other, ROCM_CLI_AMDGPU_VERSION, ROCM_CLI_AMDGPU_PACKAGE_RELEASE, and repo.radeon.com/graphics returns zero hits across docs, scripts, e2e fixtures, .github/, and apps/rocmd.
One thing to check before merge: only the CodeQL checks have reported on 28fe71ef. Since that is the commit that repointed all three URLs, the unit-test, clippy, and E2E lanes should be confirmed green on this SHA specifically.
Summary
Closes #180.
amdgpu-installis being retired: legacyamdgpu-install/packages on repo.radeon.com are frozen and won't move to the newamdrocm-*multi-arch/multi-version scheme, so any rocm-cli flow depending on it breaks on ROCm 10.0. This does a hard removal (no deprecation period) per the issue.dnf_driver_plan()andsles_driver_plan()(apps/rocm/src/main.rs) to the same repo-native flowapt_driver_plan()already uses: import the AMD GPG key, write a package-manager repo file pointing atrepo.radeon.com/graphics/..., then installamdgpu-dkmsdirectly — instead of downloading and running theamdgpu-installRPM.examine.rs: renames the install-method marker set and reported value from"amdgpu-install"to"repo-native". This also fixes a pre-existing mislabeling bug — rocm-cli's own apt-based install dropped the same marker files and was being misreported asamdgpu-install. Also adds SLES zypp repo paths (previously undetected) and renames the undetectable runfile/tarball fallback bucket to"runfile-or-tarball".diagnose.rs: drops the deadamdgpu-installupstream-tracker arm, repoints stale-repo/kernel-mismatch fix guidance atrocm install driver, and reworks the fix-12 broken-installer check to detect"repo-native"and produce a package-manager-agnostic reset+reinstall fix (fix-12-installerid kept stable).fix.rs: rewrites the staticfix-12-installerrecipe to match.No changes needed in
apps/rocm/src/therock.rs(already had noamdgpu-installreferences, matching the issue's note).Breaking change
rocm examine --jsonrenames tworocm_install_methodvalues:"amdgpu-install"becomes"repo-native", and"tarball-or-other"becomes"runfile-or-tarball". Nothing in-tree consumes these strings and there's no deprecation window; any external tooling matching the old values needs updating. There's no CHANGELOG in this repo, so this note is the durable record.Test plan
grep -rn "amdgpu-install|amdgpu_install|AMDGPU_INSTALL" apps/rocm/src crates/rocm-core/srcreturns zero matchescargo test -p rocm(388 tests, incl. all 19 driver-plan tests covering RHEL/Oracle/Rocky/SLES fixtures)cargo test -p rocm-core(199 passed; 2 pre-existingproc_lifecyclefailures unrelated to this change, reproduced on a cleanmaincheckout)cargo build --workspacecargo fmt --check