[jussielo-amd]

Why

The modularization audit found two things in host GPU detection that should not be there. detect_system_ram_gib still had an arm for macOS (sysctl -n hw.memsize), and macOS is not a supported host platform, so that path should be removed rather than extended. Separately, detect_host_gpu_summary_full and detect_host_gfx_target_with_context in host_gpu.rs were both marked #[allow(dead_code)] and had no caller outside tests; the real summary path is detect_host_gpu_summary_fast. The allow also hides everything those two functions reach, because a callee of an allowed item counts as used, so the dead chain behind them never warned. This is part of the follow-up work from the modularization audit. Completes ROCMAI-847.

What changes

The branch has three original commits (one per concern, plus a third that gates two items the second left unused on Windows, described under Verification), a merge of main, a small doc follow-up from review, and a test-only commit that adds back a fast unit test of the real library search chain (described below).

The first removes the cfg!(target_os = "macos") arm from detect_system_ram_gib in host_gpu/os_facts.rs. The Windows (CIM query) and Linux (/proc/meminfo) paths are untouched, and on a host that is neither the function now returns None, the value it already returned when the probe found nothing. No new test is needed: the macOS arm was the only thing removed and the existing Linux and Windows tests still cover the rest of the function.

The second deletes the dead chain. I removed the two #[allow(dead_code)] roots and rebuilt without tests, and the compiler then reported the rest, which goes with them. In host_gpu.rs that is detect_host_gpu_name_with_context, detect_wsl_windows_display_name, capture_optional_path_command_with_env and detect_managed_therock_sdk_gfx_target (whose only other callers were two tests of the probe itself), plus the imports they needed. In host_gpu/wsl.rs it is detect_wsl_windows_display_gfx_target and detect_wsl_windows_display_name_fast; the live Linux path keeps detect_wsl_windows_display_gfx_target_fast. The chain also reached into managed_runtime.rs, which the audit list did not name: managed_sdk_ld_library_path was only called by the removed probe, and with it collect_managed_runtime_library_paths and collect_sdk_package_library_paths are now unreferenced by production code, so they are deleted too, along with the TheRockSdkProbeCandidate fields site_packages and bin_path and the manifest's site_packages field, which only that code read (registry records that still carry site_packages keep parsing, since the struct does not deny unknown fields). collect_sdk_library_paths, managed_sdk_tool_path and managed_therock_sdk_probe_candidates stay because other production code uses them. The host_gpu.rs module doc no longer claims the managed-SDK probe stays there because the fallback chain calls it.

Tests: four tests go with the code they exercised, and two small fixtures with them. managed_sdk_probe_detects_gfx_from_therock_tool and managed_sdk_probe_skips_non_therock_manifests tested the removed probe. the_managed_copy_this_cli_installs_is_reachable and a_runtime_with_no_recorded_site_packages_still_contributes_its_root in examine/library_copies.rs tested collect_managed_runtime_library_paths, which that module's real search no longer calls (it calls collect_sdk_library_paths directly), so they pinned a helper nothing used. Two tests are kept and retargeted instead of deleted. The Windows-only inventory test now asserts the same gfx1201 through detect_windows_display_gfx_target_with_inventory, the function the live Windows summary calls, and is renamed windows_examine_gfx_detection_uses_the_display_inventory. The probe-candidate tie-break test now compares root_path, which its fixture already makes distinct per runtime key, instead of the removed bin_path. The original commits together remove about 430 lines (10 added, 431 deleted across six files).

One reviewer-requested follow-up is a comment-only commit: the library_paths doc comment on TheRockSdkProbeCandidate (and the test comment that restates it) still said the layout was guessed from root_path/site_packages, but site_packages is no longer a candidate field, so both now say root_path only.

The last commit is test-only and answers a review note that the wheel-layout case lost its fast coverage. The two deleted library_copies tests drove collect_managed_runtime_library_paths, not the chain the search really uses, so they never covered it; that chain (known_install_roots, install_library_dirs, managed_runtime_roots, collect_sdk_library_paths) had no unit test at all, and I checked with a grep that no #[cfg(test)] code calls any of those four. The new test, a_wheel_runtimes_recorded_library_dirs_outside_its_root_reach_the_search in examine/library_copies.rs, replaces the two deleted tests as the fast net, aimed at the real code. It writes a registry record in the wheel layout (the probe root is the _rocm_sdk_devel package, and the ROCm libraries sit in sibling _rocm_sdk_core and _rocm_sdk_libraries_* packages under site-packages, known only through the record's library_paths), points ROCM_CLI_DATA_DIR at the scratch data directory the way the other env-reading tests in the crate do (RestoredEnvVar under a *_TEST_LOCK, so the env-mutation contract guard in xtask is satisfied), and asserts three things through known_install_roots and install_library_dirs: the recorded directories outside the root are returned and attributed to the root that owns them, a recorded directory that is not on disk is dropped, and the root's own bin is still found. It uses directories only, since find_library_copies already has its own tests for what is found inside them. There is no production change and no visibility change.

I showed the test fails when its target is broken, each time with a targeted edit and a reverse edit: dropping the paths.extend(recorded...) line in install_library_dirs fails the first assertion (only the root's bin comes back); removing the is_dir filter fails the not-on-disk assertion; and making managed_runtime_roots return empty library_paths fails the first assertion. All three were restored and the commit diff is additions only.

Merge of main

main moved while this was in review (#726, #836 and #870 touched host_gpu.rs, gfx_target.rs and lib.rs). I merged origin/main into the branch with a signed merge commit instead of rebasing, so the reviewed commits keep their hashes. There was one conflict, in the import block of host_gpu.rs: main widened the gpu_visibility re-export to include WindowsGpuListing and windows_amd_gpu_listing, next to the two Linux imports this branch keeps. I kept both sides. The resolved file differs from the pre-merge branch version by that one re-export line only, and the merged diff against main is still only the deletions described above.

Because main gained new code, I re-ran the dead-code evidence against the merged tree. A grep of every Rust, markdown and TOML file for each deleted symbol (detect_host_gpu_summary_full, detect_host_gfx_target_with_context, detect_host_gpu_name_with_context, detect_wsl_windows_display_name, capture_optional_path_command_with_env, detect_managed_therock_sdk_gfx_target, detect_wsl_windows_display_gfx_target, managed_sdk_ld_library_path, collect_managed_runtime_library_paths, collect_sdk_package_library_paths) finds nothing, so none of the three merged PRs added a caller of a deleted item. Clippy with -D warnings on the merged tree reports nothing, so none of main's changes left a surviving item without a caller either. No cfg(target_os = "macos") code remains in the files this PR touches (the two remaining macos mentions in crates/rocm-core/src/browser.rs and report_delivery.rs are on main already and out of scope here).

Verification

Run locally on the merged branch (the exact tip is the last commit): cargo fmt --all --check (clean); cargo clippy -p rocm-core -p rocm --all-targets -- -D warnings (clean); cargo test -p rocm-core --no-fail-fast (1004 passed, 0 failed, 3 ignored, all already ignored; the one extra over the previous measurement is the new test); cargo test -p rocm advised_commands (20 passed, 1 ignored); cargo xtask manifest --check and cargo xtask check-crate-edges (both pass); and the pre-commit hooks on each commit (fmt, license headers, markdown links, commit signing, sign-off). A leak scan of the diff against origin/main is clean and a conflict-marker grep is clean.

The leaf-name test list of rocm-core --lib against origin/main differed by exactly the four intended deletions plus the one rename (windows_examine_gfx_detection_uses_inventory_without_rocm_tools to windows_examine_gfx_detection_uses_the_display_inventory): 1010 tests on main, 1006 on the branch before the test-only commit, 1007 with it (the one addition is a_wheel_runtimes_recorded_library_dirs_outside_its_root_reach_the_search).

Coverage floor (reviewer point 1). I ran cargo xtask coverage on the merged tip, which measures the whole workspace the way CI does, and it passes: no floor is breached, and I did not re-bless anything. For the number, I measured rocm-core line coverage with the same aggregation on origin/main and on this branch, from a clean profile each time: 91.35% (31740 of 34747 lines) on main and 91.54% (31531 of 34446 lines) here. The committed rocm-core floor is 82.14%, which was blessed from CI measurements and sits well below my local figure, so the comparison that matters is main against branch on the same machine: removing about 430 lines of untested dead code and four tests did not lower the ratio, it nudged it up by 0.19 points. The CI coverage job on the earlier head also passed.

Tie-break test (reviewer point 2). managed_therock_sdk_probe_candidates_break_install_time_ties_on_the_runtime_key still discriminates, and I confirmed it by breaking the tie-break in sort_records_newest_install_first and watching it go red, then restoring the line. With the runtime-key comparison dropped (so the fallback is the file path, first.json before second.json), the test fails for the file order where the key bbb is in the first file: it picked the bbb root instead of aaa. With the comparison reversed (right.runtime_key.cmp(&left.runtime_key)), it fails for the other order, where aaa is in the first file. So both file orders are covered and neither mutation survives. No change to the test was needed.

The dead-code evidence for the deletions is from a build without tests. Before the change cargo check -p rocm-core --lib reports no warnings, because the allow hides the chain. With the two roots removed it reports detect_wsl_windows_display_gfx_target and detect_wsl_windows_display_name_fast as never used, then (once their imports and callers were removed) managed_sdk_ld_library_path, collect_managed_runtime_library_paths and collect_sdk_package_library_paths as never used, and the three site_packages and bin_path fields as never read. After deleting those, the same build and clippy report nothing.

Every deleted item was private to rocm-core (pub(crate), pub(super) or private); the only public item touched is detect_system_ram_gib, whose signature is unchanged. The Windows-only test returns early on Linux, so the retargeted assertion could only be compiled here, not run; the Windows lane in CI runs it.

My first push passed the Linux checks and then failed every Windows job, because two items were dead only on a Windows build and the lane denies warnings: the detect_linux_primary_gpu_name import in host_gpu.rs (its one remaining caller is the Linux variant of detect_host_gpu_summary_fast) and parse_windows_display_name in gfx_target.rs (its remaining callers are that same Linux-gated function and its own unit test). Neither item is dead on every platform, so the third commit gates them rather than deleting them or adding an allow: the import is #[cfg(target_os = "linux")] and the function is #[cfg(any(target_os = "linux", test))]. The non-Linux stub of detect_linux_primary_gpu_name stays because host_gpu/diagnostics.rs calls it on every platform. I verified that earlier with a cfg(windows) cross-check against the GNU Windows target. For this update I could not repeat the Windows cross-check: this host has no Windows C toolchain for the ring build script (the MSVC target stops at a missing lib.exe, and the GNU cross compiler used earlier is not available now). Instead I re-read the Windows-relevant parts of the merged host_gpu.rs, host_gpu/os_facts.rs and host_gpu/wsl.rs and confirmed that the merge changed none of the gated items or their callers (the only merged line in these files is a pub use re-export, which cannot trigger an unused-import warning). The Windows lane in CI is the check for this.

Not run

The new test is not platform-gated (it builds a registry under a temp directory and compares paths, with no .so names or symlinks), so it also runs on the Windows lane; I could only run it on Linux, and the Windows cross-clippy could not be repeated for it for the toolchain reason above.

The full workspace tests, python scripts/smoke_local.py, the tests/e2e-cucumber suite, the Windows lane and a local Windows cross-clippy were not run; CI carries them.

Overlaps and scope

#836, #726 and #870 have merged into main and are included by the merge above. There is no user-visible change and no scenario is needed. The deleted functions had no production caller, so no command output, exit code or file the CLI produces changes, and the macOS arm only ever ran on a platform the project does not support. A reviewer noted that the wheel-layout managed-runtime case (ROCm libraries in a sibling _rocm_sdk_core package) had no fast unit test after this PR; that was already true before it, because the two deleted tests only exercised the unused helper, not the production chain. The last commit adds one that does (see What changes). The end-to-end coverage is unchanged: the GPU-gated scenario examine-finds-the-managed-runtimes-own-compilation-library (examine-19) in tests/e2e-cucumber/features/examine.feature, which runs on the @requires-gpu lane. The new test is not a user-observable behavior change, so it needs no scenario of its own.

[Copilot]

🟢 Approval recommended

No active review issues remain for the cleanup of unused GPU-detection and managed-runtime code.

No open issues 🎉

What changed in this PR

This PR simplifies rocm-core host GPU detection by removing unsupported macOS RAM probing and an unused GPU-detection fallback chain, while preserving the live Windows and Linux detection paths.

Changes:

• Removes the macOS-specific RAM probe.
• Deletes unused GPU-detection functions, managed-runtime helpers, and associated fields and imports.
• Removes tests for deleted helpers and retargets two retained tests to surviving code and fields.

File	Description
crates/​rocm-core/​src/​managed_runtime.rs	Removes unused SDK library-path helpers and fields; updates the candidate-ordering test.
crates/​rocm-core/​src/​lib.rs	Removes an unused managed-runtime import.
crates/​rocm-core/​src/​host_gpu/​wsl.rs	Deletes unused WSL display-probe wrappers.
crates/​rocm-core/​src/​host_gpu/​os_facts.rs	Removes macOS RAM detection.
crates/​rocm-core/​src/​host_gpu.rs	Deletes the dead fallback chain and related tests; retargets the Windows inventory test and updates module documentation.
crates/​rocm-core/​src/​examine/​library_copies.rs	Removes tests and a fixture used only by deleted helpers.

_{Review effort: Balanced}

[nowycondro]

🤖 Automated review (Claude Code) · 60ee5c5

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.

Review — no blocking findings

Full review of the whole change.
Implements ROCMAI-847, items 2 and 5 of ROCMAI-760 (Phase 8.1-H), described there as "PR 1 of 7"; the PR body's "Completes ROCMAI-847" matches the ticket's own framing of this step as fully delivered by this PR.

Blocking

None

Non-blocking

• The wheel-layout managed-runtime scenario (ROCm libraries in a sibling _rocm_sdk_core package under site-packages) has no fast unit-test coverage after this PR — crates/rocm-core/src/examine/library_copies.rs, crates/rocm-core/src/managed_runtime.rs
  This is pre-existing, not introduced here: the two deleted tests (the_managed_copy_this_cli_installs_is_reachable, a_runtime_with_no_recorded_site_packages_still_contributes_its_root) only ever exercised the now-dead collect_managed_runtime_library_paths, never the real production chain (probe_comgr → install_library_dirs → managed_runtime_roots, which already called collect_sdk_library_paths directly). That real chain has zero #[cfg(test)] callers before or after this PR; the only thing that still exercises the wheel-format scenario end-to-end is the GPU-gated e2e scenario @requires-gpu @requires-os:linux in tests/e2e-cucumber/features/examine.feature. The PR's removal reasoning ("that helper is unreferenced") is correct, but it also removed the closest thing the suite had to a fast-running safety net for that scenario shape, and neither the PR body nor the commits call that out.
  Confidence 75 · test-coverage · Fix: no action required to merge this PR, but worth a follow-up fast unit test that builds a registry JSON with library_paths pointing outside the probed root and asserts managed_runtime_roots()/install_library_dirs() surface it — or at minimum a PR-text note (per AGENTS.md §3's "say so and name the lane" rule) that this scenario's only coverage is the gated e2e lane.

Decisions for the author

None

Positive signals

• Commit 30b613ad's message independently answers nearly every question a reviewer would ask (why each deletion is production-dead, which fields/tests survive and why, backward-compatibility of the site_packages registry field) before being asked.
• The mid-PR self-correction (commit 60ee5c5c) is a genuine example of driving CI to green rather than silencing it: the author diagnosed a Windows-only unused warning caused by the prior commit, reproduced it locally via a x86_64-pc-windows-gnu cross-build (no Windows host available), and applied the minimal #[cfg] fix rather than an #[allow(dead_code)] — avoiding exactly the blind spot (allow hiding a dead chain) that this PR exists to clean up.
• The #[cfg(target_os = "linux")] vs. ungated import split for detect_linux_primary_gpu_name vs. detect_linux_amdgpu_driver_version in host_gpu.rs is correct and was verified against actual call sites (the former's only use is inside a Linux-gated function; the latter is called unconditionally from detect_gpu_driver_version behind a runtime, not compile-time, check) — easy to get backwards, and it wasn't.

Deployment notes

None. The only public item touched (detect_system_ram_gib) keeps its signature; every other deleted/changed symbol is private or pub(crate)/pub(super). Old on-disk registry JSON records that still carry a site_packages key continue to deserialize (TheRockSdkProbeManifest has no #[serde(deny_unknown_fields)]).

What this covered

Every file in the diff (crates/rocm-core/src/{host_gpu.rs, host_gpu/os_facts.rs, host_gpu/wsl.rs, gfx_target.rs, managed_runtime.rs, lib.rs, examine/library_copies.rs}) read completely at both the base and head versions, plus surrounding context (host_gpu/linux.rs, host_gpu/diagnostics.rs, apps/rocm/src/amd_smi.rs's use of managed_sdk_tool_path) via a read-only clone, across two independent fan-out workers grouped by coupling (the host_gpu/gfx-target/wsl/os_facts cluster, and the managed_runtime/lib.rs/library_copies cluster), each adversarially verifying its own candidates before reporting. Reviewed against a7f84e8a…60ee5c5c (3 commits: cf64d5ac, 30b613ad, 60ee5c5c).
Also done directly: agent-instruction adherence against the repo's AGENTS.md (in full — the macOS-unsupported and coverage-floor-rebless rules both directly bear on this change and are satisfied), git log/git blame history on all five touched source files, and a check of the two PRs (#836, #726) the description flags as touching overlapping files — both remain open and unmerged, and neither touches the deleted code paths, so there is no live conflict beyond the ordinary rebase the author already disclosed.
Did not run: a dedicated "prior review comments" pass — no earlier reviews exist on this PR besides an automated Copilot pass with no open issues, which this review does not simply defer to. CI: all 26 check runs on this head are success (two docs/skill jobs legitimately skipped as path-filtered) — including the "Coverage (rocm-dash crates, ratcheted)" gate, which despite its stale name instruments every workspace crate including rocm-core's coverage floor, so the author's own stated uncertainty about a coverage-floor drop from the deleted tests is resolved: the gate ran and passed.
Not reconciled against any discussion on this PR.

🤖 Generated with Claude Code

[juhovainio]

I read the whole diff and checked the deleted symbols against the PR head: nothing still references them, the sdk_ready filter still requires bin_path so dropping it from the mapping changes no behaviour, and the Linux/Windows gating looks consistent. The change matches the scope of ROCMAI-847 and CI is green. I found nothing that breaks behaviour, but a few things should be sorted out before merge:

• Coverage floor. About 430 lines and 4 tests were removed from rocm-core, and the PR body says cargo xtask coverage was not run. Please run it, and if a floor drops, re-bless with the reason stated in the commit.
• Retargeted tie-break test. ..._break_install_time_ties_on_the_runtime_key now compares root_path. The fixture makes root_path distinct per key, so it should still discriminate in both file orders, but please break the tie-break (e.g. drop the key comparison) and confirm the test goes red.
• Stale doc comment. The library_paths doc comment on TheRockSdkProbeCandidate in crates/rocm-core/src/managed_runtime.rs still mentions site_packages, which is no longer a candidate field.

[jussielo-amd]

Pushed c23fe1e (main merged in; the branch conflicted after #726, #836 and #870 landed) and b40bb23. Answering the three points from review:

Coverage floor: cargo xtask coverage passes on the merged tip and nothing was re-blessed. Measured the way CI does, from a clean profile each time, rocm-core line coverage is 91.35% on main (31740 of 34747 lines) and 91.54% on this branch (31531 of 34446), against a committed floor of 82.14%, so removing the dead code did not lower it.

Retargeted tie-break test: I broke sort_records_newest_install_first two ways and the test went red both times: dropping the runtime-key comparison fails the first=bbb, second=aaa file order, and reversing the comparison fails first=aaa, second=bbb. So it still discriminates in both file orders and needed no strengthening.

Stale doc comment: b40bb23 removes site_packages from the library_paths doc on TheRockSdkProbeCandidate and from the test comment that restated it.

Also re-checked after the merge: no caller of any deleted symbol exists in the merged tree (including the code that #726 and #870 added), and no macOS cfg remains in the touched files. fmt, clippy -D warnings for rocm-core and rocm, cargo test -p rocm-core (1003 passed), advised_commands, the manifest and crate-edge checks all pass; the fresh push starts the full CI lanes.

[Copilot]

🟢 Approval recommended

The review identifies no active issues in the host GPU-detection cleanup.

No open issues 🎉

_{Review effort: Balanced}

[jussielo-amd]

Pushed 27aa34e, a test-only commit. The automated review noted that the managed-runtime wheel layout (ROCm libraries in sibling _rocm_sdk_core and _rocm_sdk_libraries_* packages next to the probed root, found only through the registry's library_paths) had no fast test after the dead helper and its two tests were removed, because the real search chain (known_install_roots, install_library_dirs, managed_runtime_roots) had no unit-test caller before this PR either; only the GPU-gated scenario examine-19 covers it end to end. The new test a_wheel_runtimes_recorded_library_dirs_outside_its_root_reach_the_search writes such a registry record and asserts that the recorded directories outside the root come back attributed to their root, that a recorded directory missing on disk is dropped, and that the root's own bin is still found. I broke the code three ways and the test went red each time: dropping the recorded paths, dropping the on-disk filter, and returning empty library_paths from managed_runtime_roots. It adds 93 lines inside the test module and changes no production code. The description is updated for the extra commit and names examine-19 as the end-to-end lane.

fixed

[juhovainio]

Reviewed the full diff at 27aa34e. No blocking findings.

• Removing the macOS RAM arm is clean; detect_system_ram_gib falls through to None, and macOS is out of scope for this repo.
• Dropping bin_path from TheRockSdkProbeCandidate does not change which registry records qualify, because the sdk_ready check in managed_runtime.rs still requires sdk.bin_path.
• Dropping the manifest site_packages field is safe: the struct has no deny_unknown_fields, so older registry records still parse.
• The cfg gating on the remaining callers is consistent, and Linux and Windows clippy/build jobs are green.
• The new test a_wheel_runtimes_recorded_library_dirs_outside_its_root_reach_the_search exercises the real search chain through recorded library_paths, which replaces the coverage of the removed unreferenced helpers.
• No user-observable change, and the PR body justifies having no scenario. No coverage floor was re-blessed.

Non-blocking: the change is wider than the ticket's rough line estimate (managed_runtime.rs, wsl.rs, library_copies.rs), but the extra removals are backed by compiler dead-code evidence and the PR body explains them.

CI: the only failures are the Strix Halo Ubuntu E2E jobs, which fail at "a model is being served on GPU" after asset download errors (HTTP 404 on a llama.cpp asset, HTTP 403 on a TheRock tarball). That looks like runner/network infrastructure rather than this diff. MI300X and W7900 were still pending when I checked, so please confirm they finish green before merging.