diff --git a/crates/rocm-core/src/diagnose.rs b/crates/rocm-core/src/diagnose.rs index e4652479c..2d8c3880d 100644 --- a/crates/rocm-core/src/diagnose.rs +++ b/crates/rocm-core/src/diagnose.rs @@ -208,6 +208,28 @@ const KEYWORDS_SHM_TOO_SMALL: KeywordTable = &[ ("no space left on device", 20, "the device reported full"), ]; +/// What a shadowed code object manager leaves in the error text. +/// +/// Every one of these is a *weak* signal on its own — a failed device-code +/// compilation has many causes, and this entry is established by the state of +/// the machine rather than by the words. The keywords only raise an already +/// structural finding; none of them reaches the match threshold alone. +const KEYWORDS_COMGR_CONFLICT: KeywordTable = &[ + (r"libamd_comgr", 40, "error mentions libamd_comgr"), + ("comgr", 30, "error mentions comgr"), + ( + "code object", + 25, + "error mentions a code object (what comgr produces)", + ), + ( + "hiperrornobinaryforgpu", + 25, + "HIP found no binary for the GPU", + ), + ("device code", 15, "error mentions device code (broad)"), +]; + const KEYWORDS_PATH_MISSING: KeywordTable = &[ ("rocminfo: command not found", 50, "rocminfo not on PATH"), ("command not found.*hipcc", 40, "hipcc not on PATH"), @@ -1399,6 +1421,113 @@ 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. `rocm + // serve` puts the managed runtime's libraries first on purpose, and gets a + // different -- correct -- answer. This finding is about the plain shell the + // user's own command ran in. + evidence.push( + "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 fix = Fix { + summary: + "Make the code object manager and the HIP runtime come from the same installation." + .to_owned(), + commands: vec![ + format!("# The library that would load: {}", comgr.real_path), + format!("# The runtime that would load: {}", hip.real_path), + format!("# The runtime's own copy: {}", matching.real_path), + "# Either keep one stack and remove the other, or order the search".to_owned(), + "# path so the runtime's own copy is found first:".to_owned(), + format!( + "export LD_LIBRARY_PATH=\"{}:$LD_LIBRARY_PATH\"", + std::path::Path::new(&matching.real_path) + .parent() + .map_or_else(String::new, |dir| dir.to_string_lossy().into_owned()) + ), + ], + fix_id: ID.to_owned(), + auto_applicable: false, + verify: "python -c \"import torch; torch.zeros(1, device='cuda')\"".to_owned(), + notes: vec![crate::fix::COMGR_CONFLICT_NEITHER_OPTION_RECOMMENDED.to_owned()], + ..Fix::default() + }; + finalize(ID, TITLE, score, evidence, fix) +} + /// The vLLM engine-startup import failure (EAI-8012). /// /// Keyword-only, and not by preference. The fact that decides this failure is @@ -2058,11 +2187,16 @@ const CHECKERS: &[Checker] = &[ (check_wsl_5_distro_too_old, WSL_ONLY), (check_wsl_6_host_driver_too_old, WSL_ONLY), (check_wsl_7_wsl1, WSL_ONLY), - // Both families, opting in explicitly as the platform split requires. The - // shortage has nothing to do with `amdgpu` or `/dev/kfd` -- it is the size - // of a tmpfs -- and WSL2 ships the same 64 MB default a container does, so - // leaving this tagged `linux` alone would silence it on one of the two - // platforms most likely to have it. + // Both families, opting in explicitly as the platform split requires. Which + // copy of a library the loader picks is not a question about the amdgpu + // module or the Windows host driver -- a wheel copy and a system copy + // collide on WSL2 exactly as they do on bare metal. Windows is deferred + // until this has proved the approach. + (check_18_comgr_conflict, &["linux", "wsl"]), + // Both families, for the same reason. The shortage has nothing to do with + // `amdgpu` or `/dev/kfd` -- it is the size of a tmpfs -- and WSL2 ships the + // same 64 MB default a container does, so leaving this tagged `linux` alone + // would silence it on one of the two platforms most likely to have it. (check_19_shm_too_small, &["linux", "wsl"]), ]; @@ -2535,6 +2669,70 @@ 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(), + } + } + + fn comgr_finding(report: &DiagnoseReport) -> Option<&Diagnosis> { + report + .matched + .iter() + .find(|d| d.id == "fix-18-comgr-conflict") + } + + #[test] + fn a_code_object_manager_from_another_installation_is_reported() { + // The failure this entry exists for: the runtime resolves to the system + // install while the library that compiles device code for it comes from + // a wheel, and the error the user sees names neither. + let mut e = linux_base(); + let wheel = copy_in("/wheel", "/wheel/lib/libamd_comgr.so.2", "2.8.0"); + let system = copy_in("/opt/rocm", "/opt/rocm/lib/libamd_comgr.so.3", "3.0.0"); + e.comgr_selected = Some(wheel.clone()); + e.comgr_paths = vec![wheel, system]; + e.hip_selected = Some(copy_in( + "/opt/rocm", + "/opt/rocm/lib/libamdhip64.so.6", + "6.4", + )); + + let report = diagnose(&e, ""); + let finding = comgr_finding(&report).expect("the mismatch must be reported"); + + assert!( + finding.score >= MIN_SCORE_FOR_MATCH, + "a mismatch established from machine state alone has to clear the \ + threshold without help from the symptom text: {}", + finding.score + ); + let fix = finding.fix.as_ref().expect("the finding must carry a plan"); + assert!( + !fix.auto_applicable, + "both remedies can break a working Python environment, so nothing here \ + may be applied for the user" + ); + let evidence = finding.evidence.join("\n"); + for expected in [ + "/wheel/lib/libamd_comgr.so.2", + "/opt/rocm", + "2.8.0", + "3.0.0", + ] { + assert!( + evidence.contains(expected), + "the report has to name each copy, where it came from and its \ + version -- `{expected}` is missing:\n{evidence}" + ); + } + } + #[test] fn a_partly_used_allowance_reports_what_is_left_as_well_as_the_size() { // The two numbers answer different questions, and the evidence only @@ -2578,6 +2776,40 @@ mod tests { ); } + #[test] + fn a_healthy_managed_installation_raises_no_report() { + // The control, and the reason the rule is stated symmetrically. A managed + // runtime spreads its libraries across separate `_rocm_sdk_*` packages + // that sit *beside* each other under one `site-packages` -- not nested + // under the runtime's own root (`_rocm_sdk_devel`) at all -- so the two + // copies sit in different directories of ONE installation. Anything + // that decided ownership by walking up from the file would call these + // two installations and fire on the most common install we ship. (A + // fixture nesting `_rocm_sdk_core` under the root, the shape the + // installer never produces, would pass by construction without + // proving that -- this one matches `apps/rocm/src/therock.rs`'s + // `ROCM_SDK_PROBE_SCRIPT` instead.) + let mut e = linux_base(); + let root = "/data/runtimes/therock/_rocm_sdk_devel"; + let comgr = copy_in( + root, + "/data/runtimes/therock/_rocm_sdk_core/lib/libamd_comgr.so.2", + "2.8.0", + ); + e.comgr_selected = Some(comgr.clone()); + e.comgr_paths = vec![comgr]; + e.hip_selected = Some(copy_in( + root, + "/data/runtimes/therock/_rocm_sdk_devel/lib/libamdhip64.so.6", + "6.4", + )); + + assert!( + comgr_finding(&diagnose(&e, "")).is_none(), + "both libraries come from one installation, so there is no conflict" + ); + } + #[test] fn an_unmeasured_shared_memory_allowance_is_not_reported() { // Unknown is not zero. `None` means the path was absent or the query @@ -2593,6 +2825,31 @@ mod tests { ); } + #[test] + fn a_second_copy_that_does_not_load_raises_no_report() { + // Holding two copies is not a fault. Only the one that loads matters. + let mut e = linux_base(); + let winner = copy_in("/opt/rocm", "/opt/rocm/lib/libamd_comgr.so.3", "3.0.0"); + let loser = copy_in("/wheel", "/wheel/lib/libamd_comgr.so.2", "2.8.0"); + e.comgr_selected = Some(winner.clone()); + e.comgr_paths = vec![winner, loser]; + e.hip_selected = Some(copy_in( + "/opt/rocm", + "/opt/rocm/lib/libamdhip64.so.6", + "6.4", + )); + + assert!( + comgr_finding(&diagnose(&e, "")).is_none(), + "the copy that loads belongs to the runtime, so nothing is wrong" + ); + assert_eq!( + e.comgr_paths.len(), + 2, + "the machine report still lists both copies; only the diagnosis stays quiet" + ); + } + #[test] fn being_in_a_container_raises_the_finding_without_creating_it() { // The container flag says the cause and the remedy are known exactly, so @@ -2624,6 +2881,154 @@ 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 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..104485758 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,54 @@ 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 four, 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 loader search order. The first is the one that would + /// load. + #[serde(default)] + pub comgr_paths: Vec, + /// The copy the loader would pick. `None` when none were found. + #[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 loader search order. + /// + /// 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 the loader would pick. `None` when none were found. + #[serde(default)] + pub hip_selected: Option, + // HIP SDK install (Windows) pub hip_sdk_path: String, pub hip_sdk_version: String, @@ -303,6 +387,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 +479,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 +497,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,36 +2171,521 @@ 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"; + +/// Find every copy of the code object manager library, in loader search order. +/// +/// The first entry is the copy that would load. Every other entry is the part +/// that was invisible before: a machine can hold a system copy and a wheel copy, +/// and when the one that loads does not belong to the active runtime, device +/// code compilation fails with an error naming neither. +/// +/// `interpreter`'s `library_paths` -- the active managed runtime's own library +/// directories, when there is one -- are checked **first**, ahead of this +/// process's own `LD_LIBRARY_PATH`. That is not this process's search order; it +/// is a served child's. `rocm serve`/`rocm chat` prepend exactly those +/// directories onto `LD_LIBRARY_PATH` before launching the engine (see +/// `lemonade_process_environment_vars` and its vLLM counterpart), so they win +/// over whatever this process's own environment or the system loader cache +/// would otherwise resolve to. Reporting the plain-environment answer instead +/// would name the system's copy as "the one that would load" on a machine +/// where every process the CLI actually launches loads the wheel's. +/// +/// **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. + 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, + ); + + if let Some(first) = comgr.first() { + e.comgr_version.clone_from(&first.version); + e.comgr_selected = Some(first.clone()); + if let Some(note) = multi_copy_note(COMGR_LIB_PREFIX, &comgr) { + e.notes.push(note); } - if hit.is_some() { - break; + } else { + e.notes.push(format!( + "no {COMGR_LIB_PREFIX} found on the library path, in the loader cache, or in any known ROCm install" + )); + } + + // Same distinction as the comgr branch above, and for the same reason: with + // no note here, "no libamdhip64 found" and "probe_comgr never ran" would be + // indistinguishable from the JSON alone, and a scenario asserting only + // `hip_paths: []` / `hip_selected: null` would pass in either case. + if hip.is_empty() { + e.notes.push(format!( + "no {HIP_RUNTIME_LIB_PREFIX} found on the library path, in the loader cache, or in any known ROCm install" + )); + } + + e.comgr_matches_runtime = comgr_matches_runtime(&comgr, comgr.first(), hip.first()); + e.hip_selected = hip.first().cloned(); + 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 "N copies ... would load" note for `copies`, naming the one that wins, +/// or `None` for zero or one -- holding exactly one copy is not a fault, and +/// `probe_comgr` pushes its own "no ... found" note for zero (it has the +/// prefix's full "on the library path, in the loader cache, or in any known +/// ROCm install" phrasing to add, which does not belong on this answer). +fn multi_copy_note(prefix: &str, copies: &[LibraryCopy]) -> Option { + let first = copies.first()?; + (copies.len() > 1).then(|| { + format!( + "{} copies of {prefix} found; {} would load", + copies.len(), + first.path + ) + }) +} + +/// 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 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 let Some(hit) = hit { - e.hip_libs_on_ld_path = Some(true); - e.notes - .push(format!("libamdhip64 visible via LD_LIBRARY_PATH: {hit}")); + 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); + } + } + 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 /// truncated. Slices on char boundaries (matching Python's `value[:n]`); a byte /// slice would panic when the cut lands inside a multibyte character. @@ -2748,6 +3330,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 +3619,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 +3728,392 @@ 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 wheel runtime the registry has no record for is still found. + /// + /// The recorded `site-packages` is the better answer and is preferred, but a + /// read-only probe cannot depend on a record being present and current. A + /// host whose registry is missing or stale is exactly the kind this command + /// is called on, and reporting no copies there would read as a machine with + /// nothing wrong rather than a machine we failed to inspect. With no record, + /// `root`'s own `_rocm_sdk_` prefix is the only thing that tells the search + /// its parent is a site-packages directory worth scanning for siblings. + #[cfg(target_os = "linux")] + #[test] + fn a_wheel_runtime_with_no_registry_record_is_still_found() { + let (root, recorded) = wheel_runtime_on_disk("norecord"); + let expected = recorded.clone().unwrap().join("_rocm_sdk_core").join("lib"); + + // Non-vacuity: the same runtime, with its record, resolves to the same + // directory. Without this the assertion below could pass because the + // fallback found something else entirely. + assert!( + { + let mut d = Vec::new(); + crate::collect_managed_runtime_library_paths(&root, recorded.as_deref(), &mut d); + d.contains(&expected) + }, + "the recorded path must resolve first, or this test is not comparing like with like" + ); + + let mut found = Vec::new(); + crate::collect_managed_runtime_library_paths(&root, None, &mut found); + assert!( + found.contains(&expected), + "with no record to read, the sibling package beside root's own `_rocm_sdk_` \ + directory has to carry it: {found:?}" + ); + 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. + /// + /// Paired with the test above so "finds the wheel copy" cannot be satisfied + /// by a search that simply returns every directory it is handed. + #[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. + 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 + } + + /// 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. + 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. + #[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(), + ); + + 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!( + 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". + #[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(), + ); + + assert_eq!( + copies.first().map(|copy| ©.source), + Some(&"ld-library-path".to_owned()), + "found: {copies:?}" + ); + 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); + } + + /// `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, &[], &Default::default()) + .is_empty() + ); + } + + /// More than one copy produces the "N copies ... would load" note; exactly + /// one copy, or none, produces no note at all. + #[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(), + ); + assert_eq!(one_copy.len(), 1); + assert_eq!( + multi_copy_note(COMGR_LIB_PREFIX, &one_copy), + None, + "a single copy must not be reported as a conflict: {one_copy:?}" + ); + assert_eq!( + multi_copy_note(COMGR_LIB_PREFIX, &[]), + None, + "no copies is not a multi-copy conflict either" + ); + + 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 note = multi_copy_note(COMGR_LIB_PREFIX, &two_copies) + .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); + } + + /// `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 dc056566f..2171147ee 100644 --- a/crates/rocm-core/src/fix.rs +++ b/crates/rocm-core/src/fix.rs @@ -22,6 +22,13 @@ use std::time::Duration; const RUN_TIMEOUT: Duration = Duration::from_mins(1); const QUERY_TIMEOUT: Duration = Duration::from_secs(8); +/// Shared verbatim by this recipe's own note and `diagnose.rs`'s +/// `check_18_comgr_conflict` evidence, which states the same claim in its own +/// words at the point the real paths are known. A second statement of one +/// claim is a second thing to keep correct, and the two had already drifted +/// apart in wording before they shared this constant. +pub(crate) const COMGR_CONFLICT_NEITHER_OPTION_RECOMMENDED: &str = "Neither option is recommended over the other: which is right depends on which stack you mean to keep, and removing the wrong one breaks a working environment."; + /// Print a failure explanation to stderr, ignoring write failures (closed /// stderr, full disk) so an I/O error while explaining a failure can't itself /// panic the process. @@ -585,6 +592,39 @@ const RECIPES: &[FixRecipe] = &[ applies_on: WSL_ONLY, runner: None, }, + FixRecipe { + fix_id: "fix-18-comgr-conflict", + title: "Code object manager library does not belong to the active HIP runtime", + rationale: "HIP compiles device code at run time through libamd_comgr, and this machine holds more than one copy of it. The copy the loader picks belongs to a different installation than the HIP runtime that loads, so compilation fails with an error that names neither the library nor the second copy. A second copy is not itself a fault -- many correct installations hold one -- so what is reported here is specifically the mismatch.", + auto_applicable: false, + // No repair, and no recommendation between the two. Removing a stack or + // reordering the search path can each break a working Python + // environment, and which is right depends on which stack the user means + // to keep -- a question only they can answer. `rocm diagnose` fills in + // the real paths for this machine; these are the shapes. + commands: &[ + "# Find every copy and which one loads:", + "rocm examine --json # read comgr_paths, comgr_selected, hip_selected", + "# Then pick ONE of the following. They are alternatives, not steps.", + "# (a) Keep the system installation: remove or uninstall the wheel that", + "# supplies the second copy.", + "# (b) Keep the wheel: order the search path so the wheel's own copy of", + "# both libraries is found first, making the wheel the active runtime.", + "export LD_LIBRARY_PATH=\":$LD_LIBRARY_PATH\"", + ], + needs_sudo: false, + needs_reboot: false, + needs_relogin: false, + verify: "python -c \"import torch; torch.zeros(1, device='cuda')\"", + notes: &[ + COMGR_CONFLICT_NEITHER_OPTION_RECOMMENDED, + "This describes the environment outside the CLI's managed runtimes. `rocm serve` puts a managed runtime's libraries first on purpose, so inside one the wheel copy wins by design and that is correct.", + ], + // Not `LINUX_ONLY`: which copy the loader picks has nothing to do with + // the amdgpu module, and the two copies collide on WSL2 just the same. + applies_on: LINUX_AND_WSL, + runner: None, + }, FixRecipe { fix_id: "fix-19-shm-too-small", title: "Raise the shared memory allowance", @@ -1766,9 +1806,9 @@ mod tests { let count = ids.len(); ids.dedup(); assert_eq!(ids.len(), count, "duplicate fix-id in RECIPES"); - // 17 bare-metal/Windows entries (fix-17 and fix-19 among them) plus the - // 7 WSL ones. - assert_eq!(count, 24, "expected 24 catalog entries"); + // 18 bare-metal/Windows entries (fix-17, fix-18 and fix-19 among them) + // plus the 7 WSL ones. + assert_eq!(count, 25, "expected 25 catalog entries"); } #[test] diff --git a/crates/rocm-core/src/lib.rs b/crates/rocm-core/src/lib.rs index c9d096f50..198d26811 100644 --- a/crates/rocm-core/src/lib.rs +++ b/crates/rocm-core/src/lib.rs @@ -2591,7 +2591,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); @@ -4108,7 +4108,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(); }; @@ -4144,6 +4146,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); @@ -4195,7 +4189,68 @@ 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); + match site_packages { + Some(recorded) => collect_sdk_package_library_paths(recorded, paths), + // No record to read. A read-only probe cannot depend on one being + // present and current, and a host whose registry is missing or stale is + // exactly the kind this is called on: reporting nothing there would read + // as a machine with nothing wrong rather than one we failed to inspect. + // + // `root` is never a venv root to search downward from -- it 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). Its sibling packages sit + // beside it, under its *parent*, which is the site-packages directory + // every one of them shares. A `root/lib//site-packages` guess + // never exists under a package directory, so it always found nothing; + // `root.parent()` is the one guess this shape actually supports. + None => { + if root + .file_name() + .and_then(|name| name.to_str()) + .is_some_and(|name| name.starts_with("_rocm_sdk_")) + && let Some(parent) = root.parent() + { + collect_sdk_package_library_paths(parent, 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"), @@ -4285,11 +4340,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 { @@ -10516,6 +10579,57 @@ Class Name: Display Ok(()) } + /// `managed_therock_sdk_probe_candidates` surfaces the SDK's own recorded + /// `library_paths` rather than dropping them. + /// + /// Those are what `examine`'s comgr/HIP search now reads to find a managed + /// runtime's libraries (see `known_install_roots`/`install_library_dirs` in + /// `examine.rs`), in place of re-deriving the layout from `root_path` and + /// `site_packages` after the fact -- a guess that does not hold for every + /// real install shape, which is what left a managed runtime's own code + /// object manager library unseen on a real host + /// (`examine-finds-the-managed-runtimes-own-compilation-library`). A + /// candidate whose `library_paths` came back empty would defeat that fix + /// silently, so this pins the field surviving the read. + #[test] + fn managed_sdk_probe_candidate_carries_recorded_library_paths() -> Result<()> { + let (root, paths) = temp_app_paths("managed-sdk-library-paths"); + let registry = paths.data_dir.join("runtimes").join("registry"); + let site_packages = root.join("site-packages"); + let sdk_root = site_packages.join("_rocm_sdk_devel"); + let sdk_bin = sdk_root.join("bin"); + let comgr_dir = site_packages.join("_rocm_sdk_core").join("lib"); + fs::create_dir_all(&sdk_bin)?; + fs::create_dir_all(&comgr_dir)?; + fs::create_dir_all(®istry)?; + fs::write( + registry.join("runtime.json"), + serde_json::to_vec_pretty(&serde_json::json!({ + "runtime_id": "therock-release:gfx120X-all", + "family": "gfx120X-all", + "installed_at_unix_ms": 10, + "rocm_sdk": { + "import_ok": true, + "site_packages": site_packages, + "root_path": sdk_root, + "bin_path": sdk_bin, + "library_paths": [comgr_dir] + } + }))?, + )?; + + let candidates = managed_therock_sdk_probe_candidates(®istry); + assert_eq!(candidates.len(), 1, "expected exactly one candidate"); + assert_eq!( + candidates[0].library_paths, + vec![comgr_dir], + "the recorded library_paths must survive into the candidate, or the \ + comgr/HIP search has nowhere else reliable to find them" + ); + fs::remove_dir_all(root).ok(); + Ok(()) + } + #[test] fn managed_sdk_probe_skips_non_therock_manifests() -> Result<()> { let (root, paths) = temp_app_paths("managed-sdk-skip-non-therock"); diff --git a/docs/wsl.md b/docs/wsl.md index da9974fe7..3c56f7882 100644 --- a/docs/wsl.md +++ b/docs/wsl.md @@ -200,13 +200,18 @@ The WSL entries, in the order a broken stack usually reveals them: | `fix-wsl-5-distro-too-old` | The distro release is below the floor in the prerequisites above | | `fix-wsl-6-host-driver-too-old` | The distro-side plumbing is complete but the Windows host driver is missing or too old | -One entry outside this list also applies here. `fix-19-shm-too-small` is not a +Two entries outside this list also apply here. `fix-19-shm-too-small` is not a WSL entry, but WSL2 ships the same 64 MiB `/dev/shm` a container does, and a serving workload needs gigabytes of it. It reports below 1 GiB, so a WSL2 user can meet it on an otherwise healthy stack. Its guidance names the container and bare-metal cases; under WSL2 the remedy is the host one, remounting `/dev/shm` larger and adding the matching `/etc/fstab` line inside the distro. +`fix-18-comgr-conflict` also applies here: a wheel copy and a system copy of +the code object manager library collide on WSL2 exactly as they do on bare +metal, through the same `LD_LIBRARY_PATH`/loader-cache search, so the finding +is not specific to either platform. + Every WSL remedy is print-only. `rocm fix ` shows the commands and does not run them: they either install packages with `sudo`, edit loader configuration, or belong to the Windows host, and none of that meets the bar the four diff --git a/tests/e2e-cucumber/features/diagnose.feature b/tests/e2e-cucumber/features/diagnose.feature index d33c22e74..9169660f5 100644 --- a/tests/e2e-cucumber/features/diagnose.feature +++ b/tests/e2e-cucumber/features/diagnose.feature @@ -289,3 +289,29 @@ Feature: Diagnosing failures and listing fixes When the user previews that fix without applying it Then the preview states that the fix requires sudo and a re-login And the preview states that the CLI can run it automatically + # HIP compiles device code at run time through a library a machine can hold + # more than one copy of. When the copy that loads belongs to a different + # installation than the runtime, compilation fails with an error naming + # neither. Both remedies — remove one stack, or reorder the search path — can + # break a working Python environment, and which is right depends on which + # stack the user means to keep. So the CLI states them and changes nothing. + # + # The conflict itself cannot be provoked here: the suite cannot install a + # second ROCm stack, and the detection rule is proven by unit tests that build + # the machine state directly. What this pins is the half that matters if the + # entry ever stops being advisory — that asking for it changes nothing and + # recommends neither option. + # + # `@requires-os:linux` because `fix-18-comgr-conflict` is registered for + # `["linux", "wsl"]` (comgr and LD_LIBRARY_PATH are POSIX-loader concepts, not + # Windows ones). Unlike diagnose-20's preview, this step applies the fix for + # real, so it goes through the fix's own platform gate and would be refused + # for the wrong reason -- "wrong OS", not "advisory" -- on a native Windows + # lane. `@requires-os:linux` matches WSL2 too, which is where this fix does + # apply. + @id:diagnose-fix-comgr-conflict-is-advisory-only @requires-os:linux + Scenario: diagnose-21 - The fix for a shadowed compilation library changes nothing and recommends nothing + Given a user who has chosen the fix for a shadowed compilation library + When the user asks the CLI to apply that fix + Then the CLI explains that it will not make the change itself + And the CLI offers both options without ranking them diff --git a/tests/e2e-cucumber/features/examine.feature b/tests/e2e-cucumber/features/examine.feature index 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 2c59e6de1..c8b29f12f 100644 --- a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs @@ -88,8 +88,7 @@ const CATALOG_FIX_IDS: &[&str] = &[ "fix-wsl-5-distro-too-old", "fix-wsl-6-host-driver-too-old", "fix-wsl-7-wsl1", - // `fix-18` is the code object manager entry on its own branch, kept - // distinct for the same reason `fix-16` is. + "fix-18-comgr-conflict", "fix-19-shm-too-small", ]; @@ -137,6 +136,11 @@ const fn fix_id_for_the_other_os() -> &'static str { } } +/// The entry for a code object manager library belonging to a different +/// installation than the active runtime. Advisory by design: both remedies can +/// break a working Python environment. +const COMGR_CONFLICT_FIX_ID: &str = "fix-18-comgr-conflict"; + /// Contents planted in the scenario's own shell rc file. The assertion is that /// this survives the run byte for byte. const PLANTED_RC: &str = "# planted by the e2e suite; the fix must not touch this\n"; @@ -206,6 +210,11 @@ async fn user_chose_fix_needing_sudo_and_relogin(world: &mut E2eWorld) { world.model_name = Some(COMMAND_FAILURE_FIX_ID.to_string()); } +#[given("a user who has chosen the fix for a shadowed compilation library")] +async fn user_chose_comgr_conflict_fix(world: &mut E2eWorld) { + world.model_name = Some(COMGR_CONFLICT_FIX_ID.to_string()); +} + #[given("a user who names a fix the CLI does not offer")] async fn user_named_unknown_fix(world: &mut E2eWorld) { world.model_name = Some("fix-does-not-exist".to_string()); @@ -1133,3 +1142,41 @@ async fn assert_command_failure_reported_on_stderr(world: &mut E2eWorld) { "the command-failure explanation must not also be on stdout:\n{stdout}" ); } + +#[then("the CLI explains that it will not make the change itself")] +async fn assert_fix_is_advisory(world: &mut E2eWorld) { + let output = world.cli_output.as_ref().expect("no fix output"); + assert_eq!( + world.cli_rc, + Some(0), + "printing advice is not a failure:\n{output}" + ); + assert!( + output.contains("print-only") || output.contains("will NOT run it"), + "the user has to be told the CLI is not going to do this for them:\n{output}" + ); +} + +#[then("the CLI offers both options without ranking them")] +async fn assert_both_options_unranked(world: &mut E2eWorld) { + let output = world.cli_output.as_ref().expect("no fix output"); + // Both remedies have to be present. Offering one is a recommendation by + // omission, and the wrong one breaks a working environment. + // + // Keyed on the option markers rather than on words like "remove", which also + // occur in the surrounding prose -- an assertion that matched those would + // still pass with one of the two options deleted, which is exactly the + // regression it exists to catch. + for option in ["(a)", "(b)"] { + assert!( + output.contains(option), + "only one way out was offered; option `{option}` is missing, which makes \ + the other a recommendation by omission:\n{output}" + ); + } + assert!( + output.contains("Neither option is recommended"), + "the CLI has to say it is not choosing between them -- which is right \ + depends on which stack the user means to keep:\n{output}" + ); +} diff --git a/tests/e2e-cucumber/tests/e2e/examine_steps.rs b/tests/e2e-cucumber/tests/e2e/examine_steps.rs index 5cc775850..b44e577e8 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,179 @@ 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:#}" + ); + } + } +} + +#[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 { + 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:#}")); + 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 { + 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:#}")); + 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:#}" + ); + + assert!( + copies + .iter() + .any(|copy| copy.get("source").and_then(|s| s.as_str()) == Some("managed-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:#?}" + ); +}