Skip to content

Remove amdgpu-install as an install method - #181

Open
juhovainio wants to merge 4 commits into
mainfrom
remove-amdgpu-install-issue-180
Open

Remove amdgpu-install as an install method#181
juhovainio wants to merge 4 commits into
mainfrom
remove-amdgpu-install-issue-180

Conversation

@juhovainio

@juhovainio juhovainio commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Summary

Closes #180. 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 any rocm-cli flow depending on it breaks on ROCm 10.0. This does a hard removal (no deprecation period) per the issue.

  • Converts dnf_driver_plan() and sles_driver_plan() (apps/rocm/src/main.rs) to the same repo-native flow apt_driver_plan() already uses: import the AMD GPG key, write a package-manager repo file pointing at repo.radeon.com/graphics/..., then install amdgpu-dkms directly — instead of downloading and running the amdgpu-install RPM.
  • 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 as amdgpu-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 dead amdgpu-install upstream-tracker arm, repoints stale-repo/kernel-mismatch fix guidance at rocm 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-installer id kept stable).
  • fix.rs: rewrites the static fix-12-installer recipe to match.

No changes needed in apps/rocm/src/therock.rs (already had no amdgpu-install references, matching the issue's note).

Breaking change

rocm examine --json renames two rocm_install_method values: "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/src returns zero matches
  • cargo 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-existing proc_lifecycle failures unrelated to this change, reproduced on a clean main checkout)
  • cargo build --workspace
  • cargo fmt --check

rominf
rominf previously requested changes Aug 5, 2026

@rominf rominf left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-3 is now self-contradictory. This PR updated diagnose.rs:505 to "rerun rocm install driver --dkms" but the static twin at fix.rs:109 still 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 update fails 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_id is available and the mv step above is already personalized per-host, so branching to one correct command with visible output would be safer and more consistent.
  • The verify string no longer verifies. diagnose.rs:1093 / fix.rs:299 greps rocm examine --json for rocm_install_method|rocm_version — both fields are always present, so the grep succeeds whether or not the fix worked. Only the trailing rocminfo | head -n 5 carries signal.
  • "apt"/"dnf" are now near-unreachable. In examine.rs:921-941 the marker loop runs first and unconditionally sets "repo-native"; the dpkg/rpm probes only run if 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_install has no unit tests, and there's no coverage for check_12/fix-12. The renamed sentinel values and the new SLES zypp marker paths ship untested, while the main.rs side 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-installrepo-native, tarball-or-otherrunfile-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.

@juhovainio

Copy link
Copy Markdown
Collaborator Author

Thanks for the thorough review — addressed all three blocking points plus the non-blocking ones that were straightforward, in 0c10f5d.

  1. fix-12 doesn't fix the diagnosed state: restored the state-clearing step before quarantine+reinstall — dpkg --configure -a && apt-get install -f && apt-get purge -y amdgpu-dkms on apt hosts, dnf remove -y amdgpu-dkms / zypper remove -y amdgpu-dkms on the others — in both check_12 (host-specific, picks one family) and the static fix-12-installer (lists all three, comment-labeled, since the static catalog has no host to branch on).
  2. Destructive sudo mv emitted uncommented: now commented out (# sudo mv {r} {r}.bak # quarantine, do not delete yet), matching check_7's precedent exactly.
  3. Inaccurate provenance wording: REPO_NATIVE_INSTALL_MARKERS's doc comment now says only amdgpu.list/amdgpu.repo are written by rocm-cli's own install; the rest are legacy/manual-setup names it never creates.

Non-blocking, also fixed:

  • fix-3's stale --no-dkms--dkms (the real flag).
  • The silent apt||dnf||zypper refresh chain is now a single command, picked from the host's detected package-manager family (via rocm_repos_seen, falling back to distro_id) — visible output instead of /dev/null.
  • verify now greps for the actual "repo-native" value instead of field names that are always present.
  • Added unit tests: repo_pkg_family, check_12/fix-12's command shape (asserts the state-clearing step and the commented-out mv), and a table-driven test over the marker → install-method mapping (split probe_rocm_install's marker logic into a pure repo_native_method_from_markers fn so it's testable without touching the filesystem).

Left as-is (per your own framing as non-blocking / worth-a-conscious-call, not requiring a code change):

  • "apt"/"dnf" near-unreachability — pre-existing ordering, agree it's worth a future look but out of scope here.
  • The install-method value rename (amdgpu-installrepo-native, tarball-or-otherrunfile-or-tarball) is a breaking change to rocm examine --json's machine-readable output; noting it here for the record since nothing in-tree consumes it and there's no deprecation window.

cargo fmt --check, cargo clippy --workspace --all-targets, and cargo test -p rocm-core all clean locally (two pre-existing proc_lifecycle test failures are unrelated to this change and reproduce identically on the pre-review commit — looks like a sandbox signal-handling limitation, not something this PR touched).

@rominf rominf left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 clearingdiagnose.rs:1090-1101 now emits dpkg --configure -a / apt-get install -f / purge (or the dnf/zypper equivalent) before quarantine and reinstall, and the static twin at fix.rs:293-296 lists all three labeled blocks. The new repo_native_broken_fires_and_clears_state_before_reinstall test pins the ordering.
  • sudo mv — now the commented # sudo mv {r} {r}.bak # quarantine, do not delete yet form, byte-identical to the check_7 precedent rather than an approximation of it.
  • Provenance wordingexamine.rs:44-49 names 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:

  1. Reuse resolve_package_manager for the fallback, and parse ID_LIKE into Examination so it can. That branch is wrong today.
  2. 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.
  3. Optionally treat a repo-dir/distro_id disagreement 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:293 puts it above the removal commands, but the three refresh commands at fix.rs:298-300 are 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 forces kw_score = 0, so the score is 0 regardless of install method — the method gate the name describes is never exercised. Passing the same dpkg: error processing package amdgpu-dkms symptom as the positive test with a non-repo-native method would actually probe it. Worth doing because score += kw_score is 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 live mv or dropping dpkg --configure -a.
  • --json rename. 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>
@juhovainio
juhovainio force-pushed the remove-amdgpu-install-issue-180 branch from 7518b1b to 7e16a06 Compare August 7, 2026 11:56
@juhovainio
juhovainio dismissed rominf’s stale review August 7, 2026 11:57

Feedback addressed

…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>
@juhovainio

juhovainio commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator Author

Follow-up commit (28fe71e) on this branch: apt_driver_plan(), dnf_driver_plan(), and sles_driver_plan() all generated a repo baseurl under repo.radeon.com/graphics/{rocm_version}/. Checked against AMD's current package-manager install docs and the live directory tree: that path only carries the userspace graphics/mesa/llvm stack and never ships amdgpu-dkms. The kernel driver package lives in a separately-versioned tree, repo.radeon.com/amdgpu/{driver_version}/ (e.g. 31.40.1), which has no relation to the ROCm version this code was interpolating (repo.radeon.com/amdgpu/7.2.4/ 404s). As written, the generated dnf install amdgpu-dkms / zypper install amdgpu-dkms / apt equivalent would have failed to resolve the package on all three distro families.

Fix: repointed all three driver plans at /amdgpu/{driver_version}/, and swapped 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 ROCm-version-to-driver-version mapping table is needed.

Re: the amdrocm-* / repo.amd.com direction referenced in this issue — cross-checked against TheRock's RFC0012 (Repo Structure), the approved spec behind that naming. It explicitly reserves amdgpu/ under each stream subdomain "for future use; future GPU driver artifacts" and defers the actual repo.radeon.comrepo.amd.com driver consolidation to an unpublished companion RFC (RFC00XX-Repository-Package.md). So repo.amd.com is the right long-term target for ROCm software packages, but the driver isn't served there yet — today's fix (repo.radeon.com/amdgpu/) is correct in the interim. Tracking the eventual migration internally so it isn't lost once that RFC ships.

@juhovainio
juhovainio requested review from a team and rominf August 7, 2026 13:06
@rominf
rominf requested a lite review from Copilot August 7, 2026 14:36

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-install RPM/bootstrap and toward writing repo definitions + installing amdgpu-dkms directly (APT/DNF/Zypper).
  • Updates rocm examine install-method detection/markers and reported values (including SLES/zypp markers and an internal ID_LIKE capture for package-family fallback).
  • Updates rocm diagnose + rocm fix guidance/recipes to remove amdgpu-install references and keep the fix-12-installer ID 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 baseurl line 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 baseurl line is single-quoted, which prevents ${ROCM_CLI_AMDGPU_DRIVER_VERSION:-latest} from expanding. This would create a .repo file 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.

Comment thread apps/rocm/src/main.rs
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"
@rominf
rominf requested a balanced review from Copilot August 7, 2026 14:48

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 and apt-get update cannot 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 baseurl prevents ${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 | head returns the status of head, so this verification may succeed even if rocminfo is absent or fails to access the GPU. Check rocminfo directly 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 rocminfo failures because a successful head determines the pipeline status in the default shell. The verification can therefore return success when rocminfo is missing or cannot initialize a GPU. Run rocminfo directly (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_plan creates no commands, and without --yes, install_driver returns 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 driver without --dkms generates 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_seen is broader than the repo files written by the driver installer: probe_rocm_install adds every filename containing rocm, amdgpu, or radeon (examine.rs:981-995). This loop therefore tells a normal repo-native user to quarantine rocm.list/rocm.repo too, but rocm install driver only recreates amdgpu.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"
            ));

Comment thread apps/rocm/src/main.rs
&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 rominf left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  • dkms is a plain #[arg(long)] defaulting to false (main.rs:528)
  • the apt/dnf/sles plans only populate commands and set mutating: dkms inside if dkms { ... }
  • install_driver() returns early with executed: false when !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_family can return "pacman" via resolve_package_manager, but the state-clearing match has no pacman arm while the refresh match's _ arm still pushes sudo 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_score short-circuits on an empty symptom, so the assertion holds regardless of rocm_install_method. The install-method gate the name claims to pin is never exercised.
  • Quarantine loop scope. diagnose.rs:1106-1116 iterates all of rocm_repos_seen (any filename matching rocm/amdgpu/radeon), but rocm install driver only recreates amdgpu.list/amdgpu.repo. A user who un-comments the suggested mv for a hand-added rocm.list loses 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.
  • verify masks exit status. rocminfo | head -n 5 reports head's status. Only ever printed, never executed, so this is recipe quality rather than a functional gate. Related pre-existing drift: fix-3's verify uses head -n 20 in diagnose.rs:510 but head -n 5 in fix.rs:119 — predates this PR, worth a drive-by while the file is open.
  • zypper trust path. :3009 runs install -y with gpgcheck=1 relying on the rpm --import at :2997. Since stdin is Stdio::null() this fails fast rather than hanging, but --gpg-auto-import-keys would 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 SUSEConnect prelude 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 new repo_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.rs twins 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 and verify strings would be cheap.
  • The default moving from pinned 7.2.4 to floating latest is 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove amdgpu-install as a rocm-cli installation method in ROCm 10.0

3 participants