Skip to content

Remove amdgpu-install as an install method - #181

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

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

Conversation

@juhovainio

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).

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

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>

@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.

- 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>
@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).

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

2 participants