diff --git a/crates/rocm-core/src/diagnose.rs b/crates/rocm-core/src/diagnose.rs index fbc1b8867..70fc1a83a 100644 --- a/crates/rocm-core/src/diagnose.rs +++ b/crates/rocm-core/src/diagnose.rs @@ -236,6 +236,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"), @@ -1388,6 +1410,136 @@ 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(), + 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 @@ -2038,11 +2190,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"]), ]; @@ -2589,6 +2746,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 @@ -2632,6 +2868,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 @@ -2647,6 +2917,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 @@ -2678,6 +2973,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 2a3244f12..13a85a71d 100644 --- a/crates/rocm-core/src/examine.rs +++ b/crates/rocm-core/src/examine.rs @@ -155,6 +155,42 @@ pub struct WslFacts { pub locally_probed: bool, } +/// 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 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, + /// The file the path resolves to. Two entries naming one file through a + /// symlink are collapsed on this, so a versioned library and its unversioned + /// alias are not reported as two copies. + pub real_path: String, + /// Version read from the resolved file name, empty when it carries none. + /// + /// Empty is an honest answer. The version is read from the name rather than + /// by loading the library, so a renamed file yields nothing -- which is + /// better than a confident wrong answer. + pub version: String, + /// 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. /// /// Field order and names mirror `examine.py`'s `Examination` dataclass so the @@ -206,6 +242,64 @@ pub struct Examination { pub hip_libs_on_ld_path: Option, pub rocm_repos_seen: Vec, + // Code object manager (Linux). HIP compiles device code at run time through + // `libamd_comgr`, and a machine can hold more than one copy -- a system ROCm + // install and a ROCm Python wheel each ship one. Holding two is not a fault; + // every managed environment this CLI creates holds one, and there the wheel + // copy is the right copy to load. What matters is which copy the loader + // picks, and that was invisible: nothing looked past the first match. + // `serde(default)` on all three, because this structure is read back from + // *another machine*: `rocm remote doctor` deserializes an examination the + // remote's own CLI produced, and that CLI may predate these fields. Without + // a default, adding a field here refuses every remote running an older + // build -- reported to the user as "the remote CLI is probably a different + // version", which is true and useless, since the older CLI is the one that + // cannot be changed. + /// 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, + /// 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, + /// 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, pub hip_sdk_version: String, @@ -303,6 +397,12 @@ impl Default for Examination { rocminfo_status: String::new(), hip_libs_on_ld_path: None, rocm_repos_seen: Vec::new(), + 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, @@ -389,6 +489,10 @@ impl Examination { // WSL2 ships the same 64 MB default a container does, so this is one // of the platforms where the shortage is most likely to be real. probe_shared_memory(&mut e); + // A wheel copy and a system copy collide on WSL2 exactly as they do + // on bare metal, so skipping the search here would leave a WSL user + // unable to see a conflict that is really there. + probe_comgr(&mut e, interpreter); e.status = e.compute_status(); return e; } @@ -403,6 +507,9 @@ impl Examination { probe_secure_boot(&mut e); probe_rocm_install(&mut e); probe_env(&mut e); + // After both: the search consults `LD_LIBRARY_PATH` (read by + // `probe_env`) and the resolved ROCm install (`probe_rocm_install`). + probe_comgr(&mut e, interpreter); probe_container(&mut e); probe_shared_memory(&mut e); probe_dmesg_amdgpu(&mut e); @@ -2074,34 +2181,570 @@ fn probe_env(e: &mut Examination) { e.env.insert((*key).to_owned(), value); } let ld = std::env::var("LD_LIBRARY_PATH").unwrap_or_default(); - let mut hit: Option = None; + let hit = libraries_on_ld_path(&ld, "libamdhip64").into_iter().next(); + if let Some(hit) = hit { + e.hip_libs_on_ld_path = Some(true); + e.notes + .push(format!("libamdhip64 visible via LD_LIBRARY_PATH: {hit}")); + } else { + e.hip_libs_on_ld_path = if ld.is_empty() { None } else { Some(false) }; + } +} + +/// Every file in `ld` whose name starts with `prefix`, in search order. +/// +/// One walker, two callers. The HIP probe wants only the first hit; the code +/// object manager scan wants all of them, because stopping at the first is +/// exactly what made a second copy invisible. Written once so the two searches +/// cannot come to disagree about what "on the library path" means. +/// +/// `LD_LIBRARY_PATH` is a Linux concept and so is its `:` separator — a Windows +/// path contains a colon, so splitting one here would destroy it. That costs +/// nothing in practice: the variable is not what the Windows loader reads, the +/// code object manager scan runs only on Linux, and on Windows the HIP caller +/// sees an unset variable and finds nothing, exactly as it did before. It does +/// mean the tests for this are Unix-only, and they say so. +fn libraries_on_ld_path(ld: &str, prefix: &str) -> Vec { + let mut found = Vec::new(); for dir in ld.split(':') { if dir.is_empty() { continue; } - if let Ok(entries) = std::fs::read_dir(dir) { - for entry in entries.flatten() { - if entry - .file_name() - .to_string_lossy() - .starts_with("libamdhip64") - { - hit = Some(entry.path().to_string_lossy().into_owned()); - break; - } - } + collect_libraries_in_dir(std::path::Path::new(dir), prefix, &mut found); + } + found +} + +/// Append every file in `dir` whose name starts with `prefix`, sorted so the +/// result does not depend on directory iteration order. +/// +/// An unreadable directory contributes nothing and is not an error: the library +/// path routinely names directories that do not exist, and a probe that failed +/// on one would report nothing about the machine it was asked to describe. +fn collect_libraries_in_dir(dir: &std::path::Path, prefix: &str, found: &mut Vec) { + let Ok(entries) = std::fs::read_dir(dir) else { + return; + }; + let mut matches: Vec = entries + .flatten() + .filter(|entry| entry.file_name().to_string_lossy().starts_with(prefix)) + .map(|entry| entry.path().to_string_lossy().into_owned()) + .collect(); + // Sorted, which does change the HIP probe's tie-break when one directory + // holds more than one matching file: it used to take whatever `read_dir` + // happened to yield first. Deterministic is the better answer for a report + // two people compare, but it is a change, not a no-op. + matches.sort(); + found.append(&mut matches); +} + +/// The library HIP uses to compile device code at run time. +const COMGR_LIB_PREFIX: &str = "libamd_comgr"; + +/// Sources the loader itself actually consults, in the order it consults them. +/// +/// `rocm-install` and `managed-runtime` hits are real copies sitting on disk, +/// but the loader does not search `/opt` or a managed runtime's directory on +/// its own -- a copy found only there loads solely because something *else* +/// (an active runtime's prepended `LD_LIBRARY_PATH`, or the loader cache after +/// `ldconfig` has been run on it) already put it on a path the loader does +/// consult, and that case is already covered by the tiers listed here. A copy +/// whose *only* hit is `rocm-install`/`managed-runtime` is evidence the file +/// exists, not evidence anything would load it. +const LOADER_PATH_SOURCES: [&str; 3] = ["active-runtime", "ld-library-path", "loader-cache"]; + +/// 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 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 +/// remaps paths at run time. The selected copy is the one that *would* load on +/// the evidence available, and the list of copies is that evidence -- not a +/// guarantee. Reporting the search honestly is worth more here than a confident +/// answer that cannot be justified. +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_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 comgr_selected = select_loader_copy(&comgr); + if let Some(selected) = &comgr_selected { + e.comgr_version.clone_from(&selected.version); + } + if let Some(note) = copies_note(COMGR_LIB_PREFIX, &comgr, comgr_selected.as_ref()) { + e.notes.push(note); + } + + 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" + )) +} + +/// Every copy of `prefix` on the machine, in loader search order, each attributed +/// to the installation that owns it. +/// +/// `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. +/// +/// `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_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 + // would consult, so the first copy recorded on a loader-consulted tier is + // the one that wins. + for dir in active_runtime_dirs { + let mut hits = Vec::new(); + collect_libraries_in_dir(dir, prefix, &mut hits); + for path in hits { + record_library_copy(&mut copies, &mut seen, &path, "active-runtime", dir_owners); } - if hit.is_some() { - break; + } + for path in libraries_on_ld_path(ld_library_path, prefix) { + record_library_copy(&mut copies, &mut seen, &path, "ld-library-path", dir_owners); + } + 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); } } - if let Some(hit) = hit { - e.hip_libs_on_ld_path = Some(true); - e.notes - .push(format!("libamdhip64 visible via LD_LIBRARY_PATH: {hit}")); + for (dir, source, _root) in dirs { + let mut hits = Vec::new(); + collect_libraries_in_dir(dir, prefix, &mut hits); + for path in hits { + record_library_copy(&mut copies, &mut seen, &path, source, dir_owners); + } + } + copies +} + +/// 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. +/// +/// 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_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 + // it is more use than pretending the machine does not hold it. + let real_path = std::fs::canonicalize(path).map_or_else( + |_| path.to_owned(), + |resolved| resolved.to_string_lossy().into_owned(), + ); + if !seen.insert(real_path.clone()) { + return; + } + 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(), + }); +} + +/// Read the version out of a resolved library file name. +/// +/// `libamd_comgr.so.2.8.0` carries its version in the soname, which is where +/// this reads it from. Deliberately not by loading the library and asking it: +/// `dlopen` runs the library's initialisers, and running code out of an +/// unknown library is not something a diagnostic tool should do on a machine it +/// has been called to because something is already wrong. A renamed file yields +/// an empty version, which is the honest answer. +fn comgr_version_from_file_name(real_path: &str) -> String { + let name = std::path::Path::new(real_path) + .file_name() + .map(|value| value.to_string_lossy().into_owned()) + .unwrap_or_default(); + let Some((_, version)) = name.split_once(".so.") else { + return String::new(); + }; + // Digits and dots only: `libamd_comgr.so.2.8.0` yields a version, while a + // file that merely happens to carry `.so.` in a longer name does not get a + // nonsense one read out of it. + if !version.is_empty() && version.chars().all(|c| c.is_ascii_digit() || c == '.') { + version.to_owned() } else { - e.hip_libs_on_ld_path = if ld.is_empty() { None } else { Some(false) }; + String::new() + } +} + +/// 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/...`. +/// 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. +/// +/// 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() + .filter(|line| line.contains(prefix)) + .filter_map(|line| line.split_once("=> ")) + .map(|(_, path)| path.trim().to_owned()) + .collect() +} + +/// Sibling ROCm installs under `/opt`, sorted for a deterministic search order. +/// +/// A versioned install left behind by an upgrade is one of the two copies +/// `probe_comgr` exists to find. Takes the directory to scan as a parameter +/// -- always `/opt` in production -- so a test can point it at a temp folder +/// instead of depending on what happens to live under the real `/opt` on the +/// machine running the suite. +fn rocm_install_siblings(opt_dir: &std::path::Path) -> Vec { + let Ok(entries) = std::fs::read_dir(opt_dir) else { + return Vec::new(); + }; + let mut roots: Vec = entries + .flatten() + .map(|entry| entry.path()) + .filter(|path| { + path.file_name() + .is_some_and(|name| name.to_string_lossy().starts_with("rocm")) + }) + .collect(); + roots.sort(); + 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) +} + +/// Drop repeats of a root already seen, keeping the first occurrence. +/// +/// 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 seen = std::collections::HashSet::new(); + roots + .into_iter() + .filter(|(root, _)| seen.insert(root.clone())) + .collect() +} + +/// The library directories of every known installation, in root order, each +/// paired with the source label and the root that owns it. +/// +/// 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, 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 +/// 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 +/// `crate::runtime::default_data_dir` directly: the latter only knows +/// `$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. 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, Vec)> = + crate::managed_therock_sdk_probe_candidates(®istry) + .into_iter() + .map(|candidate| (candidate.root_path, candidate.library_paths)) + .collect(); + runtimes.sort(); + runtimes } /// Truncate `value` to at most `max_chars` characters, appending a marker when @@ -2748,6 +3391,220 @@ mod tests { assert_eq!(distro_clears_wsl_floor("UBUNTU", "24.04"), Some(true)); } + /// A scratch directory unique to the calling test, cleaned up by the caller. + fn scratch_dir(label: &str) -> std::path::PathBuf { + let dir = std::env::temp_dir().join(format!( + "rocm-comgr-{label}-{}-{:?}", + std::process::id(), + std::thread::current().id() + )); + std::fs::create_dir_all(&dir).expect("create scratch dir"); + dir + } + + fn plant(dir: &std::path::Path, name: &str) -> std::path::PathBuf { + let path = dir.join(name); + std::fs::write(&path, b"").expect("plant library"); + path + } + + // Unix-only: these drive `LD_LIBRARY_PATH` semantics and POSIX symlinks + // directly. The variable uses `:` as its separator, which a Windows path + // contains, and `symlink` needs privileges there. The code under test runs + // only on Linux, so gating the tests loses no coverage of a path that ships. + #[cfg(unix)] + #[test] + fn the_library_path_is_searched_left_to_right_and_every_copy_is_kept() { + // The defect this whole entry exists for: the old scan stopped at the + // first hit, so a second copy could never be reported. Order matters as + // much as completeness -- the first entry is the claim about which copy + // wins. + let root = scratch_dir("order"); + let first = root.join("first"); + let second = root.join("second"); + std::fs::create_dir_all(&first).expect("create dir"); + std::fs::create_dir_all(&second).expect("create dir"); + plant(&first, "libamd_comgr.so.2.8.0"); + plant(&second, "libamd_comgr.so.3.0.0"); + + let ld = format!("{}:{}", first.display(), second.display()); + let found = libraries_on_ld_path(&ld, COMGR_LIB_PREFIX); + + assert_eq!(found.len(), 2, "both copies must be reported: {found:?}"); + assert!( + found[0].starts_with(first.to_string_lossy().as_ref()), + "the earlier library-path entry has to come first: {found:?}" + ); + std::fs::remove_dir_all(&root).ok(); + } + + // Unix-only: these drive `LD_LIBRARY_PATH` semantics and POSIX symlinks + // directly. The variable uses `:` as its separator, which a Windows path + // contains, and `symlink` needs privileges there. The code under test runs + // only on Linux, so gating the tests loses no coverage of a path that ships. + #[cfg(unix)] + #[test] + fn a_missing_library_path_entry_is_skipped_rather_than_failing_the_probe() { + // `LD_LIBRARY_PATH` routinely names directories that do not exist. A + // probe that gave up on one would report nothing about the machine it + // was asked to describe. + let root = scratch_dir("missing"); + plant(&root, "libamd_comgr.so.2.8.0"); + let ld = format!("/nonexistent-{}:{}", std::process::id(), root.display()); + + let found = libraries_on_ld_path(&ld, COMGR_LIB_PREFIX); + + assert_eq!(found.len(), 1, "the readable entry still counts: {found:?}"); + std::fs::remove_dir_all(&root).ok(); + } + + // Unix-only: these drive `LD_LIBRARY_PATH` semantics and POSIX symlinks + // directly. The variable uses `:` as its separator, which a Windows path + // contains, and `symlink` needs privileges there. The code under test runs + // only on Linux, so gating the tests loses no coverage of a path that ships. + #[cfg(unix)] + #[test] + fn a_versioned_library_and_its_symlink_count_as_one_copy() { + // ROCm ships `libamd_comgr.so.2` beside `libamd_comgr.so.2.8.0`, one a + // symlink to the other. Counting those as two copies would invent a + // conflict on an ordinary install -- the false report that matters most + // to avoid, since it would fire on healthy machines. + let root = scratch_dir("symlink"); + let real = plant(&root, "libamd_comgr.so.2.8.0"); + let link = root.join("libamd_comgr.so.2"); + std::os::unix::fs::symlink(&real, &link).expect("create symlink"); + + let mut copies = Vec::new(); + let mut seen = std::collections::BTreeSet::new(); + 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(), + 1, + "one file reached by two names is one copy: {copies:?}" + ); + assert_eq!( + copies[0].version, "2.8.0", + "the version comes from the resolved file, not the name used to reach it" + ); + std::fs::remove_dir_all(&root).ok(); + } + + #[test] + fn two_distinct_files_are_two_copies() { + // The converse of the symlink case, so that test cannot be satisfied by + // a probe that simply never reports more than one. + let root = scratch_dir("distinct"); + let a = plant(&root, "libamd_comgr.so.2.8.0"); + let b = plant(&root, "libamd_comgr.so.3.0.0"); + + let mut copies = Vec::new(); + let mut seen = std::collections::BTreeSet::new(); + 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 [ + ("libamd_comgr.so.2.8.0", "2.8.0"), + ("libamd_comgr.so.2", "2"), + // No version to read. Empty is the honest answer; the alternative + // is a confident wrong one, and the version is only ever report + // text. + ("libamd_comgr.so", ""), + ("libamd_comgr-renamed.so.beta", ""), + ] { + assert_eq!( + comgr_version_from_file_name(&format!("/x/{name}")), + expected, + "{name} read wrong" + ); + } + } + #[test] fn examination_serializes_expected_keys() { let e = Examination::default(); @@ -2823,6 +3680,19 @@ mod tests { "rocminfo_status", "hip_libs_on_ld_path", "rocm_repos_seen", + // CLI additions beyond examine.py, added deliberately: which copies + // of the code object manager library the machine holds, and which + // one would load. examine.py never looked, which is why a second + // copy shadowing the first was invisible. + "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", @@ -2919,6 +3789,580 @@ 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 -- + /// `_rocm_sdk_devel`, matching what the probe script's + /// `_devel.get_devel_root()` records as `root_path` when the `devel` extra + /// is installed -- never a venv root above `site_packages`. The ROCm + /// libraries do not sit under it either: they sit in the sibling + /// `_rocm_sdk_core` package, beside `root`, both directly under + /// `site_packages`. A fixture built as `root/lib//site-packages` + /// (a venv layout the installer never produces) would pass against a shape + /// production cannot create -- this one matches `apps/rocm/src/therock.rs`'s + /// `ROCM_SDK_PROBE_SCRIPT` instead. + #[cfg(target_os = "linux")] + fn wheel_runtime_on_disk(tag: &str) -> (std::path::PathBuf, Option) { + use std::fs; + let base = std::env::temp_dir().join(format!( + "rocm-comgr-wheel-{tag}-{}-{:?}", + std::process::id(), + std::thread::current().id() + )); + let _ = fs::remove_dir_all(&base); + let site_packages = base.join("site-packages"); + let root = site_packages.join("_rocm_sdk_devel"); + let sdk_lib = site_packages.join("_rocm_sdk_core").join("lib"); + fs::create_dir_all(&root).unwrap(); + fs::create_dir_all(&sdk_lib).unwrap(); + fs::write(sdk_lib.join("libamd_comgr.so.3"), b"fake").unwrap(); + (root, Some(site_packages)) + } + + /// The managed copy this CLI installs itself is reachable by the search. + /// + /// That copy is the whole reason this entry exists: the CLI puts ROCm + /// wheels into a managed environment, so a user who follows that path on a + /// host already carrying a system install ends up holding both, having done + /// nothing unusual. A search that cannot see the copy we put there reports + /// a conflict-free machine no matter what else is true of it. + #[cfg(target_os = "linux")] + #[test] + fn the_managed_copy_this_cli_installs_is_reachable() { + let (root, site_packages) = wheel_runtime_on_disk("found"); + 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") + .join("_rocm_sdk_core") + .join("lib"); + assert!( + dirs.contains(&expected), + "the search missed the copy the CLI installs, which is the case this entry exists \ + for. Looked in: {dirs:?}" + ); + let _ = std::fs::remove_dir_all(root.parent().and_then(|p| p.parent()).unwrap()); + } + + /// A runtime whose SDK recorded no `site-packages` still contributes what + /// can be read from its root. + /// + /// 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_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. + #[cfg(target_os = "linux")] + #[test] + fn a_runtime_with_no_recorded_site_packages_still_contributes_its_root() { + use std::fs; + let root = std::env::temp_dir().join(format!( + "rocm-comgr-rootonly-{}-{:?}", + std::process::id(), + std::thread::current().id() + )); + let _ = fs::remove_dir_all(&root); + fs::create_dir_all(root.join("lib")).unwrap(); + + 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 \ + working: {dirs:?}" + ); + let _ = fs::remove_dir_all(&root); + } + + /// 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. + /// + /// `#[cfg(unix)]`, matching its only callers: every one of them feeds the + /// directory into the `:`-split `LD_LIBRARY_PATH` parser, so leaving this + /// helper itself ungated would make it unused (and clippy-denied) on + /// Windows once its callers are gated. + #[cfg(unix)] + fn comgr_copy_dir(tag: &str) -> std::path::PathBuf { + let dir = std::env::temp_dir().join(format!( + "rocm-comgr-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("libamd_comgr.so.2"), b"fake").unwrap(); + 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 `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. + /// + /// Unix-only: feeds a temp path into the `:`-split `LD_LIBRARY_PATH` + /// parser, same as the neighbouring LD-path tests above -- a Windows path + /// contains `:` after its drive letter, which would corrupt the split. + #[cfg(unix)] + #[test] + fn the_active_runtimes_own_copy_outranks_the_ambient_environment() { + 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 = find_library_copies( + COMGR_LIB_PREFIX, + std::slice::from_ref(&active), + &ambient.to_string_lossy(), + Some(&loader_cache_text), + &[], + &std::collections::HashMap::new(), + ); + let selected = select_loader_copy(&copies); + + assert_eq!( + copies.first().map(|copy| ©.source), + Some(&"active-runtime".to_owned()), + "the active runtime's own copy must be selected ahead of the ambient \ + `LD_LIBRARY_PATH` and the loader cache, found: {copies:?}" + ); + assert_eq!( + selected.as_ref().map(|copy| ©.source), + Some(&"active-runtime".to_owned()), + "the selected copy must also be the active runtime's own: {selected:?}" + ); + assert_eq!( + copies.len(), + 3, + "all three copies must still be reported: {copies:?}" + ); + + let _ = std::fs::remove_dir_all(&active); + let _ = std::fs::remove_dir_all(&ambient); + let _ = std::fs::remove_dir_all(&cached); + } + + /// With no active-runtime evidence, the plain `LD_LIBRARY_PATH` still wins + /// over the loader cache and the trailing search directories -- the + /// ordering `find_library_copies`'s doc comment says is "the whole point". + /// + /// Unix-only: same `:`-split reason as above. + #[cfg(unix)] + #[test] + fn ld_library_path_outranks_loader_cache_and_search_dirs() { + 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 = find_library_copies( + COMGR_LIB_PREFIX, + &[], + &ld.to_string_lossy(), + 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), + Some(&"ld-library-path".to_owned()), + "found: {copies:?}" + ); + assert_eq!( + selected.as_ref().map(|copy| ©.source), + Some(&"ld-library-path".to_owned()), + "selected: {selected:?}" + ); + assert_eq!(copies.len(), 3, "found: {copies:?}"); + + let _ = std::fs::remove_dir_all(&ld); + let _ = std::fs::remove_dir_all(&cached); + let _ = std::fs::remove_dir_all(&known); + } + + /// When the active runtime's own directory is *also* one of the generic + /// managed-runtime search directories -- the normal case, since the + /// active runtime is itself a managed runtime -- the copy is reported + /// once, labelled `active-runtime`, not twice under both labels. + /// + /// This is the property `examine-finds-the-managed-runtimes-own-compilation-library`'s + /// step relies on: it accepts either label as proof the CLI's own + /// installed copy was found, specifically because a healthy host reports + /// `active-runtime` here and `managed-runtime` is unreachable once an + /// active runtime is present -- deduplication (keyed on the resolved + /// path) drops the later, identical hit before it can be recorded a + /// second time under the other source. + #[cfg(unix)] + #[test] + fn an_active_runtime_directory_that_is_also_a_managed_root_keeps_one_label() { + let dir = comgr_copy_dir("overlap"); + + let copies = find_library_copies( + COMGR_LIB_PREFIX, + std::slice::from_ref(&dir), + "", + None, + &[(dir.clone(), "managed-runtime", dir.clone())], + &std::collections::HashMap::new(), + ); + + assert_eq!( + copies.len(), + 1, + "one file found through two sources is one copy, not two: {copies:?}" + ); + assert_eq!( + copies[0].source, "active-runtime", + "the active-runtime hit comes first, so it keeps the label: {copies:?}" + ); + + 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, 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 = 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!( + 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 loader_cache_text = loader_cache_line_for(&second); + let two_copies = find_library_copies( + COMGR_LIB_PREFIX, + &[], + &only.to_string_lossy(), + Some(&loader_cache_text), + &[], + &std::collections::HashMap::new(), + ); + 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:?}" + ); + assert!( + note.contains(&two_copies[0].path), + "the note must name the one that would load: {note:?}" + ); + + let _ = std::fs::remove_dir_all(&only); + let _ = std::fs::remove_dir_all(&second); + } + + /// No copies anywhere produces the "no libamd_comgr found" note, naming + /// every place that was searched. + #[test] + fn no_copies_anywhere_names_every_place_searched() { + 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 = 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 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 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_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 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); + } + } + + /// The loader cache outranks a search directory, independent of any + /// `ld-library-path` hit. + /// + /// `ld_library_path_outranks_loader_cache_and_search_dirs` above already + /// orders all four tiers together, but a `LD_LIBRARY_PATH` hit sorts first + /// regardless of whether the *loader-cache* loop or the *search-dir* loop + /// 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_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 = find_library_copies( + COMGR_LIB_PREFIX, + &[], + "", + Some(&loader_cache_text), + &[(searched.clone(), "rocm-install", searched.clone())], + &std::collections::HashMap::new(), + ); + + assert_eq!( + copies.first().map(|copy| copy.source.as_str()), + Some("loader-cache"), + "the loader cache must be consulted ahead of the search directories: {copies:?}" + ); + assert_eq!(copies.len(), 2, "found: {copies:?}"); + + let _ = std::fs::remove_dir_all(&cached); + let _ = std::fs::remove_dir_all(&searched); + } + + /// `parse_ldconfig_cache_paths` reads the fixed `ldconfig -p` line format + /// without needing a real `ldconfig` on the machine running the test. + #[test] + fn ldconfig_cache_parsing_reads_only_matching_lines() { + let text = "2 libs found in cache `/etc/ld.so.cache'\n\ + \tlibamd_comgr.so.2 (libc6,x86-64) => /opt/rocm/lib/libamd_comgr.so.2\n\ + \tlibfoo.so.1 (libc6,x86-64) => /usr/lib/libfoo.so.1\n"; + let paths = parse_ldconfig_cache_paths(text, COMGR_LIB_PREFIX); + assert_eq!(paths, vec!["/opt/rocm/lib/libamd_comgr.so.2".to_owned()]); + } + + /// `parse_ldconfig_cache_paths` returns nothing when the cache lists no + /// match, rather than panicking on a header line with no `=>`. + #[test] + fn ldconfig_cache_parsing_handles_no_match() { + let text = "0 libs found in cache `/etc/ld.so.cache'\n"; + assert!(parse_ldconfig_cache_paths(text, COMGR_LIB_PREFIX).is_empty()); + } + + /// `rocm_install_siblings` finds every `rocm*`-named directory under the + /// directory it is given, sorted, and ignores everything else -- without + /// depending on what happens to live under the real `/opt` on the machine + /// running the test. + #[test] + fn rocm_install_siblings_finds_only_rocm_named_dirs_sorted() { + let opt = std::env::temp_dir().join(format!( + "rocm-opt-siblings-{}-{:?}", + std::process::id(), + std::thread::current().id() + )); + let _ = std::fs::remove_dir_all(&opt); + for name in ["rocm-6.4", "rocm-5.7", "not-rocm", "other"] { + std::fs::create_dir_all(opt.join(name)).unwrap(); + } + + let found = rocm_install_siblings(&opt); + assert_eq!( + found, + vec![opt.join("rocm-5.7"), opt.join("rocm-6.4")], + "must list only `rocm`-prefixed directories, sorted: {found:?}" + ); + + let _ = std::fs::remove_dir_all(&opt); + } + + /// A directory that does not exist (the common case: most hosts have no + /// `/opt`) contributes nothing rather than panicking. + #[test] + fn rocm_install_siblings_tolerates_a_missing_directory() { + let missing = std::env::temp_dir().join(format!( + "rocm-opt-missing-{}-{:?}", + std::process::id(), + std::thread::current().id() + )); + let _ = std::fs::remove_dir_all(&missing); + assert!(rocm_install_siblings(&missing).is_empty()); + } + /// A stand-in for a managed runtime's interpreter. /// /// It answers like a ROCm torch **only** when the runtime's library diff --git a/crates/rocm-core/src/fix.rs b/crates/rocm-core/src/fix.rs index ea443f3cf..ff7e87177 100644 --- a/crates/rocm-core/src/fix.rs +++ b/crates/rocm-core/src/fix.rs @@ -23,6 +23,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. @@ -694,6 +701,39 @@ const RECIPES: &[FixRecipe] = &[ applies_on: PRINT_ON_WSL, 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.", + // 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 `PRINT_ON_LINUX`: which copy the loader picks has nothing to do + // with the amdgpu module, and the two copies collide on WSL2 just the + // same. Print-only on both: neither way out can be chosen for the user. + applies_on: PRINT_ON_LINUX_AND_WSL, + runner: None, + }, FixRecipe { fix_id: "fix-19-shm-too-small", title: "Raise the shared memory allowance", @@ -2109,9 +2149,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"); } /// Restored, not new. This PR made the `wsl` arm of the platform lookup diff --git a/crates/rocm-core/src/lib.rs b/crates/rocm-core/src/lib.rs index 823dffea2..4bb8e43bd 100644 --- a/crates/rocm-core/src/lib.rs +++ b/crates/rocm-core/src/lib.rs @@ -2600,7 +2600,7 @@ pub(crate) fn is_wsl1_kernel(kernel_release: &str) -> bool { /// /// Search the conventional locations, and report "could not ask" as `None` /// rather than as an empty answer. -fn ldconfig_cache() -> Option { +pub(crate) fn ldconfig_cache() -> Option { for program in ["ldconfig", "/sbin/ldconfig", "/usr/sbin/ldconfig"] { if let Some(text) = capture_optional_command(program, &["-p"]) { return Some(text); @@ -4117,7 +4117,9 @@ fn push_existing_runtime_path(paths: &mut Vec, path: PathBuf) { paths.push(path); } -fn managed_therock_sdk_probe_candidates(registry_dir: &Path) -> Vec { +pub(crate) fn managed_therock_sdk_probe_candidates( + registry_dir: &Path, +) -> Vec { let Ok(entries) = fs::read_dir(registry_dir) else { return Vec::new(); }; @@ -4153,6 +4155,7 @@ fn managed_therock_sdk_probe_candidates(registry_dir: &Path) -> Vec Option { fn managed_sdk_ld_library_path(candidate: &TheRockSdkProbeCandidate) -> Option { let mut paths = Vec::new(); - collect_sdk_library_paths(&candidate.root_path, &mut paths); - if let Some(site_packages) = candidate.site_packages.as_deref() - && let Ok(entries) = fs::read_dir(site_packages) - { - for entry in entries.flatten() { - let path = entry.path(); - let Some(name) = path.file_name().and_then(|value| value.to_str()) else { - continue; - }; - if name.starts_with("_rocm_sdk_") { - collect_sdk_library_paths(&path, &mut paths); - } - } - } + collect_managed_runtime_library_paths( + &candidate.root_path, + candidate.site_packages.as_deref(), + &mut paths, + ); let wsl_lib = PathBuf::from("/usr/lib/wsl/lib"); if wsl_lib.is_dir() { paths.push(wsl_lib); @@ -4204,7 +4198,64 @@ fn managed_sdk_ld_library_path(candidate: &TheRockSdkProbeCandidate) -> Option) { +/// Every library directory a managed runtime keeps, given its root and the +/// `site-packages` its SDK recorded. +/// +/// One description of the layout, deliberately. A wheel-format runtime does not +/// keep its ROCm libraries under the root: they sit in a sibling `_rocm_sdk_*` +/// package inside `site-packages`, and a caller that walks the root alone sees +/// an empty runtime rather than a populated one. That is not a difference a +/// caller should have to remember, so it lives here and every search shares it. +pub(crate) fn collect_managed_runtime_library_paths( + root: &Path, + site_packages: Option<&Path>, + paths: &mut Vec, +) { + collect_sdk_library_paths(root, paths); + // Nothing to do when `site_packages` is `None`, and the absence of an + // `else` is deliberate rather than an oversight. + // + // `None` is not reachable from a real candidate today: `ROCM_SDK_PROBE_SCRIPT` + // has recorded `site_packages` unconditionally, outside its `try`, since the + // probe's initial version, so every manifest that parses at all carries + // `Some` here. `site_packages` stays `Option` because the field is + // `serde(default)` (a read-only probe cannot depend on a registry record + // being current), not because there is a real shape it needs to degrade + // gracefully for. + // + // There also is not a guess worth making if this were ever reached: `root` + // is one of the runtime's own `_rocm_sdk_*` package directories (the probe + // script's `_devel.get_devel_root()` result, or the first package root it + // found when there is no `devel` extra), never a venv root with a + // `root/lib//site-packages` layout underneath it to re-derive. A + // fallback that assumed that shape previously shipped here and could not + // have been exercised by any real record; removed along with its test + // rather than kept as a guess for a case that cannot arise. + if let Some(recorded) = site_packages { + collect_sdk_package_library_paths(recorded, paths); + } +} + +/// Library directories of the `_rocm_sdk_*` packages inside `site_packages`. +/// +/// They belong to the runtime that contains them, not to themselves. +fn collect_sdk_package_library_paths(site_packages: &Path, paths: &mut Vec) { + let Ok(entries) = fs::read_dir(site_packages) else { + return; + }; + for entry in entries.flatten() { + let path = entry.path(); + if path + .file_name() + .and_then(|value| value.to_str()) + .is_some_and(|name| name.starts_with("_rocm_sdk_")) + { + collect_sdk_library_paths(&path, paths); + } + } +} + +pub(crate) fn collect_sdk_library_paths(root: &Path, paths: &mut Vec) { for path in [ root.join("bin"), root.join("lib"), @@ -4294,11 +4345,19 @@ struct TheRockSdkProbeManifest { } #[derive(Debug, Clone)] -struct TheRockSdkProbeCandidate { +pub(crate) struct TheRockSdkProbeCandidate { installed_at_unix_ms: u128, - site_packages: Option, - root_path: PathBuf, + 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 { @@ -10525,6 +10584,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 ee3c279ce..62cdfdc10 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 an auto-applied fix 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 d60e6d52e..2ad54f6a2 100644 --- a/skills/rocm-doctor/reference.md +++ b/skills/rocm-doctor/reference.md @@ -93,7 +93,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 @@ -119,6 +119,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" | print-only | | `fix-15-msvc-redist` | windows | MSVC runtime missing (HIP DLLs can't load) | `vcruntime140.dll` / `vcruntime140_1.dll` missing | print-only | | `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` | print-only | +| `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 | print-only | | `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 | print-only | | `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 | print-only | | `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) | print-only | @@ -135,7 +136,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 b2b7960c3..8adde616e 100644 --- a/tests/e2e-cucumber/features/diagnose.feature +++ b/tests/e2e-cucumber/features/diagnose.feature @@ -476,3 +476,30 @@ Feature: Diagnosing failures and listing fixes Then the CLI either shows the whole report or says why it will not prepare one And the CLI states that nothing has been sent And the CLI prints the address to mail and a link, and starts nothing + + # 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-33 - 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 9f5ea08b5..32c7d4a2a 100644 --- a/tests/e2e-cucumber/features/examine.feature +++ b/tests/e2e-cucumber/features/examine.feature @@ -174,7 +174,6 @@ Feature: GPU detection and system inspection Given a managed runtime is active When the user inspects the system both for reading and for scripting Then the framework report names the runtime's interpreter - # EAI-8950. The text form repairs a lost registry entry from the install tree # before rendering (`recover_setup_runtime_registration`), so it names the # folder; `--json` skips that call because it writes, and used to answer @@ -222,3 +221,57 @@ Feature: GPU detection and system inspection Given a machine with an AMD GPU When the user inspects the system both for reading and for scripting Then it lists one AMD GPU per kernel GPU node, each with its PCI address and gfx target + + # HIP compiles device code at run time through a library a machine can hold + # more than one copy of — a system ROCm install and a ROCm Python wheel each + # ship one, and this CLI installs the second itself. When the copy that loads + # is not the one the active runtime needs, compilation fails with an error + # naming neither the library nor the second copy. Nothing looked past the + # first match before, so the second copy could not be seen at all. + # + # No GPU needed: the suite cannot install a second ROCm stack, so it cannot + # prove the two-copy case. What every lane can prove is that the inspection + # answers the question at all rather than staying silent, and that finding + # none is reported as a finding rather than a failure — which is the case + # the mock lane actually has. The two-copy behaviour is proven by unit tests + # that build the directory layout directly. + # + # `@requires-os:linux` because `probe_comgr` only runs for `os_family` + # "linux" or "wsl" (see `examine.rs`); on native Windows it is never called, + # so `comgr_paths: []` and `comgr_selected: null` would hold by nothing more + # than `Examination`'s own defaults, and the assertions below would pass + # whether the probe ran and found nothing or never ran at all. WSL reports + # `os_family` "linux" (see `expectation.rs`), so this still runs there. + @id:examine-reports-code-object-manager-copies @requires-os:linux + Scenario: examine-18 - The inspection says which code object manager libraries the machine holds + 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 + # on a host that already carries system ROCm ends up holding both copies + # having done nothing unusual. + # + # `@requires-gpu` because the precondition installs the SDK, and only a GPU + # lane does that. This is the half the unit tests cannot reach: they build the + # directory layout by hand, so they prove the search understands a layout we + # 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` + # 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 `.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 + When the user inspects the system in machine-readable form + Then the inspection attributes a code object manager library to that runtime diff --git a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs index 16b87ad9e..468fbec51 100644 --- a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs @@ -89,8 +89,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", ]; @@ -184,6 +183,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"; @@ -253,6 +257,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()); @@ -2148,3 +2157,41 @@ fn hostname_of_this_machine() -> String { .map(|s| s.trim().to_owned()) .unwrap_or_default() } + +#[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 5cc775850..58e9f2f0f 100644 --- a/tests/e2e-cucumber/tests/e2e/examine_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/examine_steps.rs @@ -473,6 +473,14 @@ async fn user_inspects_for_scripting_first(world: &mut E2eWorld) { world.cli_rc = Some(rc); } +#[when("the user inspects the system in machine-readable form")] +async fn user_inspects_for_scripting(world: &mut E2eWorld) { + let (stdout, stderr, rc) = crate::run_rocm(world, &["examine", "--json"]); + world.cli_output = Some(stdout); + world.cli_stderr = Some(stderr); + world.cli_rc = Some(rc); +} + #[when("the user inspects the system without probing frameworks")] async fn user_inspects_skipping_frameworks(world: &mut E2eWorld) { let (stdout, stderr, rc) = @@ -1013,3 +1021,255 @@ async fn assert_gpus_match_kfd_nodes(world: &mut E2eWorld) { ); } } + +#[then("the inspection lists the code object manager libraries it found")] +async fn assert_comgr_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("comgr_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:#}")); + // 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", "install_root"] { + assert!( + copy.get(field).is_some(), + "a reported copy is missing `{field}`, so a reader cannot tell \ + where it came from:\n{copy:#}" + ); + } + } +} + +#[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) { + let value = parsed_json(world); + let copies = value["comgr_paths"] + .as_array() + .expect("comgr_paths must be a list") + .clone(); + let selected = value + .get("comgr_selected") + .unwrap_or_else(|| panic!("the inspection never said which copy wins:\n{value:#}")); + + // The two have to agree. "Some copies exist but none was selected" would + // leave a reader unable to tell which one the loader picks, which is the + // only thing the list is for. + if copies.is_empty() { + assert!( + selected.is_null(), + "no copies were found, so none can have been selected:\n{selected:#}" + ); + // `comgr_paths: []` and `comgr_selected: null` are also what an + // `Examination` defaults to, so on their own they would pass whether + // the probe ran and found nothing or never ran at all -- exactly the + // state of every lane where this scenario is the only coverage. The + // "no libamd_comgr found" note is pushed only by the probe actually + // running and coming up empty (see `probe_comgr` in examine.rs), so + // requiring it here is what makes this assertion prove the probe ran. + 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 libamd_comgr found")), + "no code object manager 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. Reporting `null` here, rather than naming the first + // entry regardless of its tier, is the whole fix: a leftover + // `/opt/rocm-*` install or an inactive managed runtime must not be + // reported as "the library that loads" just because it was found. + 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!("the selected copy must name a path:\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("the inspection attributes a code object manager library to that runtime")] +async fn assert_managed_comgr_copy_reported(world: &mut E2eWorld) { + let value = parsed_json(world); + let copies = value["comgr_paths"] + .as_array() + .expect("comgr_paths must be a list") + .clone(); + + // The precondition installed a managed runtime, so one has to be there. + // Without this the assertion below is satisfied by a machine holding no + // copies at all, which is the state that hid this gap in the first place. + assert!( + !copies.is_empty(), + "a managed runtime is installed, so the inspection cannot report zero \ + code object manager libraries:\n{value:#}" + ); + + // `active-runtime`, not `managed-runtime`: the precondition makes this + // runtime the *active* one, and the search checks the active runtime's own + // directories first, ahead of the generic managed-runtime scan, so that is + // the label this copy gets -- the later `managed-runtime` hit for the same + // resolved file is the dedup's job to drop, not a second, differently + // labelled copy. Accepting either label is what proves "the CLI's own + // installed copy was found" without over-specifying which of the two + // overlapping sources happened to see it first. + assert!( + copies.iter().any(|copy| { + matches!( + copy.get("source").and_then(|s| s.as_str()), + Some("managed-runtime" | "active-runtime") + ) + }), + "the CLI installed this runtime and its ROCm wheels, so the search has to \ + find the copy it put there. Reported copies:\n{copies:#?}" + ); +} diff --git a/tests/e2e-cucumber/tests/skill_reference.rs b/tests/e2e-cucumber/tests/skill_reference.rs index 8f6719d04..a996402d4 100644 --- a/tests/e2e-cucumber/tests/skill_reference.rs +++ b/tests/e2e-cucumber/tests/skill_reference.rs @@ -66,6 +66,123 @@ fn catalog_rows(md: &str) -> Vec<(String, 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]`). +/// Rows whose marker is anything other than a plain `print-only`, which is the +/// complement of the set `SKILL.md`'s "the other N are print-only" counts. +/// +/// Deliberately not a count of "auto-applicable": the two docs do not agree on +/// what that phrase covers. `reference.md` says three ids are ever +/// auto-applicable and that `fix-9-igpu-dgpu` is not one of them, while +/// `SKILL.md` says four -- counting `needs-arg` and the windows-only exception +/// alongside the two plain `auto` entries. Counting the complement sidesteps +/// that disagreement and checks the claim `SKILL.md` actually makes, so this +/// guard does not quietly take a side in it. +fn catalog_not_print_only_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] != "print-only" + }) + .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 not_print_only = catalog_not_print_only_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 - not_print_only, + "skills/rocm-doctor/SKILL.md says {print_only_count} fixes are print-only, but the \ + table has {total} rows of which {not_print_only} carry some other marker \ + ({} expected)", + total - not_print_only + ); +} + #[test] fn catalog_os_scopes_use_the_cli_spellings() { // A shorthand such as `both` reads as "every platform" but cannot say