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