From 0ed84bac0e5401c3c726b7266b2540a51323e192 Mon Sep 17 00:00:00 2001 From: Eugene Volen Date: Fri, 11 Sep 2026 10:36:04 +0000 Subject: [PATCH] feat(diagnose): report a code object manager that is not the runtime's own Builds on the copy search: with every copy of libamd_comgr known, the catalog can say when the one that loads belongs to a different installation than the HIP runtime that loads, and device code compilation therefore fails with an error naming neither. The rule is symmetric, and that is what makes it safe. The design note for this entry proposed a special case -- "when the active runtime is the managed runtime, the matching copy is the wheel copy" -- without which it "fires on every healthy CLI installation". That is a patch over a rule stated asymmetrically. Asking one question of both libraries instead, does the libamd_comgr that would load come from the same installation as the libamdhip64 that would load, makes every case fall out of the rule: a healthy managed install takes both from the managed runtime, so nothing differs and nothing is reported. A control that has to be written as an exception is a rule that has not been stated correctly yet. That turns on attributing a copy to its installation correctly. A managed runtime spreads its libraries across separate _rocm_sdk_* packages that sit beside each other under one site-packages, not nested under the runtime's own root at all, so ownership is decided by looking a found file's directory up against the same directories install_library_dirs already recorded for each root -- never by walking up from the file, which would call each package its own install and fire on the most common install we ship. That is the property the mutation check targets. Two further guards on firing: an unattributed copy is a gap in what the search knows rather than a finding about the machine, and the runtime must actually ship a copy of its own, or there is nothing to point the user at. The entry is print-only and ranks neither remedy. Removing a stack and reordering the search path can each break a working Python environment, and which is right depends on which stack the user means to keep. The finding says out loud that it describes the environment outside the managed runtimes. `rocm serve` puts a managed runtime's libraries first on purpose and gets a different, correct answer, and a report that did not name the environment it examined would read as a claim about one it never looked at. The rule above is now stated exactly once, as `comgr_matches_runtime` in examine.rs, and both this entry and `rocm examine --json`'s field of the same name call it rather than each restating the conditions -- so the two surfaces cannot disagree about one machine, and a regression test pins that. Fix option (b)'s advice matched an earlier, asymmetric statement of the rule; it now says to prefer the wheel's own copy of both libraries, which is what the symmetric rule actually asks the user to do. The machine-readable inspection now carries the same `install_root` attribution for the HIP runtime side that it already carried for the code object manager side, since the conflict question is about both libraries and a reader could not previously check either the attribution or the selection for one of them. Known roots are deduplicated across the whole list, not only where repeats happen to land next to each other. `dedup_by` drops consecutive duplicates, and these roots arrive from three independent sources -- the active install, a sorted /opt scan, and the managed runtimes -- so the active install colliding with a sibling was caught or missed depending on where the path sorted. A root recorded twice can make a machine holding one stack read as a machine holding two, which is the false conflict this entry exists not to report. hip_paths and hip_selected are `serde(default)` for the same reason the comgr fields are: an examination read back over the remote path may have been produced by an older CLI that never wrote them. Once a managed runtime is found, its root resolves to its `_rocm_sdk_devel` package directory, and the search used to guess a venv layout (`root/lib//site-packages`) to find its sibling packages; that guess does not hold for a real install, so the search now reads back the library directories the SDK probe already recorded (`library_paths`) instead of re-deriving them. This generalises the walker introduced in #384 (`find_library_copies`/`install_library_dirs`) so it serves the HIP side too, which is why `managed_runtime_roots` now returns `library_paths` plural rather than a single `site_packages` guess. The AppPaths::discover data-dir fix and the @requires-os:linux platform tags this commit originally carried for examine-17/18 belong with the scenario they correct, which #384 introduced -- moved there so the parent doesn't depend on the child to pass its own CI, and this commit now only rebases on top of that fix rather than restating it. Sweeping the same gap once it was visible: examine-17 (the host-independent code-object-manager scenario, renumbered by #384's rebase) asserts `hip_paths`/`hip_selected` the same way it asserts `comgr_paths`/`comgr_selected`, and the empty branch had the identical problem #384's review flagged for the comgr side -- `hip_paths: []` / `hip_selected: null` also hold by nothing more than `Examination`'s own defaults, so the assertion could not tell "probed, found none" from "never probed". `probe_comgr` now pushes a "no libamdhip64 found" note the same way it already did for libamd_comgr, and the HIP assertion now requires it in the empty branch. Verified by temporarily suppressing the note and watching the scenario go red, then restoring it. Review: the attribution half still did not survive a realistic managed runtime. `known_install_roots` registers a managed runtime by `candidate.root_path` alone -- `_rocm_sdk_devel` when the `devel` extra is installed -- and `libamd_comgr`/`libamdhip64` live in the sibling `_rocm_sdk_core` package, which sits *beside* `root_path`, not under it. A string-prefix match against the bare root therefore attributed nothing on exactly this shape: both libraries came back `install_root: ""`, `comgr_matches_runtime` hit its empty-root guard and returned `None` instead of `true`, and `check_18_comgr_conflict` could not fire for that runtime at all. The two unit tests meant to catch this (`a_library_deep_inside_a_managed_runtime_belongs_to_the_runtime`, `a_healthy_managed_installation_raises_no_report`) built their fixtures from a venv root (`/data/runtimes/therock/default`, `.../lib/python3.12/site-packages/_rocm_sdk_core/...`) that production never produces, so they passed regardless. Fixed at the root: `install_library_dirs` already walks, for every known root, exactly the directories that root's copies live in (that is the discovery-side fix this commit already made). `find_library_copies` now builds a directory -> owning-root map from that same walk and attributes each found file by looking its directory up in it, rather than by string-prefix match against the bare root. A sibling package's directory is simply one more entry in the map, keyed to the runtime that recorded it, so it attributes correctly without being nested under anything. `owning_install_root` and `record_library_copy` take that map instead of the bare root list; `the_nearest_enclosing_installation_claims_a_library` is removed because the "longest prefix wins" ambiguity it guarded against cannot arise under exact directory lookup. Both fixture tests above were rebuilt from the real installer shape (sibling `_rocm_sdk_*` packages under one `site-packages`, matching `apps/rocm/src/therock.rs`'s `ROCM_SDK_PROBE_SCRIPT`), and `a_library_deep_inside_a_managed_runtime_belongs_to_the_runtime` was confirmed to fail against the old string-prefix attribution before passing against the fix. `find_library_copies` also gained the `active_runtime_dirs` parameter runtime's own library directories are now searched first, ahead of the ambient `LD_LIBRARY_PATH`, for both comgr and HIP, since `rocm serve` prepends them for both libraries equally. The loader cache and the directory walk are now gathered once in `probe_comgr` and passed to both the comgr and HIP searches, rather than each search reading `LD_LIBRARY_PATH`, spawning `ldconfig -p`, and walking every managed runtime's directories again on every invocation -- neither depends on which library is being searched for, so the duplication cost an extra subprocess and directory walk on every `rocm examine`/`rocm diagnose` for no reason. `multi_copy_note` is split out as its own pure function so the "N copies ... would load" note stays independently testable now that `find_library_copies` only returns copies. Two doc-comment splices, both from a missing blank `///` line between two unrelated items: `check_18_comgr_conflict`'s doc paragraph had absorbed check_17's above it, leaving check_17 undocumented; the same shape left `known_install_roots` undocumented under a paragraph describing `dedup_roots_keeping_first`. Both are split back onto the function they actually describe. `docs/wsl.md` said `fix-19-shm-too-small` was the only non-WSL entry that applies on WSL; registering `check_18_comgr_conflict` for `["linux", "wsl"]` made that false the moment this entry existed. Updated to name both. The "Neither option is recommended..." sentence was stated independently in `fix.rs`'s static recipe and `diagnose.rs`'s dynamically-generated one, in slightly different words -- already drifted, and only the `fix.rs` copy was pinned by an e2e assertion, so the `diagnose.rs` copy could drift further with nothing to catch it. Both now read `fix::COMGR_CONFLICT_NEITHER_OPTION_RECOMMENDED`, so they cannot disagree. Not changed: `fix.rs`'s option (b) wording and `comgr_matches_runtime`'s parity with `check_18_comgr_conflict` were flagged in an earlier review round and were already correct by the time this round started -- verified against the current code rather than re-fixed. Rebase onto #384 and a second symmetry gap: #384's own review fixed `comgr_selected` picking `copies.first()` regardless of tier -- a `rocm-install`/`managed-runtime`-only hit used to be reported as "the one that would load" purely because it sorted first, when neither source is one the loader consults on its own. Rebasing that fix under this entry's HIP-runtime work surfaced a second instance of the same asymmetry a human reviewer caught directly on this branch: `probe_comgr`'s doc comment claimed comgr and HIP were searched and reported identically, but only comgr ever got a `multi_copy_note` call; the HIP branch only checked `hip.is_empty()` and pushed nothing for the "found, but only in a directory the loader does not consult" case the comgr side had just been fixed to report. Fixed by stating the tier rule once and applying it to both prefixes rather than fixing it twice: `select_loader_copy` replaces `copies.first()` with a search restricted to `LOADER_PATH_SOURCES` (`active-runtime`, `ld-library-path`, `loader-cache` -- the tiers the loader itself consults), and `copies_note` replaces `multi_copy_note` with the three-way note (no copies / more than one with a winner / one-or-more with none selected) that a `rocm-install`-only hit now needs. `probe_comgr` calls both exactly once for comgr and once for HIP, so there is one call site per library rather than one function's logic duplicated by hand into the other's branch, and the doc comment now says what the code does instead of what it used to do before #384's fix landed. Pinned by `a_search_dir_only_hit_is_not_selected_for_either_prefix`, which runs the same assertions for `COMGR_LIB_PREFIX` and `HIP_RUNTIME_LIB_PREFIX` in one test rather than two, so the rule is proven to apply to both rather than merely written to apply to both. The existing `loader_cache_outranks_search_dirs` and `the_active_runtimes_own_copy_outranks_the_ambient_environment`/ `ld_library_path_outranks_loader_cache_and_search_dirs` tests were carried through the rebase onto `find_library_copies`/ `select_loader_copy` rather than dropped, since they pin mutation-tested tier-ordering bugs #384 already fixed once. `comgr_search_dirs_labels_each_tier`, which tested the now-deleted `comgr_search_dirs_in`, is removed as redundant: `install_library_dirs`'s own tier-labelling is already covered by `a_library_deep_inside_a_managed_runtime_belongs_to_the_runtime` and `a_library_no_installation_claims_is_attributed_to_none`. Verified: `cargo fmt --all -- --check`, `cargo clippy --workspace --all-targets -- -D warnings`, `cargo clippy -p e2e-cucumber --test e2e -- -D warnings`, and `cargo test --workspace --all-targets` all pass (one `xtask::workflow_contract` test flaked under full-suite parallel load on a timing assertion against a real `rocm-smi` subprocess call and passed cleanly in isolation and on a full-suite rerun; unrelated to this change). Cross-compiled the whole workspace, tests included, for `x86_64-pc-windows-gnu` under `-D warnings` to confirm no code is left unix-only-reachable and therefore dead on Windows. `cargo xtask e2e -- -n skill` passes all 6 scenarios, since this round also touches `skills/rocm-doctor/reference.md`. Second review round, two independent reviewers filing the same four findings (verified rather than merely accepted): fix-18's evidence line claimed to describe "the environment as it stands outside the CLI's managed runtimes" unconditionally, even when a selection actually came from the active-runtime tier -- a served child's search order, not a plain shell's. That also means the `export LD_LIBRARY_PATH=...` remedy can be inert for exactly that case, since `lemonade_process_environment_vars` prepends the runtime's own directories ahead of whatever the remedy exports. Rewording the evidence line is the easy half; the detection logic that decides which sentence to print cannot move to plain-shell-only without reintroducing the false positives/negatives `comgr_matches_runtime` and `a_healthy_managed_installation_raises_no_report` were written to rule out, since that guarantee depends on both selections already being active-runtime-tier-aware. So `comgr_matches_runtime` is untouched; `check_18_comgr_conflict` now checks whether either selected copy's source is `active-runtime` and, when it is, swaps in an honest sentence plus a new remedy note stating the export may not reach a served child and pointing at reinstalling/repairing that runtime instead. Pinned by `an_active_runtime_s_own_hip_without_its_own_comgr_notes_the_scope_limit`, mutation-tested by forcing the branch condition false and watching the new test fail on the stale sentence, then reverting. `assert_hip_selection_is_stated`, the e2e HIP twin of the comgr selection assertion, had only two branches, so "copies found, but none on a tier the loader consults" -- `select_loader_copy` returning `None` on purpose, already pinned for `libamdhip64` by `a_search_dir_only_hit_is_not_selected_for_either_prefix` -- fell into the `else` arm and asserted against a selected copy that does not exist. Added the missing third branch, mirroring `assert_comgr_selection_is_stated` exactly, including its `LOADER_PATH_SOURCES` check on the selected copy's source, which the HIP step's final arm was also missing. Hoisted the shared `LOADER_PATH_SOURCES` const the two steps now both use so they cannot drift apart again. `skills/rocm-doctor/reference.md`'s "Closed catalog (24 failure modes)" heading and `SKILL.md`'s "the other 20 are print-only" line had both gone stale against the table's 25 rows and 4 auto-applicable fixes (`exactly_the_four_known_fixes_are_auto`); fixed to 25 and 21. `rocm_doctor_skill.feature` deliberately leaves both counts unparsed against the live CLI, and that scope decision stands -- the CLI is not wired to report "how many rows exist" as a fact an agent reads. What it does not rule out is checking the two numbers against each other, so `tests/e2e-cucumber/tests/skill_reference.rs` (plain `cargo test`, no binary needed) gained `free_standing_catalog_counts_match_the_table`, which parses both numbers back out of the two docs and asserts them against the table's own row and auto-fix counts. Mutation-tested by reverting each number one at a time and confirming the new test goes red, then restoring it. Open question from the first round, never answered: does `install_library_dirs` calling `collect_sdk_library_paths` unconditionally widen what a bare-metal `/opt/rocm` root probes in a way that matters? Traced it rather than taking the earlier answer on faith: every directory `install_library_dirs` adds for a root carries that root's own source (`rocm-install` or `managed-runtime`, never a loader-consulted tier), and `find_library_copies` only records a copy under that source when no loader-consulted tier (`active-runtime`/`ld-library-path`/`loader-cache`) already found the same path first -- so a wider probe can only add more `rocm-install`-sourced evidence, never promote a path onto a tier `select_loader_copy` would pick. The one place a wider probe does change an answer is `comgr_matches_runtime`'s "does the runtime ship its own copy" check, and there a new hit can only turn a false conflict report into `None` (not enough evidence), which is strictly more correct, not a new risk. The reviewer's reasoning holds; recorded here since the first round left it unanswered. Re-verified after this round's changes: `cargo fmt --all -- --check`, `cargo clippy --workspace --all-targets -- -D warnings`, `cargo clippy -p e2e-cucumber --test e2e -- -D warnings`, `cargo test --workspace --all-targets` (251 passed, 0 failed across 33 binaries), and `RUSTFLAGS="-D warnings" cargo check --workspace --all-targets --target x86_64-pc-windows-gnu` all pass clean. `cargo xtask e2e -- -n skill` passes all 6 scenarios again; `cargo xtask e2e -- -n examine-18` passes its 5 steps, confirming the new HIP e2e branch does not break the existing scenario. Signed-off-by: Eugene Volen --- crates/rocm-core/src/diagnose.rs | 504 ++++++++- crates/rocm-core/src/examine.rs | 981 ++++++++++++------ crates/rocm-core/src/fix.rs | 46 +- crates/rocm-core/src/lib.rs | 60 ++ docs/wsl.md | 7 +- skills/rocm-doctor/SKILL.md | 2 +- skills/rocm-doctor/reference.md | 5 +- tests/e2e-cucumber/features/diagnose.feature | 26 + tests/e2e-cucumber/features/examine.feature | 17 +- .../e2e-cucumber/tests/e2e/diagnose_steps.rs | 51 +- tests/e2e-cucumber/tests/e2e/examine_steps.rs | 117 ++- tests/e2e-cucumber/tests/skill_reference.rs | 104 ++ 12 files changed, 1601 insertions(+), 319 deletions(-) diff --git a/crates/rocm-core/src/diagnose.rs b/crates/rocm-core/src/diagnose.rs index e4652479c..cdd6bc9d6 100644 --- a/crates/rocm-core/src/diagnose.rs +++ b/crates/rocm-core/src/diagnose.rs @@ -208,6 +208,28 @@ const KEYWORDS_SHM_TOO_SMALL: KeywordTable = &[ ("no space left on device", 20, "the device reported full"), ]; +/// What a shadowed code object manager leaves in the error text. +/// +/// Every one of these is a *weak* signal on its own — a failed device-code +/// compilation has many causes, and this entry is established by the state of +/// the machine rather than by the words. The keywords only raise an already +/// structural finding; none of them reaches the match threshold alone. +const KEYWORDS_COMGR_CONFLICT: KeywordTable = &[ + (r"libamd_comgr", 40, "error mentions libamd_comgr"), + ("comgr", 30, "error mentions comgr"), + ( + "code object", + 25, + "error mentions a code object (what comgr produces)", + ), + ( + "hiperrornobinaryforgpu", + 25, + "HIP found no binary for the GPU", + ), + ("device code", 15, "error mentions device code (broad)"), +]; + const KEYWORDS_PATH_MISSING: KeywordTable = &[ ("rocminfo: command not found", 50, "rocminfo not on PATH"), ("command not found.*hipcc", 40, "hipcc not on PATH"), @@ -1399,6 +1421,137 @@ fn check_15_msvc_redist(e: &Examination, symptom: &str) -> Diagnosis { ) } +/// A code object manager library that does not belong to the active runtime. +/// +/// The rule is symmetric, and that is what makes it safe. Rather than asking +/// "is this the wheel copy?" and then carving out an exception for the managed +/// runtime, it asks one question of both libraries: **does the copy of +/// `libamd_comgr` that would load belong to the same installation as the copy of +/// `libamdhip64` that would load?** If it does, there is nothing wrong, whichever +/// installation that is. +/// +/// Every case falls out of that instead of being legislated. A healthy managed +/// install takes both libraries from the managed runtime, so nothing differs and +/// nothing is reported -- no special case required. A user who has put a wheel's +/// library directory on the search path while the runtime still resolves to the +/// system install takes them from two different places, and that is the failure. +/// +/// A control written as an exception is a rule that has not been stated +/// correctly yet. +fn check_18_comgr_conflict(e: &Examination, symptom: &str) -> Diagnosis { + const ID: &str = "fix-18-comgr-conflict"; + const TITLE: &str = "code object manager library does not belong to the active HIP runtime"; + + let (Some(comgr), Some(hip)) = (e.comgr_selected.as_ref(), e.hip_selected.as_ref()) else { + // Nothing to compare. A machine with no ROCm at all is not a machine + // with a conflict, and saying otherwise would be the worst kind of false + // report: confident, and about something that is not there. + return zero(ID, TITLE); + }; + // The same rule `probe_comgr` uses to compute `comgr_matches_runtime`. Calling + // it here rather than restating the conditions is what keeps `rocm examine + // --json` and this finding from disagreeing about one machine: an + // unattributed copy, a runtime with no code object manager of its own to + // prefer, or the two already agreeing are all `None`/`Some(true)` here and + // "nothing to report" below, exactly as they are for that field. + if crate::examine::comgr_matches_runtime(&e.comgr_paths, Some(comgr), Some(hip)) != Some(false) + { + return zero(ID, TITLE); + } + let Some(matching) = e + .comgr_paths + .iter() + .find(|copy| copy.install_root == hip.install_root) + else { + // Unreachable: `comgr_matches_runtime` only returns `Some(false)` when + // this search succeeds. Kept as a guard rather than an `unwrap` so a + // future change to either function fails safely instead of panicking. + return zero(ID, TITLE); + }; + + let mut score = 60; + let mut evidence = vec![ + format!( + "the code object manager that would load is {} (from {})", + comgr.path, comgr.install_root + ), + format!( + "the HIP runtime that would load is {} (from {})", + hip.path, hip.install_root + ), + format!( + "{} also ships a code object manager at {}", + hip.install_root, matching.path + ), + ]; + if !comgr.version.is_empty() && !matching.version.is_empty() { + evidence.push(format!( + "versions differ: {} would load, the runtime ships {}", + comgr.version, matching.version + )); + } + // Said out loud because the answer depends on which process asks, and + // because that answer changes what the `export` below is worth. + // `find_library_copies` checks `active-runtime` directories first -- that + // is a served child's search order (`rocm serve`/`rocm chat`), not a + // plain shell's. When neither selection came from there, the measurement + // really is the plain shell the user's own command ran in, and the + // `export` is the whole fix. When either did, that half of the + // measurement reflects what the active managed runtime (and anything it + // serves) loads, not a plain shell -- and a shell `export` is not + // guaranteed to reach a served child that prepends its own runtime's + // directories ahead of `LD_LIBRARY_PATH`. + let active_runtime_involved = + comgr.source == "active-runtime" || hip.source == "active-runtime"; + evidence.push(if active_runtime_involved { + "an active managed runtime decided part of this: rocm serve/rocm chat would see this \ + result, a plain shell might not" + .to_owned() + } else { + "this describes the environment as it stands outside the CLI's managed runtimes".to_owned() + }); + + let (kw_score, kw_ev) = keyword_score(symptom, KEYWORDS_COMGR_CONFLICT); + score += kw_score; + evidence.extend(kw_ev); + + let mut notes = vec![crate::fix::COMGR_CONFLICT_NEITHER_OPTION_RECOMMENDED.to_owned()]; + if active_runtime_involved { + notes.push( + "rocm serve/rocm chat prepend the active managed runtime's own directories ahead \ + of LD_LIBRARY_PATH, so the export below may not change what they load; \ + reinstalling or repairing that runtime so it ships its own code object manager is \ + the fix that reaches it directly." + .to_owned(), + ); + } + + let fix = Fix { + summary: + "Make the code object manager and the HIP runtime come from the same installation." + .to_owned(), + commands: vec![ + format!("# The library that would load: {}", comgr.real_path), + format!("# The runtime that would load: {}", hip.real_path), + format!("# The runtime's own copy: {}", matching.real_path), + "# Either keep one stack and remove the other, or order the search".to_owned(), + "# path so the runtime's own copy is found first:".to_owned(), + format!( + "export LD_LIBRARY_PATH=\"{}:$LD_LIBRARY_PATH\"", + std::path::Path::new(&matching.real_path) + .parent() + .map_or_else(String::new, |dir| dir.to_string_lossy().into_owned()) + ), + ], + fix_id: ID.to_owned(), + auto_applicable: false, + verify: "python -c \"import torch; torch.zeros(1, device='cuda')\"".to_owned(), + notes, + ..Fix::default() + }; + finalize(ID, TITLE, score, evidence, fix) +} + /// The vLLM engine-startup import failure (EAI-8012). /// /// Keyword-only, and not by preference. The fact that decides this failure is @@ -2058,11 +2211,16 @@ const CHECKERS: &[Checker] = &[ (check_wsl_5_distro_too_old, WSL_ONLY), (check_wsl_6_host_driver_too_old, WSL_ONLY), (check_wsl_7_wsl1, WSL_ONLY), - // Both families, opting in explicitly as the platform split requires. The - // shortage has nothing to do with `amdgpu` or `/dev/kfd` -- it is the size - // of a tmpfs -- and WSL2 ships the same 64 MB default a container does, so - // leaving this tagged `linux` alone would silence it on one of the two - // platforms most likely to have it. + // Both families, opting in explicitly as the platform split requires. Which + // copy of a library the loader picks is not a question about the amdgpu + // module or the Windows host driver -- a wheel copy and a system copy + // collide on WSL2 exactly as they do on bare metal. Windows is deferred + // until this has proved the approach. + (check_18_comgr_conflict, &["linux", "wsl"]), + // Both families, for the same reason. The shortage has nothing to do with + // `amdgpu` or `/dev/kfd` -- it is the size of a tmpfs -- and WSL2 ships the + // same 64 MB default a container does, so leaving this tagged `linux` alone + // would silence it on one of the two platforms most likely to have it. (check_19_shm_too_small, &["linux", "wsl"]), ]; @@ -2535,6 +2693,85 @@ mod tests { } } + /// A library copy belonging to `install_root`, sitting at `path`. + fn copy_in(install_root: &str, path: &str, version: &str) -> crate::examine::LibraryCopy { + crate::examine::LibraryCopy { + path: path.to_owned(), + real_path: path.to_owned(), + version: version.to_owned(), + source: "test".to_owned(), + install_root: install_root.to_owned(), + } + } + + /// Like `copy_in`, but with an explicit loader-search source instead of + /// the default `"test"` -- needed to exercise the branches in fix-18 that + /// key off which tier a selection came from. + fn copy_in_with_source( + source: &str, + install_root: &str, + path: &str, + version: &str, + ) -> crate::examine::LibraryCopy { + crate::examine::LibraryCopy { + source: source.to_owned(), + ..copy_in(install_root, path, version) + } + } + + fn comgr_finding(report: &DiagnoseReport) -> Option<&Diagnosis> { + report + .matched + .iter() + .find(|d| d.id == "fix-18-comgr-conflict") + } + + #[test] + fn a_code_object_manager_from_another_installation_is_reported() { + // The failure this entry exists for: the runtime resolves to the system + // install while the library that compiles device code for it comes from + // a wheel, and the error the user sees names neither. + let mut e = linux_base(); + let wheel = copy_in("/wheel", "/wheel/lib/libamd_comgr.so.2", "2.8.0"); + let system = copy_in("/opt/rocm", "/opt/rocm/lib/libamd_comgr.so.3", "3.0.0"); + e.comgr_selected = Some(wheel.clone()); + e.comgr_paths = vec![wheel, system]; + e.hip_selected = Some(copy_in( + "/opt/rocm", + "/opt/rocm/lib/libamdhip64.so.6", + "6.4", + )); + + let report = diagnose(&e, ""); + let finding = comgr_finding(&report).expect("the mismatch must be reported"); + + assert!( + finding.score >= MIN_SCORE_FOR_MATCH, + "a mismatch established from machine state alone has to clear the \ + threshold without help from the symptom text: {}", + finding.score + ); + let fix = finding.fix.as_ref().expect("the finding must carry a plan"); + assert!( + !fix.auto_applicable, + "both remedies can break a working Python environment, so nothing here \ + may be applied for the user" + ); + let evidence = finding.evidence.join("\n"); + for expected in [ + "/wheel/lib/libamd_comgr.so.2", + "/opt/rocm", + "2.8.0", + "3.0.0", + ] { + assert!( + evidence.contains(expected), + "the report has to name each copy, where it came from and its \ + version -- `{expected}` is missing:\n{evidence}" + ); + } + } + #[test] fn a_partly_used_allowance_reports_what_is_left_as_well_as_the_size() { // The two numbers answer different questions, and the evidence only @@ -2578,6 +2815,40 @@ mod tests { ); } + #[test] + fn a_healthy_managed_installation_raises_no_report() { + // The control, and the reason the rule is stated symmetrically. A managed + // runtime spreads its libraries across separate `_rocm_sdk_*` packages + // that sit *beside* each other under one `site-packages` -- not nested + // under the runtime's own root (`_rocm_sdk_devel`) at all -- so the two + // copies sit in different directories of ONE installation. Anything + // that decided ownership by walking up from the file would call these + // two installations and fire on the most common install we ship. (A + // fixture nesting `_rocm_sdk_core` under the root, the shape the + // installer never produces, would pass by construction without + // proving that -- this one matches `apps/rocm/src/therock.rs`'s + // `ROCM_SDK_PROBE_SCRIPT` instead.) + let mut e = linux_base(); + let root = "/data/runtimes/therock/_rocm_sdk_devel"; + let comgr = copy_in( + root, + "/data/runtimes/therock/_rocm_sdk_core/lib/libamd_comgr.so.2", + "2.8.0", + ); + e.comgr_selected = Some(comgr.clone()); + e.comgr_paths = vec![comgr]; + e.hip_selected = Some(copy_in( + root, + "/data/runtimes/therock/_rocm_sdk_devel/lib/libamdhip64.so.6", + "6.4", + )); + + assert!( + comgr_finding(&diagnose(&e, "")).is_none(), + "both libraries come from one installation, so there is no conflict" + ); + } + #[test] fn an_unmeasured_shared_memory_allowance_is_not_reported() { // Unknown is not zero. `None` means the path was absent or the query @@ -2593,6 +2864,31 @@ mod tests { ); } + #[test] + fn a_second_copy_that_does_not_load_raises_no_report() { + // Holding two copies is not a fault. Only the one that loads matters. + let mut e = linux_base(); + let winner = copy_in("/opt/rocm", "/opt/rocm/lib/libamd_comgr.so.3", "3.0.0"); + let loser = copy_in("/wheel", "/wheel/lib/libamd_comgr.so.2", "2.8.0"); + e.comgr_selected = Some(winner.clone()); + e.comgr_paths = vec![winner, loser]; + e.hip_selected = Some(copy_in( + "/opt/rocm", + "/opt/rocm/lib/libamdhip64.so.6", + "6.4", + )); + + assert!( + comgr_finding(&diagnose(&e, "")).is_none(), + "the copy that loads belongs to the runtime, so nothing is wrong" + ); + assert_eq!( + e.comgr_paths.len(), + 2, + "the machine report still lists both copies; only the diagnosis stays quiet" + ); + } + #[test] fn being_in_a_container_raises_the_finding_without_creating_it() { // The container flag says the cause and the remedy are known exactly, so @@ -2624,6 +2920,204 @@ mod tests { ); } + #[test] + fn nothing_is_reported_when_there_is_nothing_to_compare() { + // A machine with no ROCm is not a machine with a conflict. Reporting one + // would be the worst kind of false finding: confident, and about + // something that is not there. + let cases = [ + ("neither library found", None, None), + ( + "no runtime to attribute the library to", + Some(copy_in("/opt/rocm", "/opt/rocm/lib/libamd_comgr.so.3", "3")), + None, + ), + ( + "a copy no known installation claims", + Some(copy_in("", "/somewhere/libamd_comgr.so.3", "3")), + Some(copy_in( + "/opt/rocm", + "/opt/rocm/lib/libamdhip64.so.6", + "6.4", + )), + ), + ]; + for (case, comgr, hip) in cases { + let mut e = linux_base(); + e.comgr_paths = comgr.iter().cloned().collect(); + e.comgr_selected = comgr; + e.hip_selected = hip; + assert!( + comgr_finding(&diagnose(&e, "")).is_none(), + "{case}: reported a conflict it could not have established" + ); + } + } + + #[test] + fn a_runtime_with_no_copy_of_its_own_is_not_a_conflict() { + // The second half of the rule. Without a copy belonging to the runtime + // there is nothing to switch to, so the advice would be empty -- and the + // machine is simply one where the only copy lives elsewhere. + let mut e = linux_base(); + let only = copy_in("/wheel", "/wheel/lib/libamd_comgr.so.2", "2.8.0"); + e.comgr_selected = Some(only.clone()); + e.comgr_paths = vec![only]; + e.hip_selected = Some(copy_in( + "/opt/rocm", + "/opt/rocm/lib/libamdhip64.so.6", + "6.4", + )); + + assert!( + comgr_finding(&diagnose(&e, "")).is_none(), + "the runtime ships no code object manager of its own, so there is no \ + alternative to point the user at" + ); + } + + #[test] + fn an_active_runtime_s_own_hip_without_its_own_comgr_notes_the_scope_limit() { + // Reachable, not theoretical: a managed runtime can ship its own HIP + // runtime while leaning on the system's code object manager. Then + // `hip_selected` comes from the `active-runtime` tier -- the order a + // served child (`rocm serve`/`rocm chat`) consults, not a plain + // shell's -- so the finding must say so instead of claiming the + // plain-shell sentence, and the `export` remedy must carry a caveat + // that it may not reach that served child. + let mut e = linux_base(); + let system = copy_in("/opt/rocm", "/opt/rocm/lib/libamd_comgr.so.3", "3.0.0"); + let runtime_comgr = copy_in( + "/data/runtimes/therock", + "/data/runtimes/therock/lib/libamd_comgr.so.2", + "2.8.0", + ); + e.comgr_selected = Some(system.clone()); + e.comgr_paths = vec![system, runtime_comgr]; + e.hip_selected = Some(copy_in_with_source( + "active-runtime", + "/data/runtimes/therock", + "/data/runtimes/therock/lib/libamdhip64.so.6", + "6.4", + )); + + let report = diagnose(&e, ""); + let finding = comgr_finding(&report).expect("the mismatch must be reported"); + let evidence = finding.evidence.join("\n"); + assert!( + evidence.contains("active managed runtime decided part of this"), + "the evidence must own up to the active-runtime tier rather than claim a \ + plain shell measured it:\n{evidence}" + ); + assert!( + !evidence.contains("outside the CLI's managed runtimes"), + "the plain-shell sentence must not be printed when the measurement is \ + not a plain shell's:\n{evidence}" + ); + let notes = finding + .fix + .as_ref() + .expect("the finding must carry a plan") + .notes + .join("\n"); + assert!( + notes.contains("may not change what they load"), + "the remedy has to say it might not reach a served child:\n{notes}" + ); + } + + #[test] + fn comgr_matches_runtime_never_disagrees_with_the_diagnosis() { + // `rocm examine --json`'s `comgr_matches_runtime` and `rocm diagnose`'s + // fix-18 finding answer the same question about the same machine. Both + // are driven by `comgr_matches_runtime` in `examine.rs`, so this checks + // the two surfaces stay in lockstep across every shape the catalog + // itself tests -- including the case that used to disagree: a runtime + // whose own installation ships no code object manager at all. + let cases: Vec<(&str, Examination)> = vec![ + ("neither library found", linux_base()), + { + let mut e = linux_base(); + e.comgr_selected = + Some(copy_in("/opt/rocm", "/opt/rocm/lib/libamd_comgr.so.3", "3")); + e.comgr_paths = vec![e.comgr_selected.clone().unwrap()]; + ("no runtime to attribute the library to", e) + }, + { + let mut e = linux_base(); + let unattributed = copy_in("", "/somewhere/libamd_comgr.so.3", "3"); + e.comgr_selected = Some(unattributed.clone()); + e.comgr_paths = vec![unattributed]; + e.hip_selected = Some(copy_in( + "/opt/rocm", + "/opt/rocm/lib/libamdhip64.so.6", + "6.4", + )); + ("a copy no known installation claims", e) + }, + { + // The disagreement this test guards against: the runtime's own + // install ships no comgr copy of its own to switch to. + let mut e = linux_base(); + let only = copy_in("/wheel", "/wheel/lib/libamd_comgr.so.2", "2.8.0"); + e.comgr_selected = Some(only.clone()); + e.comgr_paths = vec![only]; + e.hip_selected = Some(copy_in( + "/opt/rocm", + "/opt/rocm/lib/libamdhip64.so.6", + "6.4", + )); + ("a runtime with no copy of its own", e) + }, + { + let mut e = linux_base(); + let winner = copy_in("/opt/rocm", "/opt/rocm/lib/libamd_comgr.so.3", "3.0.0"); + let loser = copy_in("/wheel", "/wheel/lib/libamd_comgr.so.2", "2.8.0"); + e.comgr_selected = Some(winner.clone()); + e.comgr_paths = vec![winner, loser]; + e.hip_selected = Some(copy_in( + "/opt/rocm", + "/opt/rocm/lib/libamdhip64.so.6", + "6.4", + )); + ("the copy that loads already belongs to the runtime", e) + }, + { + let mut e = linux_base(); + let wheel = copy_in("/wheel", "/wheel/lib/libamd_comgr.so.2", "2.8.0"); + let system = copy_in("/opt/rocm", "/opt/rocm/lib/libamd_comgr.so.3", "3.0.0"); + e.comgr_selected = Some(wheel.clone()); + e.comgr_paths = vec![wheel, system]; + e.hip_selected = Some(copy_in( + "/opt/rocm", + "/opt/rocm/lib/libamdhip64.so.6", + "6.4", + )); + ("a genuine conflict", e) + }, + ]; + + for (case, e) in cases { + let matches_runtime = crate::examine::comgr_matches_runtime( + &e.comgr_paths, + e.comgr_selected.as_ref(), + e.hip_selected.as_ref(), + ); + let diagnosis_fires = comgr_finding(&diagnose(&e, "")).is_some(); + assert_eq!( + matches_runtime == Some(false), + diagnosis_fires, + "{case}: comgr_matches_runtime={matches_runtime:?} but the \ + diagnosis {}", + if diagnosis_fires { + "fired" + } else { + "did not fire" + } + ); + } + } + #[test] fn render_group_missing_is_diagnosed() { let mut e = linux_base(); diff --git a/crates/rocm-core/src/examine.rs b/crates/rocm-core/src/examine.rs index d6409ada7..13a85a71d 100644 --- a/crates/rocm-core/src/examine.rs +++ b/crates/rocm-core/src/examine.rs @@ -155,9 +155,13 @@ pub struct WslFacts { pub locally_probed: bool, } -/// One copy of the AMD code object manager library found on the machine. +/// One copy of a ROCm library found on the machine. +/// +/// Used for both the code object manager and the HIP runtime, because the +/// question asked of them is the same one: of the copies on this machine, which +/// would load, and which installation does it belong to. #[derive(Debug, Clone, Default, Serialize, Deserialize, PartialEq, Eq)] -pub struct ComgrCopy { +pub struct LibraryCopy { /// The path as found, before symlinks are resolved -- this is the name the /// loader would use, and the one a user will recognise. pub path: String, @@ -174,6 +178,17 @@ pub struct ComgrCopy { /// Where the search found it: `active-runtime`, `ld-library-path`, /// `loader-cache`, `rocm-install`, or `managed-runtime`. pub source: String, + /// The installation this copy belongs to, empty when no known installation + /// claims it. + /// + /// Matched against the known installation roots rather than derived from the + /// path, and this is the detail the whole diagnosis turns on. A managed + /// runtime spreads its libraries across several `_rocm_sdk_*` directories; + /// deriving a root by walking up from `.../_rocm_sdk_core/lib` would make + /// each of them look like a separate installation, and the entry would then + /// report a conflict on every healthy managed install -- the exact false + /// report this design exists to avoid. + pub install_root: String, } /// Structured machine state consumed by the diagnosis catalog. @@ -243,17 +258,47 @@ pub struct Examination { /// Every copy found, in an estimated loader search order. Not every tier is /// one the loader actually consults -- see [`Self::comgr_selected`]. #[serde(default)] - pub comgr_paths: Vec, + pub comgr_paths: Vec, /// The copy that would load: the first entry in `comgr_paths` found on the /// `active-runtime`, `ld-library-path` or `loader-cache` tier. `None` when /// no copy was found on one of those tiers, even when `comgr_paths` is not /// empty -- a `rocm-install` or `managed-runtime` hit is evidence a copy /// exists, not evidence the loader would pick it. #[serde(default)] - pub comgr_selected: Option, + pub comgr_selected: Option, /// Version of the selected copy; empty when it cannot be read from the name. #[serde(default)] pub comgr_version: String, + /// Whether the selected code object manager copy belongs to the same + /// installation as the selected HIP runtime. + /// + /// Computed by [`comgr_matches_runtime`], the same function + /// `check_18_comgr_conflict` in `diagnose.rs` uses to decide whether to + /// report a conflict -- so this field and that finding cannot disagree + /// about one machine. + /// + /// `None` when there is not enough evidence to call it either way: either + /// library missing, an unattributed copy on either side, or a runtime whose + /// own installation ships no code object manager at all to compare against. + #[serde(default)] + pub comgr_matches_runtime: Option, + + /// Every copy of the HIP runtime found, in an estimated loader search + /// order. Not every tier is one the loader actually consults -- see + /// [`Self::hip_selected`]. + /// + /// Searched the same way, and for the same reason: deciding whether the code + /// object manager belongs to the active runtime means knowing which runtime + /// is active, and that is the same question about a different file. + #[serde(default)] + pub hip_paths: Vec, + /// The HIP runtime copy that would load: the first entry in `hip_paths` + /// found on the `active-runtime`, `ld-library-path` or `loader-cache` + /// tier. `None` when no copy was found on one of those tiers, even when + /// `hip_paths` is not empty, for the same reason `comgr_selected` can be + /// `None` while `comgr_paths` is not. + #[serde(default)] + pub hip_selected: Option, // HIP SDK install (Windows) pub hip_sdk_path: String, @@ -355,6 +400,9 @@ impl Default for Examination { comgr_paths: Vec::new(), comgr_selected: None, comgr_version: String::new(), + comgr_matches_runtime: None, + hip_paths: Vec::new(), + hip_selected: None, hip_sdk_path: String::new(), hip_sdk_version: String::new(), hipinfo_present: false, @@ -2205,24 +2253,19 @@ const COMGR_LIB_PREFIX: &str = "libamd_comgr"; /// exists, not evidence anything would load it. const LOADER_PATH_SOURCES: [&str; 3] = ["active-runtime", "ld-library-path", "loader-cache"]; -/// Find every copy of the code object manager library, in loader search order. +/// Find every copy of `prefix`, decide which one (if any) would load, and note +/// the result -- applied identically to the code object manager and the HIP +/// runtime, since both are searched by [`find_library_copies`] and both answer +/// "which one would load" by the same rule: see [`select_loader_copy`]. /// -/// Every entry is evidence a copy exists; only the one returned as "selected" -/// (see [`find_comgr_copies`]) is one the loader would actually pick. A machine -/// can hold a system copy and a wheel copy, and when the one that loads does -/// not belong to the active runtime, device code compilation fails with an -/// error naming neither. -/// -/// `interpreter`'s `library_paths` -- the active managed runtime's own library -/// directories, when there is one -- are checked **first**, ahead of this -/// process's own `LD_LIBRARY_PATH`. That is not this process's search order; it -/// is a served child's. `rocm serve`/`rocm chat` prepend exactly those -/// directories onto `LD_LIBRARY_PATH` before launching the engine (see -/// `lemonade_process_environment_vars` and its vLLM counterpart), so they win -/// over whatever this process's own environment or the system loader cache -/// would otherwise resolve to. Reporting the plain-environment answer instead -/// would name the system's copy as "the one that would load" on a machine -/// where every process the CLI actually launches loads the wheel's. +/// Every entry in `copies` is evidence a copy exists; only the selected one is +/// one the loader would actually pick. A machine can hold a system copy and a +/// wheel copy, and when the code object manager that loads does not belong to +/// the active runtime, device code compilation fails with an error naming +/// neither -- which is why the HIP runtime gets exactly the same treatment: +/// deciding whether the code object manager belongs to the active runtime +/// means knowing which runtime is active, and that is the same question about +/// a different file. /// /// **This emulates the loader; it is not the loader.** It does not account for /// `RUNPATH`/`RPATH` on the calling binary, `ld.so.preload`, or a container that @@ -2234,41 +2277,195 @@ fn probe_comgr(e: &mut Examination, interpreter: Option<&FrameworkInterpreter>) let active_runtime_dirs: Vec = interpreter .map(|interpreter| interpreter.library_paths.clone()) .unwrap_or_default(); + let roots = known_install_roots(e); + // Computed once and shared by both searches below: neither the loader + // cache nor the directory walk depends on which library is being searched + // for, so computing them per prefix ran `ldconfig -p` and walked every + // managed runtime's lib/lib64 -> site-packages -> `_rocm_sdk_*` tree twice + // on every `rocm examine`/`rocm diagnose` invocation -- a command typically + // run repeatedly while debugging. + // + // `interpreter`'s `library_paths` -- the active managed runtime's own + // library directories, when there is one -- are checked **first**, ahead + // of this process's own `LD_LIBRARY_PATH`. That is not this process's + // search order; it is a served child's. `rocm serve`/`rocm chat` prepend + // exactly those directories onto `LD_LIBRARY_PATH` before launching the + // engine (see `lemonade_process_environment_vars` and its vLLM + // counterpart), so they win over whatever this process's own environment + // or the system loader cache would otherwise resolve to. Reporting the + // plain-environment answer instead would name the system's copy as "the + // one that would load" on a machine where every process the CLI actually + // launches loads the wheel's. let ld = std::env::var("LD_LIBRARY_PATH").unwrap_or_default(); - let loader_cache_paths = comgr_paths_in_loader_cache(); - let search_dirs = comgr_search_dirs(e); + let loader_cache_text = crate::ldconfig_cache(); + let dirs = install_library_dirs(&roots); + let dir_owners: std::collections::HashMap = dirs + .iter() + .map(|(dir, _source, root)| (dir.clone(), root.clone())) + .collect(); + + let comgr = find_library_copies( + COMGR_LIB_PREFIX, + &active_runtime_dirs, + &ld, + loader_cache_text.as_deref(), + &dirs, + &dir_owners, + ); + let hip = find_library_copies( + HIP_RUNTIME_LIB_PREFIX, + &active_runtime_dirs, + &ld, + loader_cache_text.as_deref(), + &dirs, + &dir_owners, + ); - let (copies, selected, note) = - find_comgr_copies(&active_runtime_dirs, &ld, &loader_cache_paths, &search_dirs); - if let Some(selected) = &selected { + let comgr_selected = select_loader_copy(&comgr); + if let Some(selected) = &comgr_selected { e.comgr_version.clone_from(&selected.version); } - e.comgr_selected = selected; - if let Some(note) = note { + if let Some(note) = copies_note(COMGR_LIB_PREFIX, &comgr, comgr_selected.as_ref()) { e.notes.push(note); } - e.comgr_paths = copies; + + let hip_selected = select_loader_copy(&hip); + if let Some(note) = copies_note(HIP_RUNTIME_LIB_PREFIX, &hip, hip_selected.as_ref()) { + e.notes.push(note); + } + + e.comgr_matches_runtime = + comgr_matches_runtime(&comgr, comgr_selected.as_ref(), hip_selected.as_ref()); + e.comgr_selected = comgr_selected; + e.hip_selected = hip_selected; + e.comgr_paths = comgr; + e.hip_paths = hip; +} + +/// Whether the selected code object manager belongs to the same installation as +/// the selected HIP runtime -- and, when it does not, whether that runtime ships +/// a copy of its own to prefer instead. +/// +/// This is the one place the conflict rule is stated. `probe_comgr` reports the +/// result as `comgr_matches_runtime` and `check_18_comgr_conflict` in +/// `diagnose.rs` turns a `Some(false)` into the finding; both call this function +/// rather than restating the rule, so the two surfaces of one question cannot +/// disagree about the same machine. +/// +/// `None` covers every case where there is not enough evidence to call it either +/// way: either library missing, an unattributed copy on either side, or -- the +/// case that matters most -- a runtime whose own installation ships no code +/// object manager at all. That last one is not a conflict: there is no copy of +/// its own for it to prefer, so pointing the user at "the runtime's own copy" +/// would be pointing at nothing. +pub(crate) fn comgr_matches_runtime( + comgr_paths: &[LibraryCopy], + comgr_selected: Option<&LibraryCopy>, + hip_selected: Option<&LibraryCopy>, +) -> Option { + let (comgr, hip) = (comgr_selected?, hip_selected?); + if comgr.install_root.is_empty() || hip.install_root.is_empty() { + return None; + } + if comgr.install_root == hip.install_root { + return Some(true); + } + let has_own_copy = comgr_paths + .iter() + .any(|copy| copy.install_root == hip.install_root); + if !has_own_copy { + return None; + } + Some(false) +} + +/// The HIP runtime. Which copy of it loads decides which installation is the +/// active one, which is the other half of the code object manager question. +const HIP_RUNTIME_LIB_PREFIX: &str = "libamdhip64"; + +/// The copy the loader would actually pick: the first entry whose source is +/// one of [`LOADER_PATH_SOURCES`]. `None` when no copy was found on one of +/// those tiers, even when `copies` is not empty -- a `rocm-install` or +/// `managed-runtime` hit is evidence a copy exists, not evidence the loader +/// would pick it. +/// +/// Applied identically to the code object manager and the HIP runtime: both +/// are searched by [`find_library_copies`], so both answer "which one would +/// load" by the same rule. See `a_search_dir_only_hit_is_not_selected_for_either_prefix` +/// for the test that pins this for both. +fn select_loader_copy(copies: &[LibraryCopy]) -> Option { + copies + .iter() + .find(|copy| LOADER_PATH_SOURCES.contains(©.source.as_str())) + .cloned() +} + +/// The note (if any) `probe_comgr` should push about `copies` found for +/// `prefix`, given which one (if any) was `selected`. +/// +/// Three cases: no copies at all; more than one copy with one of them +/// selected (the "would load" case); and one or more copies with none +/// selected, meaning every hit sits only in a `rocm-install` or +/// `managed-runtime` directory the loader does not consult on its own, so it +/// is unknown whether any of them loads. +fn copies_note( + prefix: &str, + copies: &[LibraryCopy], + selected: Option<&LibraryCopy>, +) -> Option { + if copies.is_empty() { + return Some(format!( + "no {prefix} found on the library path, in the loader cache, or in any known ROCm install" + )); + } + if let Some(selected) = selected { + return (copies.len() > 1).then(|| { + format!( + "{} copies of {prefix} found; {} would load", + copies.len(), + selected.path + ) + }); + } + let count = copies.len(); + let plural = if count == 1 { "copy" } else { "copies" }; + Some(format!( + "{count} {plural} of {prefix} found, but none on a path the loader consults -- only in \ + a ROCm install or a managed runtime, which the loader does not search on its own, so it \ + is unknown whether any of them loads" + )) } -/// Pure core of [`probe_comgr`]: given each source's evidence already gathered, -/// find every copy in loader search order and say which one would load, plus -/// the note (if any) `probe_comgr` should push. +/// Every copy of `prefix` on the machine, in loader search order, each attributed +/// to the installation that owns it. /// -/// Split out so the ordering -- the reason this entry exists -- can be pinned -/// with directories built in a test's own temp folder, instead of needing a -/// real `ldconfig`, a real `/opt`, or a real managed runtime to exercise. +/// `active_runtime_dirs` -- the active managed runtime's own library +/// directories, when there is one -- are checked **first**, ahead of +/// `ld_library_path`. That is not this process's search order; it is a served +/// child's. `rocm serve`/`rocm chat` prepend exactly those directories onto +/// `LD_LIBRARY_PATH` before launching the engine (see +/// `lemonade_process_environment_vars` and its vLLM counterpart), so they win +/// over whatever this process's own environment or the system loader cache +/// would otherwise resolve to. Reporting the plain-environment answer instead +/// would name the system's copy as "the one that would load" on a machine +/// where every process the CLI actually launches loads the wheel's. /// -/// Returns the full list of copies found, the one the loader would actually -/// pick (the first copy whose source is in [`LOADER_PATH_SOURCES`] -- `None` -/// when no copy was found on one of those tiers, even when the list is not -/// empty), and the note (if any) `probe_comgr` should push. -fn find_comgr_copies( +/// `ld_library_path`, `loader_cache_text`, and `dirs`/`dir_owners` are +/// gathered once by the caller and shared between the comgr and HIP searches, +/// rather than each reading the real environment, spawning `ldconfig`, and +/// walking every managed runtime's directories again -- neither depends on +/// which library is being searched for. That sharing is also what makes this +/// fully testable without a real `ldconfig`, a real `/opt`, or a real managed +/// runtime: every input is a plain value a test can construct. +fn find_library_copies( + prefix: &str, active_runtime_dirs: &[PathBuf], ld_library_path: &str, - loader_cache_paths: &[String], - search_dirs: &[(PathBuf, &'static str)], -) -> (Vec, Option, Option) { - let mut copies: Vec = Vec::new(); + loader_cache_text: Option<&str>, + dirs: &[(std::path::PathBuf, &'static str, std::path::PathBuf)], + dir_owners: &std::collections::HashMap, +) -> Vec { + let mut copies: Vec = Vec::new(); let mut seen: std::collections::BTreeSet = std::collections::BTreeSet::new(); // Order is the whole point: this is the order a served child's loader @@ -2276,52 +2473,42 @@ fn find_comgr_copies( // the one that wins. for dir in active_runtime_dirs { let mut hits = Vec::new(); - collect_libraries_in_dir(dir, COMGR_LIB_PREFIX, &mut hits); + collect_libraries_in_dir(dir, prefix, &mut hits); for path in hits { - record_comgr_copy(&mut copies, &mut seen, &path, "active-runtime"); + record_library_copy(&mut copies, &mut seen, &path, "active-runtime", dir_owners); } } - for path in libraries_on_ld_path(ld_library_path, COMGR_LIB_PREFIX) { - record_comgr_copy(&mut copies, &mut seen, &path, "ld-library-path"); + for path in libraries_on_ld_path(ld_library_path, prefix) { + record_library_copy(&mut copies, &mut seen, &path, "ld-library-path", dir_owners); } - for path in loader_cache_paths { - record_comgr_copy(&mut copies, &mut seen, path, "loader-cache"); + if let Some(text) = loader_cache_text { + for path in parse_ldconfig_cache_paths(text, prefix) { + record_library_copy(&mut copies, &mut seen, &path, "loader-cache", dir_owners); + } } - for (dir, source) in search_dirs { + for (dir, source, _root) in dirs { let mut hits = Vec::new(); - collect_libraries_in_dir(dir, COMGR_LIB_PREFIX, &mut hits); + collect_libraries_in_dir(dir, prefix, &mut hits); for path in hits { - record_comgr_copy(&mut copies, &mut seen, &path, source); + record_library_copy(&mut copies, &mut seen, &path, source, dir_owners); } } + copies +} - let selected = copies - .iter() - .find(|copy| LOADER_PATH_SOURCES.contains(©.source.as_str())) - .cloned(); - - let note = if copies.is_empty() { - Some(format!( - "no {COMGR_LIB_PREFIX} found on the library path, in the loader cache, or in any known ROCm install" - )) - } else if let Some(selected) = &selected { - (copies.len() > 1).then(|| { - format!( - "{} copies of {COMGR_LIB_PREFIX} found; {} would load", - copies.len(), - selected.path - ) - }) - } else { - let count = copies.len(); - let plural = if count == 1 { "copy" } else { "copies" }; - Some(format!( - "{count} {plural} of {COMGR_LIB_PREFIX} found, but none on a path the loader \ - consults -- only in a ROCm install or a managed runtime, which the loader does not \ - search on its own, so it is unknown whether any of them loads" - )) - }; - (copies, selected, note) +/// The installation that owns `real_path`: whichever root's directory holds +/// it, per `dir_owners`. Empty when no known installation's directory holds +/// it -- a library somewhere unexpected is reported as belonging nowhere +/// rather than guessed into an install it is not part of. +fn owning_install_root( + real_path: &str, + dir_owners: &std::collections::HashMap, +) -> String { + std::path::Path::new(real_path) + .parent() + .and_then(|dir| dir_owners.get(dir)) + .map(|root| root.to_string_lossy().into_owned()) + .unwrap_or_default() } /// Record `path` unless an earlier entry already resolved to the same file. @@ -2329,11 +2516,12 @@ fn find_comgr_copies( /// Deduplicated on the resolved path, not the given one: ROCm ships a versioned /// library and an unversioned symlink beside it, and reporting those as two /// copies would invent a conflict on a perfectly ordinary install. -fn record_comgr_copy( - copies: &mut Vec, +fn record_library_copy( + copies: &mut Vec, seen: &mut std::collections::BTreeSet, path: &str, source: &str, + dir_owners: &std::collections::HashMap, ) { // A path that cannot be resolved is kept as given rather than dropped: a // dangling symlink is still something the loader would try, and reporting @@ -2345,8 +2533,11 @@ fn record_comgr_copy( if !seen.insert(real_path.clone()) { return; } - copies.push(ComgrCopy { + copies.push(LibraryCopy { version: comgr_version_from_file_name(&real_path), + // Attributed by the resolved path: a symlink from one installation into + // another's file belongs to the installation holding the file. + install_root: owning_install_root(&real_path, dir_owners), path: path.to_owned(), real_path, source: source.to_owned(), @@ -2379,31 +2570,22 @@ fn comgr_version_from_file_name(real_path: &str) -> String { } } -/// Paths the loader cache knows for the library. +/// Parse `ldconfig -p`'s own output (`crate::ldconfig_cache()`) for every path +/// it lists against `prefix`. /// /// `ldconfig -p` prints `libamd_comgr.so.2 (libc6,x86-64) => /opt/rocm/lib/...`. -/// Its absence, or a nonzero exit, contributes nothing rather than failing the -/// probe: plenty of hosts have no `ldconfig` on the path, and a machine with no -/// loader cache is still a machine worth describing. -fn comgr_paths_in_loader_cache() -> Vec { - // `which("ldconfig")` used to gate this, but `ldconfig` lives in `/sbin`, - // off a non-root user's `PATH` on Debian and derivatives — `which` alone - // would silently read a working install as having no loader-cache entry. - // `crate::ldconfig_cache` already searches the conventional locations for - // exactly this reason; reuse it instead of re-introducing the bug it was - // written to fix. - let Some(out) = crate::ldconfig_cache() else { - return Vec::new(); - }; - parse_ldconfig_cache_paths(&out, COMGR_LIB_PREFIX) -} - -/// Parse `ldconfig -p`'s own output for every path it lists against `prefix`. +/// A missing cache, or a nonzero exit from `ldconfig` itself, contributes +/// nothing rather than failing the probe: plenty of hosts have no `ldconfig` +/// on the path, and a machine with no loader cache is still a machine worth +/// describing -- `probe_comgr` reads `crate::ldconfig_cache()` once and passes +/// `None` straight through for exactly that case. /// -/// Split out of [`comgr_paths_in_loader_cache`] so the line format -- a fixed -/// property of `ldconfig`, not of this host -- can be pinned against literal -/// text instead of a real `ldconfig` binary, which not every machine running -/// this test has on `PATH`. +/// Takes the already-read text rather than calling `crate::ldconfig_cache()` +/// itself, so the line format -- a fixed property of `ldconfig`, not of this +/// host -- can be pinned against literal text instead of a real `ldconfig` +/// binary, which not every machine running this test has on `PATH`; it also +/// means `probe_comgr` reads the cache once and parses it twice (comgr, HIP) +/// rather than spawning `ldconfig -p` twice for one invocation. fn parse_ldconfig_cache_paths(cache_text: &str, prefix: &str) -> Vec { cache_text .lines() @@ -2436,76 +2618,111 @@ fn rocm_install_siblings(opt_dir: &std::path::Path) -> Vec { roots } -/// Directories to search after the library path and the loader cache, each -/// paired with the source label it is recorded under. -fn comgr_search_dirs(e: &Examination) -> Vec<(std::path::PathBuf, &'static str)> { - comgr_search_dirs_in( - &e.rocm_path, - std::path::Path::new("/opt"), - &managed_runtime_roots(), - ) +/// Every ROCm installation on the machine, each paired with the source label its +/// libraries are recorded under. +/// +/// A managed runtime is **one** root even though its libraries are spread across +/// several `_rocm_sdk_*` directories. That is what lets the diagnosis tell a +/// mixed environment from a healthy managed one: split those directories into +/// separate installations and every healthy managed install looks like a +/// conflict. +fn known_install_roots(e: &Examination) -> Vec<(std::path::PathBuf, &'static str)> { + let mut roots: Vec<(std::path::PathBuf, &'static str)> = Vec::new(); + if !e.rocm_path.is_empty() { + roots.push((std::path::PathBuf::from(&e.rocm_path), "rocm-install")); + } + // Sibling ROCm installs the active one does not cover. A versioned install + // left behind by an upgrade is one of the two copies this entry exists to + // find. + roots.extend( + rocm_install_siblings(std::path::Path::new("/opt")) + .into_iter() + .map(|root| (root, "rocm-install")), + ); + // Only the root is carried here; `install_library_dirs` looks the recorded + // `library_paths` back up by root when it needs it, since this tuple's + // shape is shared with `rocm-install` roots that have no such thing. + roots.extend( + managed_runtime_roots() + .into_iter() + .map(|(root, _library_paths)| (root, "managed-runtime")), + ); + dedup_roots_keeping_first(roots) } -/// Pure core of [`comgr_search_dirs`]: given the rocm-install path, the `/opt` -/// directory to scan for sibling installs, and the managed runtimes already -/// resolved, build the search-dir list. +/// Drop repeats of a root already seen, keeping the first occurrence. /// -/// Split out so the three tiers -- a configured `rocm_path`, `/opt` siblings, -/// and managed runtimes -- can each be pinned with a test's own temp folder -/// and its own runtime list, instead of needing the real `/opt` or a real -/// runtime registry on the machine running the suite. -fn comgr_search_dirs_in( - rocm_path: &str, - opt_dir: &std::path::Path, - managed_runtimes: &[(std::path::PathBuf, Option)], +/// Order is load-bearing: it is the order the loader would search, and +/// `install_library_dirs` attributes by exact directory membership, so a root +/// recorded twice would walk (and attribute to two distinct, textually-equal) +/// copies of the same installation's directories. +fn dedup_roots_keeping_first( + roots: Vec<(std::path::PathBuf, &'static str)>, ) -> Vec<(std::path::PathBuf, &'static str)> { - let mut dirs: Vec<(std::path::PathBuf, &'static str)> = Vec::new(); - let mut add = |dir: std::path::PathBuf, source: &'static str| { - if dir.is_dir() { - dirs.push((dir, source)); - } - }; - - if !rocm_path.is_empty() { - let root = std::path::PathBuf::from(rocm_path); - add(root.join("lib"), "rocm-install"); - add(root.join("lib64"), "rocm-install"); - } - for root in rocm_install_siblings(opt_dir) { - add(root.join("lib"), "rocm-install"); - add(root.join("lib64"), "rocm-install"); - } - // The copy this CLI installs itself. Reusing the layout the SDK probe - // already knows rather than restating it: a second description of where a - // managed runtime keeps its libraries is a second thing to keep correct. - for path in managed_comgr_dirs(managed_runtimes) { - add(path, "managed-runtime"); - } - dirs + let mut seen = std::collections::HashSet::new(); + roots + .into_iter() + .filter(|(root, _)| seen.insert(root.clone())) + .collect() } -/// Library directories of every managed runtime, given each runtime's root and -/// the `site-packages` its SDK recorded. +/// The library directories of every known installation, in root order, each +/// paired with the source label and the root that owns it. /// -/// Shares `collect_managed_runtime_library_paths` with the loader path the CLI -/// sets for processes it starts, so the search looks exactly where a managed -/// runtime's libraries are actually loaded from. -fn managed_comgr_dirs( - runtimes: &[(std::path::PathBuf, Option)], -) -> Vec { - let mut paths = Vec::new(); - for (root, site_packages) in runtimes { - crate::collect_managed_runtime_library_paths(root, site_packages.as_deref(), &mut paths); - } - paths +/// Reuses the layout the SDK probe already knows rather than restating it: a +/// second description of where an installation keeps its libraries is a second +/// thing to keep correct. +/// +/// The owning root travels with each directory because a managed runtime's +/// directories are not all nested under its root (see the comment on `root` +/// below) -- a caller attributing a found file by string-prefix match against +/// the bare root would find nothing for exactly the directories this function +/// exists to add. Keying attribution off the same directories this search +/// walks, instead, is what `find_library_copies` does with the return value. +fn install_library_dirs( + roots: &[(std::path::PathBuf, &'static str)], +) -> Vec<(std::path::PathBuf, &'static str, std::path::PathBuf)> { + // For a managed runtime, `root` is the `_rocm_sdk_devel` package directory + // the SDK probe resolved (see `managed_runtime_roots`), not a venv root -- + // it has no `lib//site-packages` of its own to walk, and guessing + // one there finds nothing. The probe already recorded where the runtime's + // libraries actually are: `library_paths` is built by importing each real + // package ("core", "libraries", "device", "profiler") and asking Python for + // its file location (`ROCM_SDK_PROBE_SCRIPT`), the same value + // `probe_runtime_devices` puts on `LD_LIBRARY_PATH` for a served process. + // Prefer that recorded truth over re-deriving the layout, which is the bug + // `examine-finds-the-managed-runtimes-own-compilation-library` catches on a + // real install: the guess never matches, so the runtime's own code object + // manager library goes unseen. + let managed_library_paths: std::collections::HashMap< + std::path::PathBuf, + Vec, + > = managed_runtime_roots().into_iter().collect(); + let mut dirs = Vec::new(); + for (root, source) in roots { + let mut paths = Vec::new(); + crate::collect_sdk_library_paths(root, &mut paths); + if *source == "managed-runtime" + && let Some(recorded) = managed_library_paths.get(root) + { + paths.extend(recorded.iter().cloned()); + } + dirs.extend( + paths + .into_iter() + .filter(|path| path.is_dir()) + .map(|path| (path, *source, root.clone())), + ); + } + dirs } -/// Every managed runtime, as `(root, site-packages)`. +/// Every managed runtime, as `(root, library_paths)`. /// /// Read from the runtime registry rather than by listing `/runtimes` on /// disk. A directory listing yields a root and nothing else, and the root alone /// does not locate a wheel runtime's libraries -- only the SDK record knows -/// which `site-packages` its interpreter uses. Listing the directory is also a +/// where its packages actually resolved to. Listing the directory is also a /// second answer to "which runtimes exist", and the registry is the first one. /// /// Resolved through [`crate::AppPaths::discover`], not @@ -2513,16 +2730,18 @@ fn managed_comgr_dirs( /// `$HOME/.rocm` and the OS default, so it disagrees with the rest of the CLI /// -- and finds nothing at all -- on any host where `ROCM_CLI_DATA_DIR` /// relocates the data directory, which is exactly what every GPU e2e scenario -/// does for isolation. -fn managed_runtime_roots() -> Vec<(std::path::PathBuf, Option)> { +/// does for isolation. That mismatch, not the library-directory guess this +/// search used to make, is why a managed runtime's own libraries were +/// unreachable end to end. +fn managed_runtime_roots() -> Vec<(std::path::PathBuf, Vec)> { let Ok(paths) = crate::AppPaths::discover() else { return Vec::new(); }; let registry = paths.data_dir.join("runtimes").join("registry"); - let mut runtimes: Vec<(std::path::PathBuf, Option)> = + let mut runtimes: Vec<(std::path::PathBuf, Vec)> = crate::managed_therock_sdk_probe_candidates(®istry) .into_iter() - .map(|candidate| (candidate.root_path, candidate.site_packages)) + .map(|candidate| (candidate.root_path, candidate.library_paths)) .collect(); runtimes.sort(); runtimes @@ -3257,8 +3476,20 @@ mod tests { let mut copies = Vec::new(); let mut seen = std::collections::BTreeSet::new(); - record_comgr_copy(&mut copies, &mut seen, &link.to_string_lossy(), "test"); - record_comgr_copy(&mut copies, &mut seen, &real.to_string_lossy(), "test"); + record_library_copy( + &mut copies, + &mut seen, + &link.to_string_lossy(), + "test", + &std::collections::HashMap::new(), + ); + record_library_copy( + &mut copies, + &mut seen, + &real.to_string_lossy(), + "test", + &std::collections::HashMap::new(), + ); assert_eq!( copies.len(), @@ -3282,13 +3513,79 @@ mod tests { let mut copies = Vec::new(); let mut seen = std::collections::BTreeSet::new(); - record_comgr_copy(&mut copies, &mut seen, &a.to_string_lossy(), "test"); - record_comgr_copy(&mut copies, &mut seen, &b.to_string_lossy(), "test"); + record_library_copy( + &mut copies, + &mut seen, + &a.to_string_lossy(), + "test", + &std::collections::HashMap::new(), + ); + record_library_copy( + &mut copies, + &mut seen, + &b.to_string_lossy(), + "test", + &std::collections::HashMap::new(), + ); assert_eq!(copies.len(), 2, "two files are two copies: {copies:?}"); std::fs::remove_dir_all(&root).ok(); } + #[test] + fn a_library_deep_inside_a_managed_runtime_belongs_to_the_runtime() { + // The property the whole diagnosis rests on. A managed runtime spreads + // its libraries across separate `_rocm_sdk_*` packages that sit + // *beside* each other under one `site-packages`, not nested under the + // runtime's own root (`_rocm_sdk_devel`, what the SDK probe records as + // `root_path`) at all -- a venv-shaped fixture nesting + // `_rocm_sdk_core` under the runtime's root would pass by construction + // without proving anything about the real layout, which is exactly the + // shape that let this gap through review once already. Attribution + // instead keys on the directories `install_library_dirs` actually + // recorded for the runtime, which is what `dir_owners` below stands + // in for. + let runtime = std::path::PathBuf::from("/data/runtimes/therock/_rocm_sdk_devel"); + let mut dir_owners = std::collections::HashMap::new(); + for package in ["_rocm_sdk_core", "_rocm_sdk_devel"] { + let lib_dir = std::path::PathBuf::from(format!("/data/runtimes/therock/{package}/lib")); + dir_owners.insert(lib_dir, runtime.clone()); + } + + for package in ["_rocm_sdk_core", "_rocm_sdk_devel"] { + let path = format!("/data/runtimes/therock/{package}/lib/libamd_comgr.so.2"); + assert_eq!( + owning_install_root(&path, &dir_owners), + runtime.to_string_lossy(), + "{package} belongs to the runtime that recorded its directory, not to itself" + ); + } + } + + #[test] + fn a_library_no_installation_claims_is_attributed_to_none() { + // Empty rather than guessed. A library somewhere unexpected is a gap in + // what the search knows, and inventing an owner for it would turn that + // gap into a false finding about the user's machine. + let mut dir_owners = std::collections::HashMap::new(); + dir_owners.insert( + std::path::PathBuf::from("/opt/rocm/lib"), + std::path::PathBuf::from("/opt/rocm"), + ); + assert_eq!( + owning_install_root("/somewhere/else/libamd_comgr.so.2", &dir_owners), + "" + ); + // A sibling whose name merely starts the same way is not a parent -- + // and is not even a near miss here: attribution keys on the exact + // directory recorded for each root, not a string-prefix match, so + // `/opt/rocm-other/lib` is simply a different key from `/opt/rocm/lib`. + assert_eq!( + owning_install_root("/opt/rocm-other/lib/x.so", &dir_owners), + "" + ); + } + #[test] fn a_version_is_read_only_when_the_name_actually_carries_one() { for (name, expected) in [ @@ -3390,6 +3687,12 @@ mod tests { "comgr_paths", "comgr_selected", "comgr_version", + "comgr_matches_runtime", + // The HIP runtime is searched the same way and for the same reason: + // deciding whether the code object manager belongs to the active + // runtime means knowing which runtime is active. + "hip_paths", + "hip_selected", "hip_sdk_path", "hip_sdk_version", "hipinfo_present", @@ -3486,6 +3789,52 @@ mod tests { #[cfg(unix)] const RUNTIME_TORCH_OK_JSON: &str = r#"{"ok":true,"version":"2.11.0+rocm7.14.1","hip":"7.14.60850","cuda":null,"is_available":true,"device_count":1,"arch_list":["gfx942"]}"#; + /// One installation is never counted twice, wherever it appears in the list. + /// + /// `dedup_by` only drops *consecutive* duplicates, and these roots arrive + /// from three independent sources: the active install, a sorted `/opt` + /// scan, and the managed runtimes. The active install collides with a + /// sibling whenever it is one, and whether the two land adjacent depends on + /// where the path happens to sort -- so the bug was invisible for exactly + /// the inputs where the sort was kind. + /// + /// A duplicated root is not cosmetic: attribution is by longest known root, + /// so the same installation appearing twice can make a machine holding one + /// stack look like a machine holding two, which is the false conflict this + /// entry exists to avoid reporting. + #[test] + fn one_installation_is_never_counted_twice_however_the_paths_sort() { + use std::path::PathBuf; + let collide = PathBuf::from("/opt/rocm-7.1"); + let roots = dedup_roots_keeping_first(vec![ + (collide.clone(), "rocm-install"), + // A sibling that sorts *before* the collision, so the duplicate is + // not adjacent to it. This ordering is what the old dedup missed. + (PathBuf::from("/opt/rocm-6.4"), "rocm-install"), + (collide.clone(), "rocm-install"), + ( + PathBuf::from("/home/u/.local/share/rocm-cli/runtimes/wheel/a"), + "managed-runtime", + ), + ]); + + let seen = roots.iter().filter(|(p, _)| *p == collide).count(); + assert_eq!( + seen, 1, + "one installation was recorded twice, so a single stack can be read as two: {roots:?}" + ); + // Non-vacuity: deduplicating must not be achieved by dropping roots. + assert_eq!( + roots.len(), + 3, + "every distinct root has to survive, in first-seen order: {roots:?}" + ); + assert_eq!( + roots[0].0, collide, + "first-seen order is loader search order" + ); + } + /// A wheel-format managed runtime, laid out the way the CLI installs one. /// /// `root` is one of the runtime's own `_rocm_sdk_*` package directories -- @@ -3527,7 +3876,8 @@ mod tests { #[test] fn the_managed_copy_this_cli_installs_is_reachable() { let (root, site_packages) = wheel_runtime_on_disk("found"); - let dirs = managed_comgr_dirs(&[(root.clone(), site_packages.clone())]); + let mut dirs = Vec::new(); + crate::collect_managed_runtime_library_paths(&root, site_packages.as_deref(), &mut dirs); let expected = site_packages .expect("fixture always records site_packages") @@ -3547,7 +3897,7 @@ mod tests { /// A direct call with `None`, not a path any real registry record takes /// (every real candidate's `site_packages` is recorded unconditionally, /// so `collect_managed_runtime_library_paths` never actually receives - /// `None` from `managed_comgr_dirs`'s real caller). What this pins is the + /// `None` from `managed_runtime_roots`'s real caller). What this pins is the /// `None` arm's own contract for any caller that does pass it directly: a /// plain install still contributes what sits directly under its root, /// with no sibling packages to go looking for. @@ -3563,7 +3913,8 @@ mod tests { let _ = fs::remove_dir_all(&root); fs::create_dir_all(root.join("lib")).unwrap(); - let dirs = managed_comgr_dirs(&[(root.clone(), None)]); + let mut dirs = Vec::new(); + crate::collect_managed_runtime_library_paths(&root, None, &mut dirs); assert!( dirs.contains(&root.join("lib")), "a root-format runtime keeps its libraries under the root, and that has to keep \ @@ -3572,7 +3923,7 @@ mod tests { let _ = fs::remove_dir_all(&root); } - /// A directory holding a `libamd_comgr` file, for [`find_comgr_copies`] + /// A directory holding a `libamd_comgr` file, for [`find_library_copies`] /// tests below. Each call gets its own temp directory so the tests can run /// concurrently without seeing each other's files. /// @@ -3593,16 +3944,49 @@ mod tests { dir } + /// Like [`comgr_copy_dir`], but for any `prefix` -- used by the tests that + /// pin a rule shared by comgr and the HIP runtime, where the file name + /// has to match whichever prefix is under test. + /// + /// `#[cfg(unix)]` for the same reason as `comgr_copy_dir`. + #[cfg(unix)] + fn library_copy_dir(prefix: &str, tag: &str) -> std::path::PathBuf { + let dir = std::env::temp_dir().join(format!( + "rocm-library-copy-{tag}-{}-{:?}", + std::process::id(), + std::thread::current().id() + )); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write(dir.join(format!("{prefix}.so.2")), b"fake").unwrap(); + dir + } + + /// A synthetic `ldconfig -p` line naming `path`, for the `loader_cache_text` + /// parameter below -- so these tests pin the ordering without depending on + /// a real `ldconfig` or a real loader cache on the machine running them. + /// Unix-only, because every caller is: the loader cache is a POSIX + /// concept and the tests that build one are gated the same way. Without + /// this the helper is dead code on Windows, and CI compiles with + /// `-D warnings`, so a dead helper is a hard error rather than a warning. + #[cfg(unix)] + fn loader_cache_line_for(path: &std::path::Path) -> String { + format!( + "libamd_comgr.so.2 (libc6,x86-64) => {}", + path.join("libamd_comgr.so.2").display() + ) + } + /// The active managed runtime's own directory wins selection even when a /// copy also sits on the plain `LD_LIBRARY_PATH` and in the loader cache. /// - /// This is the fix for the case `probe_comgr`'s own doc comment describes: - /// `rocm serve`/`rocm chat` prepend the active runtime's library - /// directories onto `LD_LIBRARY_PATH` before launching the engine, so that - /// copy is the one a served process actually loads -- regardless of what - /// this process's own environment or the system loader cache would - /// otherwise resolve to. Before this fix, `find_comgr_copies` had no - /// `active_runtime_dirs` parameter at all, and a system copy reachable + /// This is the fix for the case `find_library_copies`'s own doc comment + /// describes: `rocm serve`/`rocm chat` prepend the active runtime's + /// library directories onto `LD_LIBRARY_PATH` before launching the engine, + /// so that copy is the one a served process actually loads -- regardless + /// of what this process's own environment or the system loader cache + /// would otherwise resolve to. Before this fix, `find_library_copies` had + /// no `active_runtime_dirs` parameter at all, and a system copy reachable /// through the loader cache was reported as "would load" even on a host /// where every process the CLI actually launches loads the wheel's copy. /// @@ -3615,16 +3999,17 @@ mod tests { let active = comgr_copy_dir("active"); let ambient = comgr_copy_dir("ambient"); let cached = comgr_copy_dir("cached"); + let loader_cache_text = loader_cache_line_for(&cached); - let (copies, selected, _note) = find_comgr_copies( + let copies = find_library_copies( + COMGR_LIB_PREFIX, std::slice::from_ref(&active), &ambient.to_string_lossy(), - &[cached - .join("libamd_comgr.so.2") - .to_string_lossy() - .into_owned()], + Some(&loader_cache_text), &[], + &std::collections::HashMap::new(), ); + let selected = select_loader_copy(&copies); assert_eq!( copies.first().map(|copy| ©.source), @@ -3650,7 +4035,7 @@ mod tests { /// With no active-runtime evidence, the plain `LD_LIBRARY_PATH` still wins /// over the loader cache and the trailing search directories -- the - /// ordering `find_comgr_copies`'s doc comment says is "the whole point". + /// ordering `find_library_copies`'s doc comment says is "the whole point". /// /// Unix-only: same `:`-split reason as above. #[cfg(unix)] @@ -3659,16 +4044,17 @@ mod tests { let ld = comgr_copy_dir("ld"); let cached = comgr_copy_dir("cached2"); let known = comgr_copy_dir("known"); + let loader_cache_text = loader_cache_line_for(&cached); - let (copies, selected, _note) = find_comgr_copies( + let copies = find_library_copies( + COMGR_LIB_PREFIX, &[], &ld.to_string_lossy(), - &[cached - .join("libamd_comgr.so.2") - .to_string_lossy() - .into_owned()], - &[(known.clone(), "rocm-install")], + Some(&loader_cache_text), + &[(known.clone(), "rocm-install", known.clone())], + &std::collections::HashMap::new(), ); + let selected = select_loader_copy(&copies); assert_eq!( copies.first().map(|copy| ©.source), @@ -3704,11 +4090,13 @@ mod tests { fn an_active_runtime_directory_that_is_also_a_managed_root_keeps_one_label() { let dir = comgr_copy_dir("overlap"); - let (copies, _selected, _note) = find_comgr_copies( + let copies = find_library_copies( + COMGR_LIB_PREFIX, std::slice::from_ref(&dir), "", - &[], - &[(dir.clone(), "managed-runtime")], + None, + &[(dir.clone(), "managed-runtime", dir.clone())], + &std::collections::HashMap::new(), ); assert_eq!( @@ -3724,32 +4112,69 @@ mod tests { let _ = std::fs::remove_dir_all(&dir); } + /// `find_library_copies` with no evidence anywhere reports no copies -- + /// `probe_comgr` is what turns that into the "no ... found" note, since it + /// alone has the prefix's constant describing where it looked. + #[test] + fn no_copies_anywhere_is_an_empty_report() { + assert!( + find_library_copies( + COMGR_LIB_PREFIX, + &[], + "", + None, + &[], + &std::collections::HashMap::new(), + ) + .is_empty() + ); + } + /// More than one copy produces the "N copies ... would load" note; exactly - /// one copy produces no note at all. + /// one copy, or none, produces no note at all. /// /// Unix-only: same `:`-split reason as above. #[cfg(unix)] #[test] fn the_multi_copy_note_only_fires_past_one_copy() { let only = comgr_copy_dir("only"); - let (one_copy, _selected, note) = find_comgr_copies(&[], &only.to_string_lossy(), &[], &[]); + let one_copy = find_library_copies( + COMGR_LIB_PREFIX, + &[], + &only.to_string_lossy(), + None, + &[], + &std::collections::HashMap::new(), + ); + let one_selected = select_loader_copy(&one_copy); assert_eq!(one_copy.len(), 1); assert_eq!( - note, None, + copies_note(COMGR_LIB_PREFIX, &one_copy, one_selected.as_ref()), + None, "a single copy must not be reported as a conflict: {one_copy:?}" ); + assert_eq!( + copies_note(COMGR_LIB_PREFIX, &[], None), + Some(format!( + "no {COMGR_LIB_PREFIX} found on the library path, in the loader cache, or in any \ + known ROCm install" + )), + "no copies is not a multi-copy conflict either, but it is still worth a note" + ); let second = comgr_copy_dir("second"); - let (two_copies, _selected, note) = find_comgr_copies( + let loader_cache_text = loader_cache_line_for(&second); + let two_copies = find_library_copies( + COMGR_LIB_PREFIX, &[], &only.to_string_lossy(), - &[second - .join("libamd_comgr.so.2") - .to_string_lossy() - .into_owned()], + Some(&loader_cache_text), &[], + &std::collections::HashMap::new(), ); - let note = note.expect("more than one copy must be noted"); + let two_selected = select_loader_copy(&two_copies); + let note = copies_note(COMGR_LIB_PREFIX, &two_copies, two_selected.as_ref()) + .expect("more than one copy must be noted"); assert!( note.contains("2 copies"), "the note must say how many copies were found: {note:?}" @@ -3767,54 +4192,79 @@ mod tests { /// every place that was searched. #[test] fn no_copies_anywhere_names_every_place_searched() { - let (copies, selected, note) = find_comgr_copies(&[], "", &[], &[]); + let copies = find_library_copies( + COMGR_LIB_PREFIX, + &[], + "", + None, + &[], + &std::collections::HashMap::new(), + ); + let selected = select_loader_copy(&copies); assert!(copies.is_empty()); assert!(selected.is_none()); - let note = note.expect("an empty search must still explain itself"); + let note = copies_note(COMGR_LIB_PREFIX, &copies, selected.as_ref()) + .expect("an empty search must still explain itself"); assert!(note.contains("library path"), "{note:?}"); assert!(note.contains("loader cache"), "{note:?}"); assert!(note.contains("ROCm install"), "{note:?}"); } /// A copy found only in a search directory -- `rocm-install` or - /// `managed-runtime` -- is reported in `comgr_paths`, but is *not* selected - /// as the one that would load, because nothing puts a search-dir hit on a - /// path the loader actually consults on its own. + /// `managed-runtime` -- is reported among the copies, but is *not* + /// selected as the one that would load, because nothing puts a + /// search-dir hit on a path the loader actually consults on its own. /// /// This pins the fix for exactly the bug this entry exists to catch: a /// leftover `/opt/rocm-*` install, or a managed runtime that is not the /// active one, used to be reported as "the library that loads" purely /// because it was `copies.first()` -- regardless of which tier the hit - /// came from. A machine whose only comgr copy sits in an inactive install - /// has no comgr on the loader's path at all, and the report must say so, + /// came from. A machine whose only copy sits in an inactive install has + /// no library on the loader's path at all, and the report must say so, /// not point at a file nothing would ever load. /// + /// Run once for comgr and once for the HIP runtime: both are searched by + /// the same [`find_library_copies`] and both are selected by the same + /// [`select_loader_copy`], so the rule is pinned for each independently + /// rather than only for whichever one happened to have the bug first. + /// /// Unix-only: same `:`-split reason as the neighbouring LD-path tests. #[cfg(unix)] #[test] - fn a_search_dir_only_hit_is_not_reported_as_the_one_that_would_load() { - let leftover = comgr_copy_dir("leftover-install"); - - let (copies, selected, note) = - find_comgr_copies(&[], "", &[], &[(leftover.clone(), "rocm-install")]); + fn a_search_dir_only_hit_is_not_selected_for_either_prefix() { + for prefix in [COMGR_LIB_PREFIX, HIP_RUNTIME_LIB_PREFIX] { + let leftover = library_copy_dir(prefix, &format!("leftover-install-{prefix}")); + + let copies = find_library_copies( + prefix, + &[], + "", + None, + &[(leftover.clone(), "rocm-install", leftover.clone())], + &std::collections::HashMap::new(), + ); + let selected = select_loader_copy(&copies); - assert_eq!( - copies.len(), - 1, - "the leftover copy must still be reported: {copies:?}" - ); - assert_eq!(copies[0].source, "rocm-install"); - assert!( - selected.is_none(), - "a search-dir-only hit must not be named as the one that would load: {selected:?}" - ); - let note = note.expect("a copy that cannot load still needs an explanation"); - assert!( - !note.contains("would load"), - "the note must not claim a search-dir-only copy would load: {note:?}" - ); + assert_eq!( + copies.len(), + 1, + "the leftover copy must still be reported for {prefix}: {copies:?}" + ); + assert_eq!(copies[0].source, "rocm-install"); + assert!( + selected.is_none(), + "a search-dir-only hit must not be named as the one that would load for \ + {prefix}: {selected:?}" + ); + let note = copies_note(prefix, &copies, selected.as_ref()) + .expect("a copy that cannot load still needs an explanation"); + assert!( + !note.contains("would load"), + "the note must not claim a search-dir-only copy would load for {prefix}: {note:?}" + ); - let _ = std::fs::remove_dir_all(&leftover); + let _ = std::fs::remove_dir_all(&leftover); + } } /// The loader cache outranks a search directory, independent of any @@ -3826,22 +4276,22 @@ mod tests { /// runs first in the implementation -- so that test cannot tell the two /// loops apart. This test leaves `LD_LIBRARY_PATH` and the active-runtime /// dirs empty, so only the loader-cache-versus-search-dir order is in - /// play: swapping those two loops in `find_comgr_copies` leaves every + /// play: swapping those two loops in `find_library_copies` leaves every /// other existing test green and only this one fails. #[cfg(unix)] #[test] fn loader_cache_outranks_search_dirs() { let cached = comgr_copy_dir("cache-wins"); let searched = comgr_copy_dir("search-dir-loses"); + let loader_cache_text = loader_cache_line_for(&cached); - let (copies, _selected, _note) = find_comgr_copies( + let copies = find_library_copies( + COMGR_LIB_PREFIX, &[], "", - &[cached - .join("libamd_comgr.so.2") - .to_string_lossy() - .into_owned()], - &[(searched.clone(), "rocm-install")], + Some(&loader_cache_text), + &[(searched.clone(), "rocm-install", searched.clone())], + &std::collections::HashMap::new(), ); assert_eq!( @@ -3855,57 +4305,6 @@ mod tests { let _ = std::fs::remove_dir_all(&searched); } - /// `comgr_search_dirs_in` labels each of its three tiers correctly: a - /// configured `rocm_path`, a sibling install under `/opt`, and a managed - /// runtime's own library directory -- each pointed at a test's own temp - /// folder rather than the real `/opt` or a real runtime registry. - #[cfg(target_os = "linux")] - #[test] - fn comgr_search_dirs_labels_each_tier() { - use std::fs; - let base = std::env::temp_dir().join(format!( - "rocm-comgr-search-dirs-{}-{:?}", - std::process::id(), - std::thread::current().id() - )); - let _ = fs::remove_dir_all(&base); - - let rocm_path = base.join("rocm-install"); - fs::create_dir_all(rocm_path.join("lib")).unwrap(); - - let opt_dir = base.join("opt"); - let sibling = opt_dir.join("rocm-5.7"); - fs::create_dir_all(sibling.join("lib")).unwrap(); - - let (runtime_root, site_packages) = wheel_runtime_on_disk("search-dirs"); - - let dirs = comgr_search_dirs_in( - &rocm_path.to_string_lossy(), - &opt_dir, - &[(runtime_root.clone(), site_packages.clone())], - ); - - assert!( - dirs.contains(&(rocm_path.join("lib"), "rocm-install")), - "the configured rocm_path must be labelled rocm-install: {dirs:?}" - ); - assert!( - dirs.contains(&(sibling.join("lib"), "rocm-install")), - "a sibling /opt install must also be labelled rocm-install: {dirs:?}" - ); - let managed_lib = site_packages - .expect("fixture always records site_packages") - .join("_rocm_sdk_core") - .join("lib"); - assert!( - dirs.contains(&(managed_lib, "managed-runtime")), - "a managed runtime's library directory must be labelled managed-runtime: {dirs:?}" - ); - - let _ = fs::remove_dir_all(&base); - let _ = fs::remove_dir_all(runtime_root.parent().and_then(|p| p.parent()).unwrap()); - } - /// `parse_ldconfig_cache_paths` reads the fixed `ldconfig -p` line format /// without needing a real `ldconfig` on the machine running the test. #[test] diff --git a/crates/rocm-core/src/fix.rs b/crates/rocm-core/src/fix.rs index dc056566f..2171147ee 100644 --- a/crates/rocm-core/src/fix.rs +++ b/crates/rocm-core/src/fix.rs @@ -22,6 +22,13 @@ use std::time::Duration; const RUN_TIMEOUT: Duration = Duration::from_mins(1); const QUERY_TIMEOUT: Duration = Duration::from_secs(8); +/// Shared verbatim by this recipe's own note and `diagnose.rs`'s +/// `check_18_comgr_conflict` evidence, which states the same claim in its own +/// words at the point the real paths are known. A second statement of one +/// claim is a second thing to keep correct, and the two had already drifted +/// apart in wording before they shared this constant. +pub(crate) const COMGR_CONFLICT_NEITHER_OPTION_RECOMMENDED: &str = "Neither option is recommended over the other: which is right depends on which stack you mean to keep, and removing the wrong one breaks a working environment."; + /// Print a failure explanation to stderr, ignoring write failures (closed /// stderr, full disk) so an I/O error while explaining a failure can't itself /// panic the process. @@ -585,6 +592,39 @@ const RECIPES: &[FixRecipe] = &[ applies_on: WSL_ONLY, runner: None, }, + FixRecipe { + fix_id: "fix-18-comgr-conflict", + title: "Code object manager library does not belong to the active HIP runtime", + rationale: "HIP compiles device code at run time through libamd_comgr, and this machine holds more than one copy of it. The copy the loader picks belongs to a different installation than the HIP runtime that loads, so compilation fails with an error that names neither the library nor the second copy. A second copy is not itself a fault -- many correct installations hold one -- so what is reported here is specifically the mismatch.", + auto_applicable: false, + // No repair, and no recommendation between the two. Removing a stack or + // reordering the search path can each break a working Python + // environment, and which is right depends on which stack the user means + // to keep -- a question only they can answer. `rocm diagnose` fills in + // the real paths for this machine; these are the shapes. + commands: &[ + "# Find every copy and which one loads:", + "rocm examine --json # read comgr_paths, comgr_selected, hip_selected", + "# Then pick ONE of the following. They are alternatives, not steps.", + "# (a) Keep the system installation: remove or uninstall the wheel that", + "# supplies the second copy.", + "# (b) Keep the wheel: order the search path so the wheel's own copy of", + "# both libraries is found first, making the wheel the active runtime.", + "export LD_LIBRARY_PATH=\":$LD_LIBRARY_PATH\"", + ], + needs_sudo: false, + needs_reboot: false, + needs_relogin: false, + verify: "python -c \"import torch; torch.zeros(1, device='cuda')\"", + notes: &[ + COMGR_CONFLICT_NEITHER_OPTION_RECOMMENDED, + "This describes the environment outside the CLI's managed runtimes. `rocm serve` puts a managed runtime's libraries first on purpose, so inside one the wheel copy wins by design and that is correct.", + ], + // Not `LINUX_ONLY`: which copy the loader picks has nothing to do with + // the amdgpu module, and the two copies collide on WSL2 just the same. + applies_on: LINUX_AND_WSL, + runner: None, + }, FixRecipe { fix_id: "fix-19-shm-too-small", title: "Raise the shared memory allowance", @@ -1766,9 +1806,9 @@ mod tests { let count = ids.len(); ids.dedup(); assert_eq!(ids.len(), count, "duplicate fix-id in RECIPES"); - // 17 bare-metal/Windows entries (fix-17 and fix-19 among them) plus the - // 7 WSL ones. - assert_eq!(count, 24, "expected 24 catalog entries"); + // 18 bare-metal/Windows entries (fix-17, fix-18 and fix-19 among them) + // plus the 7 WSL ones. + assert_eq!(count, 25, "expected 25 catalog entries"); } #[test] diff --git a/crates/rocm-core/src/lib.rs b/crates/rocm-core/src/lib.rs index 303858b51..6cd78832f 100644 --- a/crates/rocm-core/src/lib.rs +++ b/crates/rocm-core/src/lib.rs @@ -4146,6 +4146,7 @@ pub(crate) fn managed_therock_sdk_probe_candidates( site_packages: sdk.site_packages, root_path, bin_path, + library_paths: sdk.library_paths, }); } candidates.sort_by_key(|candidate| std::cmp::Reverse(candidate.installed_at_unix_ms)); @@ -4340,6 +4341,14 @@ pub(crate) struct TheRockSdkProbeCandidate { pub(crate) site_packages: Option, pub(crate) root_path: PathBuf, bin_path: PathBuf, + /// The SDK's own recorded library directories -- every package root the + /// probe script actually imported and asked Python for (see + /// `ROCM_SDK_PROBE_SCRIPT`'s `add_runtime_root`), not a layout guessed from + /// `root_path`/`site_packages` after the fact. This is what + /// `probe_runtime_devices` puts on `LD_LIBRARY_PATH` for a served process; + /// a caller that needs to find a managed runtime's actual libraries (comgr + /// included) should prefer this over re-deriving the layout. + pub(crate) library_paths: Vec, } pub fn detect_host_gfx_target() -> Option { @@ -10566,6 +10575,57 @@ Class Name: Display Ok(()) } + /// `managed_therock_sdk_probe_candidates` surfaces the SDK's own recorded + /// `library_paths` rather than dropping them. + /// + /// Those are what `examine`'s comgr/HIP search now reads to find a managed + /// runtime's libraries (see `known_install_roots`/`install_library_dirs` in + /// `examine.rs`), in place of re-deriving the layout from `root_path` and + /// `site_packages` after the fact -- a guess that does not hold for every + /// real install shape, which is what left a managed runtime's own code + /// object manager library unseen on a real host + /// (`examine-finds-the-managed-runtimes-own-compilation-library`). A + /// candidate whose `library_paths` came back empty would defeat that fix + /// silently, so this pins the field surviving the read. + #[test] + fn managed_sdk_probe_candidate_carries_recorded_library_paths() -> Result<()> { + let (root, paths) = temp_app_paths("managed-sdk-library-paths"); + let registry = paths.data_dir.join("runtimes").join("registry"); + let site_packages = root.join("site-packages"); + let sdk_root = site_packages.join("_rocm_sdk_devel"); + let sdk_bin = sdk_root.join("bin"); + let comgr_dir = site_packages.join("_rocm_sdk_core").join("lib"); + fs::create_dir_all(&sdk_bin)?; + fs::create_dir_all(&comgr_dir)?; + fs::create_dir_all(®istry)?; + fs::write( + registry.join("runtime.json"), + serde_json::to_vec_pretty(&serde_json::json!({ + "runtime_id": "therock-release:gfx120X-all", + "family": "gfx120X-all", + "installed_at_unix_ms": 10, + "rocm_sdk": { + "import_ok": true, + "site_packages": site_packages, + "root_path": sdk_root, + "bin_path": sdk_bin, + "library_paths": [comgr_dir] + } + }))?, + )?; + + let candidates = managed_therock_sdk_probe_candidates(®istry); + assert_eq!(candidates.len(), 1, "expected exactly one candidate"); + assert_eq!( + candidates[0].library_paths, + vec![comgr_dir], + "the recorded library_paths must survive into the candidate, or the \ + comgr/HIP search has nowhere else reliable to find them" + ); + fs::remove_dir_all(root).ok(); + Ok(()) + } + #[test] fn managed_sdk_probe_skips_non_therock_manifests() -> Result<()> { let (root, paths) = temp_app_paths("managed-sdk-skip-non-therock"); diff --git a/docs/wsl.md b/docs/wsl.md index da9974fe7..3c56f7882 100644 --- a/docs/wsl.md +++ b/docs/wsl.md @@ -200,13 +200,18 @@ The WSL entries, in the order a broken stack usually reveals them: | `fix-wsl-5-distro-too-old` | The distro release is below the floor in the prerequisites above | | `fix-wsl-6-host-driver-too-old` | The distro-side plumbing is complete but the Windows host driver is missing or too old | -One entry outside this list also applies here. `fix-19-shm-too-small` is not a +Two entries outside this list also apply here. `fix-19-shm-too-small` is not a WSL entry, but WSL2 ships the same 64 MiB `/dev/shm` a container does, and a serving workload needs gigabytes of it. It reports below 1 GiB, so a WSL2 user can meet it on an otherwise healthy stack. Its guidance names the container and bare-metal cases; under WSL2 the remedy is the host one, remounting `/dev/shm` larger and adding the matching `/etc/fstab` line inside the distro. +`fix-18-comgr-conflict` also applies here: a wheel copy and a system copy of +the code object manager library collide on WSL2 exactly as they do on bare +metal, through the same `LD_LIBRARY_PATH`/loader-cache search, so the finding +is not specific to either platform. + Every WSL remedy is print-only. `rocm fix ` shows the commands and does not run them: they either install packages with `sudo`, edit loader configuration, or belong to the Windows host, and none of that meets the bar the four diff --git a/skills/rocm-doctor/SKILL.md b/skills/rocm-doctor/SKILL.md index b02a8535f..4b9ff4225 100644 --- a/skills/rocm-doctor/SKILL.md +++ b/skills/rocm-doctor/SKILL.md @@ -145,7 +145,7 @@ passes — the GPU is AMD. Linux, Windows and WSL2 all run the same workflow. rocm fix --yes # required to apply in a non-interactive shell ``` - Only the four auto-applicable fixes are ones the CLI runs itself. The other 20 + Only the four auto-applicable fixes are ones the CLI runs itself. The other 21 are **print-only** (bootloader, kernel, reinstall, Windows driver, …): `rocm fix ` just prints the plan for the user to run themselves — no prompt, and the CLI never performs those. diff --git a/skills/rocm-doctor/reference.md b/skills/rocm-doctor/reference.md index be7179abb..a4b5a4ba2 100644 --- a/skills/rocm-doctor/reference.md +++ b/skills/rocm-doctor/reference.md @@ -74,7 +74,7 @@ non-interactive shell without `--yes`, and confirm first. **Two exceptions:** pinned. Run `rocm fix fix-9-igpu-dgpu --device-index N` (not the bare id) once you know N. -## Closed catalog (24 failure modes) +## Closed catalog (25 failure modes) The OS column is the platform family the CLI scopes an entry to, and WSL2 is a family of its own — not a flavour of `linux`. An entry reaches a WSL host only @@ -100,6 +100,7 @@ reporting confident nonsense. | `fix-14-adrenalin-too-old` | windows | Adrenalin / kernel-mode driver too old for the HIP SDK | `hipInfo` can't enumerate, "driver too old", HSA "no agents found" | no | | `fix-15-msvc-redist` | windows | MSVC runtime missing (HIP DLLs can't load) | `vcruntime140.dll` / `vcruntime140_1.dll` missing | no | | `fix-17-torch-dlpack` | linux | `torch-c-dlpack-ext` loads its CUDA prebuilt on a ROCm torch, aborting vLLM's engine start at import time | vLLM engine start fails on import; error names `torch_c_dlpack_ext` or tvm_ffi's `_optional_torch_c_dlpack` | no | +| `fix-18-comgr-conflict` | linux/wsl | The code object manager library (`libamd_comgr`) that would load belongs to a different installation than the HIP runtime that would load, so device code compilation fails with an error naming neither | compilation error naming neither library; `rocm examine --json`'s `comgr_selected`/`hip_selected` resolve to two different `install_root`s | no | | `fix-19-shm-too-small` | linux/wsl | `/dev/shm` too small for a serving workload, which needs gigabytes where a container and WSL2 both default to 64 MB | reported under 1 GiB; a data-loader worker killed by a bus error, or a failed write to a temporary file, with nothing naming shared memory | no | | `fix-wsl-1-gpu-not-exposed` | wsl | `/dev/dxg` absent, so the distro cannot reach the GPU at all | no `/dev/dxg`; in a container, the device was never passed through | no | | `fix-wsl-2-dxcore-missing` | wsl | `/usr/lib/wsl/lib` DXCore shims missing, so the runtime cannot reach the host driver | `/usr/lib/wsl/lib/libdxcore.so` missing (or the directory absent entirely) | no | @@ -116,7 +117,7 @@ they answer for a different platform family. Linux-only: fix-3, -4, -5, -7, -10, -11, -12, -17. Windows-only: fix-13, -14, -15. WSL-only: fix-wsl-1 through fix-wsl-7. Linux + Windows: fix-9. -Linux + WSL: fix-19. Linux + Windows + WSL: fix-1, -2, -6, -8. +Linux + WSL: fix-18, -19. Linux + Windows + WSL: fix-1, -2, -6, -8. ## Framework routing diff --git a/tests/e2e-cucumber/features/diagnose.feature b/tests/e2e-cucumber/features/diagnose.feature index d33c22e74..9169660f5 100644 --- a/tests/e2e-cucumber/features/diagnose.feature +++ b/tests/e2e-cucumber/features/diagnose.feature @@ -289,3 +289,29 @@ Feature: Diagnosing failures and listing fixes When the user previews that fix without applying it Then the preview states that the fix requires sudo and a re-login And the preview states that the CLI can run it automatically + # HIP compiles device code at run time through a library a machine can hold + # more than one copy of. When the copy that loads belongs to a different + # installation than the runtime, compilation fails with an error naming + # neither. Both remedies — remove one stack, or reorder the search path — can + # break a working Python environment, and which is right depends on which + # stack the user means to keep. So the CLI states them and changes nothing. + # + # The conflict itself cannot be provoked here: the suite cannot install a + # second ROCm stack, and the detection rule is proven by unit tests that build + # the machine state directly. What this pins is the half that matters if the + # entry ever stops being advisory — that asking for it changes nothing and + # recommends neither option. + # + # `@requires-os:linux` because `fix-18-comgr-conflict` is registered for + # `["linux", "wsl"]` (comgr and LD_LIBRARY_PATH are POSIX-loader concepts, not + # Windows ones). Unlike diagnose-20's preview, this step applies the fix for + # real, so it goes through the fix's own platform gate and would be refused + # for the wrong reason -- "wrong OS", not "advisory" -- on a native Windows + # lane. `@requires-os:linux` matches WSL2 too, which is where this fix does + # apply. + @id:diagnose-fix-comgr-conflict-is-advisory-only @requires-os:linux + Scenario: diagnose-21 - The fix for a shadowed compilation library changes nothing and recommends nothing + Given a user who has chosen the fix for a shadowed compilation library + When the user asks the CLI to apply that fix + Then the CLI explains that it will not make the change itself + And the CLI offers both options without ranking them diff --git a/tests/e2e-cucumber/features/examine.feature b/tests/e2e-cucumber/features/examine.feature index 4b2a18e58..32c7d4a2a 100644 --- a/tests/e2e-cucumber/features/examine.feature +++ b/tests/e2e-cucumber/features/examine.feature @@ -247,6 +247,8 @@ Feature: GPU detection and system inspection When the user inspects the system in machine-readable form Then the inspection lists the code object manager libraries it found And it names which of them would load, or says it found none + And it lists the HIP runtime libraries the machine holds the same way + And it names which HIP runtime copy would load, or says it found none # The copy this CLI installs itself, which is the case the whole entry exists # for: the install path puts ROCm wheels into a managed environment, so a user @@ -259,14 +261,15 @@ Feature: GPU detection and system inspection # described, not that it matches the one the installer actually produces. A # real managed runtime is the only thing that distinguishes those. # - # `@requires-os:linux` because `probe_comgr` only ever looks for - # `libamd_comgr` -- an ELF shared-object name, found via `LD_LIBRARY_PATH`, + # `@requires-os:linux` because `probe_comgr` only ever looks for `libamd_comgr` + # and `libamdhip64` -- ELF shared-object names, found via `LD_LIBRARY_PATH`, # the loader cache, or an install root's `lib/` tree. None of that exists on - # native Windows, which ships the equivalent library under a different name, - # so the assertion that a managed runtime's copy must be found does not hold - # there. WSL reports `os_family` "linux" (see `expectation.rs`), so this - # still runs on the WSL lane, where the managed runtime really does carry a - # `.so`. + # native Windows, which ships `.dll`s under other names, so the assertion + # that a managed runtime's library must be found does not hold there. WSL + # reports `os_family` "linux" (see `expectation.rs`), so this still runs on + # the WSL lane, where the managed runtime really does carry a `.so`. This + # matches `check_18_comgr_conflict`'s own `&["linux", "wsl"]` gate in + # diagnose.rs -- the same boundary, stated once there and once here. @id:examine-finds-the-managed-runtimes-own-compilation-library @requires-gpu @requires-os:linux Scenario: examine-19 - The inspection finds the compilation library the CLI installed itself Given a managed runtime is active diff --git a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs index 2c59e6de1..c8b29f12f 100644 --- a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs @@ -88,8 +88,7 @@ const CATALOG_FIX_IDS: &[&str] = &[ "fix-wsl-5-distro-too-old", "fix-wsl-6-host-driver-too-old", "fix-wsl-7-wsl1", - // `fix-18` is the code object manager entry on its own branch, kept - // distinct for the same reason `fix-16` is. + "fix-18-comgr-conflict", "fix-19-shm-too-small", ]; @@ -137,6 +136,11 @@ const fn fix_id_for_the_other_os() -> &'static str { } } +/// The entry for a code object manager library belonging to a different +/// installation than the active runtime. Advisory by design: both remedies can +/// break a working Python environment. +const COMGR_CONFLICT_FIX_ID: &str = "fix-18-comgr-conflict"; + /// Contents planted in the scenario's own shell rc file. The assertion is that /// this survives the run byte for byte. const PLANTED_RC: &str = "# planted by the e2e suite; the fix must not touch this\n"; @@ -206,6 +210,11 @@ async fn user_chose_fix_needing_sudo_and_relogin(world: &mut E2eWorld) { world.model_name = Some(COMMAND_FAILURE_FIX_ID.to_string()); } +#[given("a user who has chosen the fix for a shadowed compilation library")] +async fn user_chose_comgr_conflict_fix(world: &mut E2eWorld) { + world.model_name = Some(COMGR_CONFLICT_FIX_ID.to_string()); +} + #[given("a user who names a fix the CLI does not offer")] async fn user_named_unknown_fix(world: &mut E2eWorld) { world.model_name = Some("fix-does-not-exist".to_string()); @@ -1133,3 +1142,41 @@ async fn assert_command_failure_reported_on_stderr(world: &mut E2eWorld) { "the command-failure explanation must not also be on stdout:\n{stdout}" ); } + +#[then("the CLI explains that it will not make the change itself")] +async fn assert_fix_is_advisory(world: &mut E2eWorld) { + let output = world.cli_output.as_ref().expect("no fix output"); + assert_eq!( + world.cli_rc, + Some(0), + "printing advice is not a failure:\n{output}" + ); + assert!( + output.contains("print-only") || output.contains("will NOT run it"), + "the user has to be told the CLI is not going to do this for them:\n{output}" + ); +} + +#[then("the CLI offers both options without ranking them")] +async fn assert_both_options_unranked(world: &mut E2eWorld) { + let output = world.cli_output.as_ref().expect("no fix output"); + // Both remedies have to be present. Offering one is a recommendation by + // omission, and the wrong one breaks a working environment. + // + // Keyed on the option markers rather than on words like "remove", which also + // occur in the surrounding prose -- an assertion that matched those would + // still pass with one of the two options deleted, which is exactly the + // regression it exists to catch. + for option in ["(a)", "(b)"] { + assert!( + output.contains(option), + "only one way out was offered; option `{option}` is missing, which makes \ + the other a recommendation by omission:\n{output}" + ); + } + assert!( + output.contains("Neither option is recommended"), + "the CLI has to say it is not choosing between them -- which is right \ + depends on which stack the user means to keep:\n{output}" + ); +} diff --git a/tests/e2e-cucumber/tests/e2e/examine_steps.rs b/tests/e2e-cucumber/tests/e2e/examine_steps.rs index 62cf34183..58e9f2f0f 100644 --- a/tests/e2e-cucumber/tests/e2e/examine_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/examine_steps.rs @@ -1039,7 +1039,7 @@ async fn assert_comgr_copies_reported(world: &mut E2eWorld) { // Each entry has to carry enough to act on. A list of bare paths would not // say which install a copy belongs to, which is the whole question. for copy in copies { - for field in ["path", "real_path", "version", "source"] { + for field in ["path", "real_path", "version", "source", "install_root"] { assert!( copy.get(field).is_some(), "a reported copy is missing `{field}`, so a reader cannot tell \ @@ -1049,14 +1049,117 @@ async fn assert_comgr_copies_reported(world: &mut E2eWorld) { } } +#[then("it lists the HIP runtime libraries the machine holds the same way")] +async fn assert_hip_copies_reported(world: &mut E2eWorld) { + assert_eq!( + world.cli_rc, + Some(0), + "finding no library is a finding, not a failure" + ); + let value = parsed_json(world); + let copies = value + .get("hip_paths") + .unwrap_or_else(|| panic!("the inspection never answered the question:\n{value:#}")); + let copies = copies + .as_array() + .unwrap_or_else(|| panic!("the answer has to be a list of copies:\n{copies:#}")); + // Whether the code object manager belongs to the active runtime is a + // question about two libraries, not one -- so the HIP side has to carry + // the same `install_root` attribution the comgr side does, or there is + // nothing for the conflict check to compare against. + for copy in copies { + for field in ["path", "real_path", "version", "source", "install_root"] { + assert!( + copy.get(field).is_some(), + "a reported HIP runtime copy is missing `{field}`, so a reader \ + cannot tell where it came from:\n{copy:#}" + ); + } + } +} + +// Sources the loader itself actually consults, mirrored from +// `LOADER_PATH_SOURCES` in `rocm-core`'s `examine.rs`: a `rocm-install` or +// `managed-runtime` hit is evidence a copy exists, not evidence anything +// would load it. Shared by the HIP and comgr selection assertions below so +// the two cannot drift apart. +const LOADER_PATH_SOURCES: [&str; 3] = ["active-runtime", "ld-library-path", "loader-cache"]; + +#[then("it names which HIP runtime copy would load, or says it found none")] +async fn assert_hip_selection_is_stated(world: &mut E2eWorld) { + let value = parsed_json(world); + let copies = value["hip_paths"] + .as_array() + .expect("hip_paths must be a list") + .clone(); + let selected = value + .get("hip_selected") + .unwrap_or_else(|| panic!("the inspection never said which copy wins:\n{value:#}")); + + if copies.is_empty() { + assert!( + selected.is_null(), + "no copies were found, so none can have been selected:\n{selected:#}" + ); + // Same reasoning as the comgr assertion below: `hip_paths: []` and + // `hip_selected: null` also hold by nothing more than `Examination`'s + // own defaults, so without this the assertion cannot tell "probed, + // found none" from "never probed". + let notes = value["notes"].as_array().expect("notes must be a list"); + assert!( + notes + .iter() + .filter_map(serde_json::Value::as_str) + .any(|note| note.contains("no libamdhip64 found")), + "no HIP runtime copies were reported, but the inspection's notes \ + never say the search ran and found none -- so this cannot tell \ + \"probed, found nothing\" from \"never probed\":\n{value:#}" + ); + } else if selected.is_null() { + // Copies exist, but none sits on a tier the loader itself consults -- + // every hit is a `rocm-install` or `managed-runtime` copy nothing has + // put on the library path, in the loader cache, or in front of an + // active runtime. This is `select_loader_copy` returning `None` on + // purpose (pinned there for `libamdhip64` directly), not a gap in the + // step -- without this arm it fell into the branch below and panicked + // on a state the CLI deliberately produces. + for copy in &copies { + let source = copy + .get("source") + .and_then(serde_json::Value::as_str) + .unwrap_or_else(|| panic!("every copy must name its source:\n{copy:#}")); + assert!( + !LOADER_PATH_SOURCES.contains(&source), + "a copy on a loader-consulted tier ({source}) was found, but none was \ + selected -- the selection must have missed a real loader-path hit:\n{value:#}" + ); + } + } else { + let path = selected + .get("path") + .and_then(serde_json::Value::as_str) + .unwrap_or_else(|| panic!("copies were found but none was selected:\n{value:#}")); + let selected_source = selected + .get("source") + .and_then(serde_json::Value::as_str) + .unwrap_or_else(|| panic!("the selected copy must name its source:\n{value:#}")); + assert!( + LOADER_PATH_SOURCES.contains(&selected_source), + "the selected copy's source ({selected_source}) is not one the loader actually \ + consults; a rocm-install or managed-runtime hit must never be reported as \ + \"would load\":\n{value:#}" + ); + assert_eq!( + Some(path), + copies[0].get("path").and_then(serde_json::Value::as_str), + "the selected copy has to be the first in search order; anything else \ + means the list and the verdict disagree about what the loader does" + ); + } +} + #[then("it names which of them would load, or says it found none")] async fn assert_comgr_selection_is_stated(world: &mut E2eWorld) { - // Sources the loader itself actually consults, mirrored from - // `LOADER_PATH_SOURCES` in `rocm-core`'s `examine.rs`: a `rocm-install` or - // `managed-runtime` hit is evidence a copy exists, not evidence anything - // would load it. - const LOADER_PATH_SOURCES: [&str; 3] = ["active-runtime", "ld-library-path", "loader-cache"]; - let value = parsed_json(world); let copies = value["comgr_paths"] .as_array() diff --git a/tests/e2e-cucumber/tests/skill_reference.rs b/tests/e2e-cucumber/tests/skill_reference.rs index 9d5d53e15..5cfd704c8 100644 --- a/tests/e2e-cucumber/tests/skill_reference.rs +++ b/tests/e2e-cucumber/tests/skill_reference.rs @@ -55,6 +55,110 @@ fn catalog_rows(md: &str) -> Vec<(String, String)> { rows } +fn skill_md_path() -> PathBuf { + Path::new(env!("CARGO_MANIFEST_DIR")) + .join("..") + .join("..") + .join("skills") + .join("rocm-doctor") + .join("SKILL.md") +} + +/// How many catalog rows have `yes` in the Auto-fix cell (`cells[4]`, the same +/// column `catalog_rows` leaves unparsed since the os-scope check only needs +/// `cells[1]`). +fn catalog_auto_count(md: &str) -> usize { + md.lines() + .filter(|line| { + let line = line.trim(); + if !line.starts_with('|') { + return false; + } + let cells: Vec<&str> = line.trim_matches('|').split('|').map(str::trim).collect(); + cells.len() >= 5 && cells[0].trim_matches('`').starts_with("fix-") && cells[4] == "yes" + }) + .count() +} + +/// Two free-standing counts restate the table in prose rather than in cells a +/// parser already checks: `reference.md`'s "(N failure modes)" heading, and +/// `SKILL.md`'s "the other M are print-only" line. `rocm_doctor_skill.feature` +/// says plainly that these are not parsed and can go stale even while the +/// table itself stays correct -- which is exactly what happened here (the +/// catalog grew by one row and both numbers were left behind). This does not +/// reopen that scope decision; it only adds a floor cheap enough that the next +/// drift fails a `cargo test` instead of waiting for someone to count rows by +/// hand. +#[test] +fn free_standing_catalog_counts_match_the_table() { + let reference_md = std::fs::read_to_string(reference_md_path()) + .unwrap_or_else(|e| panic!("failed to read {}: {e}", reference_md_path().display())); + let rows = catalog_rows(&reference_md); + assert!( + !rows.is_empty(), + "no catalog rows found in {}", + reference_md_path().display() + ); + let total = rows.len(); + let auto = catalog_auto_count(&reference_md); + + let heading = reference_md + .lines() + .find(|line| line.trim_start().starts_with("## Closed catalog (")) + .unwrap_or_else(|| { + panic!( + "no '## Closed catalog (N failure modes)' heading found in {}", + reference_md_path().display() + ) + }); + let heading_count: usize = heading + .split('(') + .nth(1) + .and_then(|rest| rest.split_whitespace().next()) + .and_then(|n| n.parse().ok()) + .unwrap_or_else(|| panic!("could not parse the failure-mode count out of {heading:?}")); + assert_eq!( + heading_count, total, + "skills/rocm-doctor/reference.md's '(N failure modes)' heading says {heading_count}, \ + but the table has {total} rows" + ); + + let skill_md = std::fs::read_to_string(skill_md_path()) + .unwrap_or_else(|e| panic!("failed to read {}: {e}", skill_md_path().display())); + // "The other N" and "are **print-only**" sit on adjacent source lines that + // wrap a single sentence, so join the whole doc on whitespace first rather + // than matching one line -- a line-scoped search would find the + // print-only line without the number on it and fail to parse. + let flattened = skill_md.split_whitespace().collect::>().join(" "); + let after_the_other = flattened.split("The other ").nth(1).unwrap_or_else(|| { + panic!( + "no 'The other N ... print-only' phrase found in {}", + skill_md_path().display() + ) + }); + assert!( + after_the_other.starts_with(|c: char| c.is_ascii_digit()) + && after_the_other.contains("print-only"), + "'The other N' in {} is not followed by a number and 'print-only' as expected: {:?}", + skill_md_path().display(), + after_the_other.chars().take(60).collect::() + ); + let print_only_count: usize = after_the_other + .split_whitespace() + .next() + .and_then(|n| n.parse().ok()) + .unwrap_or_else(|| { + panic!("could not parse the print-only count out of {after_the_other:?}") + }); + assert_eq!( + print_only_count, + total - auto, + "skills/rocm-doctor/SKILL.md says {print_only_count} fixes are print-only, but the \ + table has {total} rows of which {auto} are auto-applicable ({} expected)", + total - auto + ); +} + #[test] fn catalog_os_scopes_use_the_cli_spellings() { // A shorthand such as `both` reads as "every platform" but cannot say