feat(examine): report every code object manager library, not just the first - #384
volen-silo wants to merge 1 commit into
Conversation
4e5fe59 to
6f617c8
Compare
6f617c8 to
09476f8
Compare
siloteemu
left a comment
siloteemu
left a comment
There was a problem hiding this comment.
🔴 Automated review · pr-review-watcher · 09476f8
This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.
Summary
Adds a Linux probe that records every libamd_comgr copy on the machine (path, resolved path, version, source), plus four new Examination JSON fields, four unit tests and one Gherkin scenario — outcome: Needs work (one blocking issue). Verified: ran the rocm-core examine test module (41 pass) on a throwaway copy and mutated it per branch — reverting the multi-hit walk, the resolve-path dedup and the digits-only version filter each fails exactly one test, but removing the intra-directory sort(), flipping copies.first() to .last(), and gutting probe_comgr to an immediate return all leave every test green; confirmed examine --json flattens Examination so the new keys really surface, that no other consumer reads the new fields or hip_libs_on_ld_path, that the managed-runtime layout claim matches runtime.rs, and that no comgr probe existed on the base at all (so the title's "not just the first" describes a new field rather than a widened one). Checks at review time: 24 success, 2 failures, 1 skipped, 1 cancelled. Blocking: 1 · Non-blocking: 4.
🚫 Blocking (must fix before merge)
crates/rocm-core/src/examine.rs:1982 — comgr_paths_in_loader_cache gates on which("ldconfig"), and which (same file, ~line 616) walks only the process PATH with no /sbin fallback. This re-introduces a bug the same crate already fixed and documented: crates/rocm-core/src/lib.rs:2543-2560 defines ldconfig_cache() whose doc comment says in as many words that "ldconfig lives in /sbin, which is not on a non-root user's PATH on Debian and derivatives. Looking it up by bare name there yields nothing, and an empty cache is indistinguishable from a cache that does not list the library" — and records the user-visible harm it caused last time (a correctly installed library read as "not registered with the linker"). Its fix is to try ["ldconfig", "/sbin/ldconfig", "/usr/sbin/ldconfig"].
Why it blocks: on those hosts the whole loader-cache tier silently contributes nothing, and that tier is the one that finds the copy the loader actually resolves. The consequences are exactly the ones this feature exists to prevent — a copy registered only in ld.so.cache (a ROCm install outside /opt, or one whose directory is on ld.so.conf but not LD_LIBRARY_PATH) is either missed entirely, producing the "no libamd_comgr found …" note on a machine that has one, or is ranked below a rocm-install/managed-runtime entry so comgr_selected names the wrong copy. It also silently degrades rather than reporting "could not ask", which is the distinction the sibling function was written to preserve. AGENTS.md §5 requires checking sibling implementations before adding a probe.
Fix: drop the which gate and reuse the existing helper — make ldconfig_cache() pub(crate) (the same treatment this PR already applies to collect_sdk_library_paths) and parse its Option<String>, treating None as "could not ask" rather than "no copies". It is timeout-bounded (capture_optional_command → OPTIONAL_COMMAND_TIMEOUT), so this does not trade away the 5s bound the current run(..., SHORT) gives. A smaller variant — mirroring the three-candidate list locally — works too but leaves two descriptions of the same trap to keep in sync.
Non-blocking
crates/rocm-core/src/examine.rs:236,239,1875— "in loader search order" / "the copy the loader would pick" overclaims: therocm-installandmanaged-runtimetiers are not loader search locations at all unless they also appear onLD_LIBRARY_PATH/ld.so.conf, yet the hedge names onlyRUNPATH,ld.so.preloadand container remapping; one sentence saying the last two tiers are evidence of what exists rather than of what loads would make thesourcefield's purpose explicit and keep the future conflict rule honest.crates/rocm-core/src/examine.rs:1889-1917—probe_comgritself has no test: replacing its body with an immediatereturnleaves all 41 examine tests green, and the new scenario passes too (the fields serialise as[]/nullfromDefault), so on every lane without ROCm present the scenario proves only that two keys exist; thecopies.first()selection rule and the multi-copy note are unproven, and a variant taking its search roots as a parameter would make both testable.crates/rocm-core/src/examine.rs:1861— deletingmatches.sort()fails nothing, so the "sorted so the result does not depend on directory iteration order" promise has no test; the ordering test uses two separate directories and never exercises intra-directory order.crates/rocm-core/src/examine.rs:457— the comment justifying the call site says the search consultsLD_LIBRARY_PATH"(read byprobe_env)", butprobe_comgrre-reads the variable itself and uses nothingprobe_envstored; only theprobe_rocm_installhalf of the stated ordering dependency is real.crates/rocm-core/src/examine.rs:1907— the "N copies … would load" note fires on any host holding a system ROCm install alongside a runtime this CLI installed, which the commit message itself calls the normal case; harmless whileExamination.notesis JSON-only (no consumer prints it today), but it will read as a warning the moment one does.
juhovainio
left a comment
juhovainio
left a comment
There was a problem hiding this comment.
Nice write-up, and the reasoning behind the non-obvious calls (no dlopen, resolved-path dedup, WSL2 inclusion) all held up.
One thing I think is a real gap: the managed-runtime search path doesn't look like it actually covers the wheel-install case this PR is meant to catch. managed_sdk_ld_library_path (the existing helper) finds a wheel's comgr copy by walking site_packages for _rocm_sdk_* siblings, because the actual libraries live in a sibling package, not under the root it's handed. comgr_search_dirs's new managed_runtime_roots() path reuses collect_sdk_library_paths on the root directly but skips that site_packages walk entirely - so for a wheel-format install this probe likely never finds the copy that's the PR's own headline scenario. Left an inline comment at the spot.
Two smaller notes, not blocking:
- The
/opt/rocm*sibling-install scan isn't mentioned in the PR description, which frames this as strictly system-vs-wheel. Worth a line in the write-up. collect_libraries_in_dirnow sorts before taking the first match, which does change the HIP probe's tie-break order when a directory has multiple matching files - minor, but the Risk section's "unchanged" claim isn't quite true for that edge case.
Also a couple of judgement-call code-smell notes worth a look when convenient (not blockers): the same 4-line "Unix-only" test comment is duplicated across three tests in examine.rs, and ComgrCopy.source is a closed 4-value set that reads like it wants to be an enum rather than a bare String.
| // managed runtime keeps its libraries is a second thing to keep correct. | ||
| for root in managed_runtime_roots() { | ||
| let mut paths = Vec::new(); | ||
| crate::collect_sdk_library_paths(&root, &mut paths); |
There was a problem hiding this comment.
This only walks root via collect_sdk_library_paths. The existing managed_sdk_ld_library_path (lib.rs) additionally walks candidate.site_packages for _rocm_sdk_* sibling packages, because that's where the actual libraries live for a wheel install - this loop doesn't have (and doesn't look up) an equivalent site_packages value for managed_runtime_roots()'s roots, so it likely misses a wheel-installed comgr copy entirely.
| // 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. | ||
| if let Ok(entries) = std::fs::read_dir("/opt") { |
There was a problem hiding this comment.
This /opt/rocm* sibling-install scan isn't mentioned in the PR description (which frames the conflict as system-install vs. wheel-install only) - worth a line explaining it's in scope too.
| .filter(|entry| entry.file_name().to_string_lossy().starts_with(prefix)) | ||
| .map(|entry| entry.path().to_string_lossy().into_owned()) | ||
| .collect(); | ||
| matches.sort(); |
There was a problem hiding this comment.
This sort changes the HIP probe's tie-break order when a directory has more than one matching file (previously OS-arbitrary read_dir order, now sorted). The Risk section says "the HIP probe still takes the first hit and its field is unchanged" - true for the single-match case, not quite for this edge case.
d9b0a75 to
f2bd0c9
Compare
6ea3a21 to
ee48870
Compare
|
🔴 Automated review · pr-review-watcher · ee48870 This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer. SummaryThe change generalises the code object manager scan from find-first to find-all, adding four On the red check: I did find a defect in this diff that would deterministically fail any run that actually executes the new GPU-gated scenario (blocker 1). That scenario resolves to skip rather than fail where no AMD GPU is present, so whether it is the failing check depends on the lane; I cannot confirm which check failed and am not inferring one. Either way the defect is real and independent of the base branch's intermittency. 🚫 Blocking (must fix before merge)
Non-blocking
|
siloteemu
left a comment
siloteemu
left a comment
There was a problem hiding this comment.
🔴 Automated review · pr-review-watcher · ee48870
Change request filed by automation. The findings below are the blocking half of the round published in the report comment on this pull request; the non-blocking notes stay there. This will be withdrawn once they are addressed — no human needs to clear it.
🚫 Blocking (must fix before merge)
-
tests/e2e-cucumber/tests/e2e/examine_steps.rs:859-872— the new step for scenario examine-17 asserts that every copy whosesourceismanaged-runtimecarries a non-emptyinstall_root.ComgrCopy(crates/rocm-core/src/examine.rs:159-177) has exactly four fields —path,real_path,version,source— and nothing anywhere in the workspace emitsinstall_rooton a comgr copy.copy.get("install_root")therefore always yieldsNone,unwrap_or_default()gives"", and the assertion fails on every execution. This is a test that cannot pass for the reason its message states, and it is the headline coverage the PR text offers for the managed case ("examine-17 asserts that on a GPU lane"), so the case is in fact uncovered and the scenario is a guaranteed red wherever a GPU is present. Fix: delete thefor copy in managed { … }loop and its preceding comment (lines 859-872). What remains — copies non-empty, and at least one withsource == "managed-runtime"— is still a non-vacuous assertion of the thing the scenario names. If the owning-install data is wanted now rather than in the follow-up, the alternative is to add the field toComgrCopyand populate it inrecord_comgr_copy, but that is the deferred work and should not be bolted on here. -
crates/rocm-core/src/lib.rs:4172-4177— the doc comment oncollect_sdk_package_library_pathsstates "Attribution is by longest known root and no_rocm_sdk_*directory is a known root, so each resolves to its runtime — which is what keeps a healthy managed install from looking like several installations in conflict." No attribution-by-longest-root mechanism exists in this diff or in the base; nothing computes an owning installation for a comgr copy, andcomgr_matches_runtimeis documented as permanently unset. The comment presents a safety property as provided when the code provides nothing of the kind, which is precisely the guard-in-prose-only pattern — and it is the same descoped feature that produced blocker 1. The commit body carries the same claim twice ("recording every copy with the installation that owns it, plus the same for the HIP runtime so the two can be compared" and "Ownership is matched against known installation roots, longest first, and never derived by walking up from the file"); a maintainer reading either will believe attribution shipped. Fix: cut the second and third sentences of the doc comment, keeping "They belong to the runtime that contains them, not to themselves.", and drop the two ownership paragraphs from the commit body when the branch is next amended.
ee48870 to
a8749a3
Compare
Superseded: this objection was filed against an earlier commit and is replaced by a fresh round at the current head. The earlier blocking item is discharged in the tree; a new, unrelated blocking finding is filed separately.
|
🔴 Automated review · pr-review-watcher · a8749a3 This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer. SummaryThe change replaces the first-match-only 🚫 Blocking (must fix before merge)
Non-blocking
|
siloteemu
left a comment
siloteemu
left a comment
There was a problem hiding this comment.
🔴 Automated review · pr-review-watcher · a8749a3
Change request filed by the automated review round at this commit. The full round, including non-blocking notes, is in the comment posted alongside it.
🚫 Blocking (must fix before merge)
tests/e2e-cucumber/features/examine.feature:184-194— examine-16 is untagged, so it runs on every lane, and its comment claims "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 code does not provide that; the struct defaults do. All four new fields are#[serde(default)]andExamination::default()already yieldscomgr_paths: []andcomgr_selected: null, which is exactly what both steps assert when no copy is found. Measured, not read: with bothprobe_comgr(&mut e)calls deleted, the assertion sequence ofassert_comgr_copies_reportedandassert_comgr_selection_is_stated(tests/e2e-cucumber/tests/e2e/examine_steps.rs:773,:800) still passes against the serialized examination. That is the state of every Windows lane, where the probe is never called at all, and of the mock lane the comment names. So on precisely the lanes where examine-16 is the only coverage of this change, it passes with the change reverted, while its prose says it proves the opposite — and examine-17 cannot compensate because it is@requires-gpu. Fix: tag the scenario@requires-os:linux(the probe is Linux and WSL2 only, so the Windows pass is vacuous by construction), and in the empty branch ofassert_comgr_selection_is_statedassert thatnotescontains the "nolibamd_comgrfound ..." entry thatcrates/rocm-core/src/examine.rs:1927-1930pushes, in addition tocomgr_selectedbeing null. That note is produced only byprobe_comgrrunning and finding nothing, so it distinguishes "probed, found none" from "never probed" — which is the distinction the scenario's own sentence claims to make, and the same distinction the PR already builds intocomgr_matches_runtime. Assert on a substring of the note rather than the whole string so the wording stays free to change; do not assert it in the non-empty branch, where a single copy pushes no note at all.
…s own Builds on the copy search: with every copy of libamd_comgr known, the catalog can say when the one that loads belongs to a different installation than the HIP runtime that loads, and device code compilation therefore fails with an error naming neither. The rule is symmetric, and that is what makes it safe. The design note for this entry proposed a special case -- "when the active runtime is the managed runtime, the matching copy is the wheel copy" -- without which it "fires on every healthy CLI installation". That is a patch over a rule stated asymmetrically. Asking one question of both libraries instead, does the libamd_comgr that would load come from the same installation as the libamdhip64 that would load, makes every case fall out of the rule: a healthy managed install takes both from the managed runtime, so nothing differs and nothing is reported. A control that has to be written as an exception is a rule that has not been stated correctly yet. That turns on attributing a copy to its installation correctly. A managed runtime spreads its libraries across separate _rocm_sdk_* packages, so ownership is decided by matching against known installation roots, longest first -- never by walking up from the file, which would call each package its own install and fire on the most common install we ship. That is the property the mutation check targets. Two further guards on firing: an unattributed copy is a gap in what the search knows rather than a finding about the machine, and the runtime must actually ship a copy of its own, or there is nothing to point the user at. The entry is print-only and ranks neither remedy. Removing a stack and reordering the search path can each break a working Python environment, and which is right depends on which stack the user means to keep. The finding says out loud that it describes the environment outside the managed runtimes. `rocm serve` puts a managed runtime's libraries first on purpose and gets a different, correct answer, and a report that did not name the environment it examined would read as a claim about one it never looked at. The rule above is now stated exactly once, as `comgr_matches_runtime` in examine.rs, and both this entry and `rocm examine --json`'s field of the same name call it rather than each restating the conditions -- so the two surfaces cannot disagree about one machine, and a regression test pins that. Fix option (b)'s advice matched an earlier, asymmetric statement of the rule; it now says to prefer the wheel's own copy of both libraries, which is what the symmetric rule actually asks the user to do. The machine-readable inspection now carries the same `install_root` attribution for the HIP runtime side that it already carried for the code object manager side, since the conflict question is about both libraries and a reader could not previously check either the attribution or the selection for one of them. Known roots are deduplicated across the whole list, not only where repeats happen to land next to each other. `dedup_by` drops consecutive duplicates, and these roots arrive from three independent sources -- the active install, a sorted /opt scan, and the managed runtimes -- so the active install colliding with a sibling was caught or missed depending on where the path sorted. A root recorded twice can make a machine holding one stack read as a machine holding two, which is the false conflict this entry exists not to report. hip_paths and hip_selected are `serde(default)` for the same reason the comgr fields are: an examination read back over the remote path may have been produced by an older CLI that never wrote them. Fix CI: the managed-runtime comgr/HIP search resolved the data directory through `crate::runtime::default_data_dir` (`$HOME/.rocm` or the OS default) instead of `AppPaths::discover`, so it disagreed with the rest of the CLI, and found 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 is why the search saw no managed runtime whatsoever and the GPU e2e lane (`examine-finds-the-managed-runtimes-own-compilation-library`) failed identically on every GPU family. Separately, once a runtime is found, its root resolves to its `_rocm_sdk_devel` package directory, and the search then guessed a venv layout (`root/lib/<python>/site-packages`) to find its sibling packages; that guess does not hold for a real install either, so the search now reads back the library directories the SDK probe already recorded (`library_paths`) instead of re-deriving them. Also fixes a needless-`collect` clippy lint in the e2e step, and tags diagnose-21 `@requires-os:linux`: fix-18-comgr-conflict is registered for linux/wsl only, so applying it for real on a native Windows lane hit the fix's own platform gate before ever reaching the advisory behavior under test. That data-dir fix cleared the scenario everywhere except the Strix Halo Windows lane, where it still failed CI after the rest of this commit landed: `comgr_paths`/`hip_paths` came back empty even with the managed runtime found and the right data dir in hand, because the whole search -- `LD_LIBRARY_PATH`, the loader cache, and the `lib/` directory scan -- is built around ELF shared-object names (`libamd_comgr`, `libamdhip64`). Native Windows ships the equivalent libraries under different names and has neither `LD_LIBRARY_PATH` nor a loader cache, so none of it applies there; WSL is unaffected, since it reports `os_family` "linux" and the runtime underneath really does carry a `.so`. This is the same boundary `fix-18-comgr-conflict` is already gated on (`&["linux", "wsl"]`, addressed above for diagnose-21), just missed for this scenario when it was introduced in #384 -- so examine-17 now carries the same `@requires-os:linux` tag, pre-existing gap, not a regression from this commit. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
… first
HIP compiles device code at run time through libamd_comgr, and a machine
can hold more than one copy. The loader picks one. When the copy it picks
does not belong to the active HIP runtime, compilation fails with an
error naming neither the library nor the second copy.
The search stopped at the first match and looked for one library only, so
the second copy could not be seen at all. It is now one walker serving
both searches, recording every copy found, plus the same for the HIP
runtime so the two can be compared. Attributing each copy to the
installation that owns it is deferred; comgr_matches_runtime stays
permanently unset until that lands.
The managed search now reads the runtime registry rather than listing
<data>/runtimes on disk. A directory listing yields a root and nothing
else, and the root alone does not locate a wheel runtime's libraries --
those live in a sibling _rocm_sdk_* package inside the interpreter's
site-packages, which is why the loader path the CLI sets for its own
processes walks that directory. Searching the root alone therefore missed
the copy this CLI installs itself, which is the case the entry exists for.
That knowledge now lives in one place. collect_managed_runtime_library_paths
is shared by the comgr search and the loader path, so a managed runtime's
layout is described once rather than twice. A second description is a
second thing to keep correct, and the two had already diverged.
Listing the directory was also a second answer to "which runtimes exist",
where the registry is the first one, and only the registry records which
site-packages an interpreter uses.
The record is preferred but not required. 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 command is called on: reporting no copies
there would read as a machine with nothing wrong rather than one we
failed to inspect. With no record the predictable venv layout is read
directly, and a miss costs a directory that is simply not reported.
No dlopen. Reading the version through amd_comgr_get_version would run an
unknown library's initialisers on a machine called on precisely because
something is already wrong, and load a possibly conflicting HIP stack
permanently into the process. The version is read from the versioned
soname instead, and a name carrying none yields an empty version rather
than a confident wrong one.
Directory matches are now sorted, which makes the HIP probe's tie-break
deterministic where one directory holds several matching files. It used
to take whatever read_dir yielded first. Better, but a change, not a
no-op.
The loader-cache tier used to gate on which("ldconfig"), which only walks
$PATH -- and ldconfig lives in /sbin, off a non-root user's PATH on Debian
and derivatives. That silently dropped the one tier that finds a copy
registered only in ld.so.cache; it now reuses the fallback search
ldconfig_cache() already does for the same reason.
Covered at two levels, because neither is sufficient alone. Unit tests
build a wheel-format layout directly, which proves the search understands
a layout we described; only a real managed runtime proves it matches the
one the installer produces, so examine-18 asserts that on a GPU lane.
The four new fields are `serde(default)`. 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 one here refuses every remote running an older
build, reported as "the remote CLI is probably a different version" --
true, and useless, since the older CLI is the one that cannot be changed.
Fix CI: managed_runtime_roots resolved the data directory through
crate::runtime::default_data_dir (`$HOME/.rocm` or the OS default)
instead of AppPaths::discover, so it disagreed with the rest of the CLI
and found nothing at all on any host where ROCM_CLI_DATA_DIR relocates
the data directory -- which is exactly what every GPU e2e lane sets for
isolation. That is why the new GPU scenario
(examine-finds-the-managed-runtimes-own-compilation-library) failed
identically on every GPU family; it now resolves through AppPaths::discover
like everything else in the CLI. Also fixes a needless-collect clippy
lint in the e2e step, and tags that scenario @requires-os:linux: the
search looks only for libamd_comgr by its ELF soname, which native
Windows does not ship.
Rebasing onto main picked up an unrelated examine-16 (EAI-8950, #444), so
this commit's two new scenarios are renumbered to examine-17 and
examine-18 to keep every scenario number in the file unique.
Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
Review: examine-17 (the host-independent code-object-manager scenario) was
untagged and its empty-result assertion matched `Examination::default()`
alone, so it would pass with `probe_comgr` deleted outright -- on native
Windows, where the probe never runs, that is not a hypothetical. Tagged
@requires-os:linux and the assertion now also requires the "no libamd_comgr
found" note that only a probe which actually ran and came up empty pushes.
Verified the new assertion fails by temporarily suppressing that note and
watching the scenario go red, then restored it.
a8749a3 to
51a20ff
Compare
…s own Builds on the copy search: with every copy of libamd_comgr known, the catalog can say when the one that loads belongs to a different installation than the HIP runtime that loads, and device code compilation therefore fails with an error naming neither. The rule is symmetric, and that is what makes it safe. The design note for this entry proposed a special case -- "when the active runtime is the managed runtime, the matching copy is the wheel copy" -- without which it "fires on every healthy CLI installation". That is a patch over a rule stated asymmetrically. Asking one question of both libraries instead, does the libamd_comgr that would load come from the same installation as the libamdhip64 that would load, makes every case fall out of the rule: a healthy managed install takes both from the managed runtime, so nothing differs and nothing is reported. A control that has to be written as an exception is a rule that has not been stated correctly yet. That turns on attributing a copy to its installation correctly. A managed runtime spreads its libraries across separate _rocm_sdk_* packages, so ownership is decided by matching against known installation roots, longest first -- never by walking up from the file, which would call each package its own install and fire on the most common install we ship. That is the property the mutation check targets. Two further guards on firing: an unattributed copy is a gap in what the search knows rather than a finding about the machine, and the runtime must actually ship a copy of its own, or there is nothing to point the user at. The entry is print-only and ranks neither remedy. Removing a stack and reordering the search path can each break a working Python environment, and which is right depends on which stack the user means to keep. The finding says out loud that it describes the environment outside the managed runtimes. `rocm serve` puts a managed runtime's libraries first on purpose and gets a different, correct answer, and a report that did not name the environment it examined would read as a claim about one it never looked at. The rule above is now stated exactly once, as `comgr_matches_runtime` in examine.rs, and both this entry and `rocm examine --json`'s field of the same name call it rather than each restating the conditions -- so the two surfaces cannot disagree about one machine, and a regression test pins that. Fix option (b)'s advice matched an earlier, asymmetric statement of the rule; it now says to prefer the wheel's own copy of both libraries, which is what the symmetric rule actually asks the user to do. The machine-readable inspection now carries the same `install_root` attribution for the HIP runtime side that it already carried for the code object manager side, since the conflict question is about both libraries and a reader could not previously check either the attribution or the selection for one of them. Known roots are deduplicated across the whole list, not only where repeats happen to land next to each other. `dedup_by` drops consecutive duplicates, and these roots arrive from three independent sources -- the active install, a sorted /opt scan, and the managed runtimes -- so the active install colliding with a sibling was caught or missed depending on where the path sorted. A root recorded twice can make a machine holding one stack read as a machine holding two, which is the false conflict this entry exists not to report. hip_paths and hip_selected are `serde(default)` for the same reason the comgr fields are: an examination read back over the remote path may have been produced by an older CLI that never wrote them. Once a managed runtime is found, its root resolves to its `_rocm_sdk_devel` package directory, and the search used to guess a venv layout (`root/lib/<python>/site-packages`) to find its sibling packages; that guess does not hold for a real install, so the search now reads back the library directories the SDK probe already recorded (`library_paths`) instead of re-deriving them. This generalises the walker introduced in #384 (`find_library_copies`/`install_library_dirs`) so it serves the HIP side too, which is why `managed_runtime_roots` now returns `library_paths` plural rather than a single `site_packages` guess. The AppPaths::discover data-dir fix and the @requires-os:linux platform tags this commit originally carried for examine-17/18 belong with the scenario they correct, which #384 introduced -- moved there so the parent doesn't depend on the child to pass its own CI, and this commit now only rebases on top of that fix rather than restating it. Sweeping the same gap once it was visible: examine-17 (the host-independent code-object-manager scenario, renumbered by #384's rebase) asserts `hip_paths`/`hip_selected` the same way it asserts `comgr_paths`/`comgr_selected`, and the empty branch had the identical problem #384's review flagged for the comgr side -- `hip_paths: []` / `hip_selected: null` also hold by nothing more than `Examination`'s own defaults, so the assertion could not tell "probed, found none" from "never probed". `probe_comgr` now pushes a "no libamdhip64 found" note the same way it already did for libamd_comgr, and the HIP assertion now requires it in the empty branch. Verified by temporarily suppressing the note and watching the scenario go red, then restoring it. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
Addressed: examine-17 (renumbered) is now tagged @requires-os:linux and its empty-result assertion requires the "no libamd_comgr found" note, so it can no longer pass with probe_comgr reverted or never called. Verified by mutation-testing the note (suppressed it, watched the scenario fail, restored it).
3 participants
What
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. When the copy the loader picks does not belong to the active runtime, compilation fails with a general error naming neither the library nor the second copy.This CLI installs the second copy itself. Its install path puts ROCm wheels into a managed environment, so a user who follows it on a host that already has system ROCm ends up holding both, having done nothing unusual and been warned about nothing.
Nothing looked past the first match, so the second copy was invisible. The inspection now records every copy in loader search order, which one would load, and its version.
Reporting only. The conflict rule and the catalog entry that states the user's options follow in a second PR. That is where the risk of a false report on a healthy managed install sits — every managed environment this CLI creates legitimately holds two copies — so it belongs with the rule it threatens rather than bundled with an uncontroversial probe.
Non-obvious decisions
No
dlopen. The ticket specified reading the version viaamd_comgr_get_versionthroughdlopen. That runs the library's ELF initialisers — code from an unknown library, on a machine the tool was called to precisely because something is already wrong — and pulls a possibly conflicting HIP stack permanently into the process. The version is read from the versioned soname instead. A renamed file yields an empty version, which is honest; the conflict rule never needs the version, only the report text does.Deduplicated on the resolved path. ROCm ships
libamd_comgr.so.2.8.0beside an unversioned symlink. Counting those as two copies would invent a conflict on an ordinary install — the false report that matters most, since it would fire on healthy machines.It emulates the loader; it is not the loader. No account is taken of
RUNPATH/RPATH,ld.so.preload, or a container remapping paths. The doc comment says so, and the list of copies is the evidence for the verdict rather than a guarantee.It runs on WSL2 too — a deliberate departure from the ticket's "Linux" scope. The WSL2 early return exists for kernel driver questions, and the code says so while still running the framework probe. A shadowed comgr copy is a run-time compilation failure inside the framework, and ROCm on WSL2 is a supported configuration where the two copies collide identically. Skipping it would leave a WSL user unable to see a conflict that is really there.
One existing helper was made
pub(crate)rather than restating where a managed runtime keeps its libraries. A second description of that layout is a second thing to keep correct.Corrections to the ticket
examinealready reports the active runtime's owning installation. It does not —Examinationrecords the system install only. That knowledge lives in a separaterocm examinesurface. Part 2 needs it, androcm-core's own runtime helpers can resolve managed runtime roots, so no cross-crate dependency is required.Examinationhas no text renderer — it surfaces through--jsonand feedsdiagnose. Putting them in the separate text report would mean a second, independent probe. Deferred to part 2, where the diagnosis puts the conflict in front of the user in prose.Verification
cargo test --workspace --all-targets --exclude e2e-cucumber— clean;rocm-core344 passedcargo clippy --locked --workspace --all-targets,cargo fmt --all --check— cleancargo xtask e2e -- -n examine— 15 scenarios, 14 passed. The one failure isexamine-detects-gpu-and-driver, whoseGivenrequires an AMD GPU; this dev host reportsdetected_gfx_target: <unknown>and "wsl gpu plumbing missing". That value comes from a report this diff touches only to change a visibility modifier.The new unit test was verified to fail, not just to pass. Reverting the walker to stop at the first hit — the old behaviour — fails
the_library_path_is_searched_left_to_right_and_every_copy_is_keptand nothing else, confirming it pins the defect rather than the implementation.Also checked against the real binary with two planted copies and a symlink: both distinct copies reported in path order, the symlink pair collapsed to one, version read from the resolved file, and the note naming the winner.
Coverage boundary
The e2e suite cannot install a second ROCm stack, so it cannot prove the two-copy case. It proves the surface every lane has: the inspection answers the question, the reported copies carry enough to act on, and the selected copy agrees with the list. The two-copy, symlink-dedupe and version-parsing rules are proven by unit tests that build the directory layout directly. This is a real boundary, not an oversight.
Risk
Low. Additive: four new fields and a probe that only reads. Nothing existing changes behaviour except the
LD_LIBRARY_PATHwalker, which was generalised so the HIP probe and this one share one implementation — the HIP probe still takes the first hit and its field is unchanged.The
Examinationfield set is a frozen wire contract with a test guarding it; that test fired on this change and was updated deliberately, which is the guard working as intended.