Skip to content

feat(release): distribute rocm-cli as a Python wheel - #184

Open
michaelroy-amd wants to merge 1 commit into
mainfrom
feat/pypi-wheel-distribution
Open

feat(release): distribute rocm-cli as a Python wheel#184
michaelroy-amd wants to merge 1 commit into
mainfrom
feat/pypi-wheel-distribution

Conversation

@michaelroy-amd

Copy link
Copy Markdown
Member

Status: not for immediate merge

Opening this for review now. We will merge once we feel the quality bar is there, pending any other changes that come out of review. Note that the publish path is dormant by design (see the gate below), so merging this does not publish anything.

What this adds

A second distribution channel: pip install rocm-cli installs the prebuilt rocm and rocmd binaries, alongside the existing install.sh / install.ps1 bundles.

  • scripts/build_wheel.py (new) packages binaries the release job has already built and signed. Nothing is rebuilt, so the wheel payload stays byte-identical to the copy inside the signed archive for the same tag.
  • .github/workflows/release.yml builds and smoke-tests a wheel in each existing build job and adds a publish-pypi job using PyPI Trusted Publishing.
  • apps/rocm/src/main.rs stops rocm uninstall from deleting binaries a Python package manager owns.
  • Docs for the new channel and its (different) trust root.

Design decisions worth a reviewer's attention

Binaries ship in the wheel's .data/scripts/ directory, not as console-script shims. This is load-bearing, not a style choice. std::env::current_exe() has to resolve to the real binary: the CLI re-execs itself to launch managed engine services (managed_service_launcher_path) and locates rocmd as a sibling file (sibling_binary_path). A Python shim would make current_exe() the interpreter and break both. Both smoke tests assert the installed files are native executables by magic bytes (7f454c46 / MZ), so a future refactor that introduced a shim fails CI rather than shipping.

Wheels are written to dist/wheels/, a subdirectory. release_readiness.py::validate_exact_dist_assets iterates dist for files only, so the strict release-asset gate is untouched and release_readiness.py needed no changes. The Windows upload glob was also changed from Get-ChildItem dist -Include … -Recurse to Get-ChildItem dist\* -File -Include …; with -Recurse it matched dist\wheels\*.whl.sha256 and would have pushed wheel sidecars into the GitHub release asset set.

Wheels only, no source distribution, and only manylinux_2_17_x86_64 and win_amd64. On any other platform pip fails with "no matching distribution" instead of attempting a Rust build, which matches the supported-platform rule in AGENTS.md §6. The existing manylinux2014 container build already satisfies manylinux_2_17: the released Linux binaries link only allowlisted shared objects and need at most GLIBC_2.16.

Tag to PEP 440 mapping is exhaustive-or-error and cross-checked against [workspace.package] version, so a tag can never publish a wheel whose binaries report a different version. v0.1.0-experimental.1 maps to 0.1.0a1. Because PyPI never allows a version to be re-uploaded, -alpha.N and -experimental.N collapse onto the same aN form and only one spelling may be used per serial; this is documented in docs/release-trust.md.

Publication is dormant. publish-pypi runs only when the repository variable ROCM_CLI_PUBLISH_PYPI is 1 or true (an allowlist, so FALSE/no/off stay dormant), mirroring the existing ROCM_CLI_REQUIRE_PRODUCTION_TRUST pattern. Until then CI builds, smoke-tests, and retains the wheels as workflow artifacts and uploads nothing. The GitHub release asset set is unchanged either way; the pipeline does gain steps that can fail, which the docs say explicitly rather than claiming nothing changed.

The uninstall fix is a real bug, reproduced first. rocm uninstall globbed the running executable's directory and deleted every rocm-named file. Inside a pip environment:

3 item(s) would be removed:
  - config: …/.rocm
  - binary: …/venv/bin/rocm
  - binary: …/venv/bin/rocmd

That deletes files pip owns and leaves the wheel RECORD dangling while pip list still reports the package installed. A new tier in build_uninstall_plan detects a Python-managed layout (pyvenv.cfg, or a *.dist-info/RECORD that claims the script) and skips binary removal, naming the pip / pipx / uv tool command instead. Probe failures resolve conservatively: a tree that exists but cannot be read is treated as managed rather than deleted, because a false positive only leaves files in place while a false negative corrupts another package manager's state.

Trust model, stated plainly

PyPI cannot carry the detached .sig sidecar the installers verify, so this channel has a different trust root: Trusted Publishing (OIDC, no long-lived upload token) plus PEP 740 attestations. docs/release-trust.md is explicit that pip does not verify attestations at install time and offers no flag to require them, so for a plain pip install they are an auditable provenance record rather than an install gate. It also states that the cross-job checksum catches transit corruption but cannot detect a compromised builder that wrote a consistent wheel and sidecar. The GitHub release assets remain the signature-bearing artifacts.

Verification performed

Local, all passing:

  • cargo clippy --workspace --all-targets -- -D warnings, cargo test --workspace --all-targets
  • cargo test -p rocm --bin rocm is_python_managed (6 tests), plus mutation probes: stubbing the detector to false fails 5 of 6, and restoring the old RECORD parser fails the quoted-row test
  • python scripts/build_wheel.py --self-test, release_readiness.py --self-test, build_single_exe_release.py self-test, scripts/smoke_local.py, cargo xtask verify-pinned-keys, both WSL script self-tests, ruff
  • End to end: built a wheel from the published v0.1.0-experimental.1 Linux binaries, installed it into a fresh venv, and confirmed rocm --version, rocmd --version, rocm examine, and mode 0755. Also confirmed pipx install and uv tool install place both executables
  • release.yml parses; every non-pwsh run block passes bash -n; all six pwsh blocks parse under a real Windows PowerShell parser; the new PE magic check was executed there and correctly rejected a #!python shim; all ten uses: refs are 40-hex SHAs with a # vX.Y.Z comment
  • Leak scan over the diff is clean; commit is signed and signed-off

Not runnable here, so stated rather than claimed:

  • GitHub Actions runtime: the manylinux container build, the Windows job, artifact transfer, and the publish gate itself
  • A native Windows runner (PowerShell parsing and behavior checks are not a substitute)
  • Real PyPI or TestPyPI publication
  • GPU hardware lanes (not reachable from this change)

Owner prerequisites before the gate can be opened

  1. Access to the existing (currently empty) AMD organization on PyPI. rocm-cli is unregistered on both PyPI and TestPyPI; rocm itself is squatted by an unrelated placeholder, which is why the distribution name is rocm-cli.
  2. A pending Trusted Publisher for ROCm/rocm-cli, workflow release.yml, environment pypi. The pending-publisher flow creates the project on first OIDC publish, so the name is claimed without ever minting an API token.
  3. A TestPyPI rehearsal via workflow_dispatch (the dispatch input defaults to testpypi precisely so the irreversible action is always opt-in).
  4. Only then set ROCM_CLI_PUBLISH_PYPI.

Review notes

This branch already went through six independent adversarial review passes plus a cross-review round; 19 findings were applied and 1 was rejected on spec grounds (a claim that License-File must be licenses/LICENSE.TXT; the metadata spec defines that path as relative to the project root, and the wheel then stores it at .dist-info/licenses/<path>, so the bare filename is correct). Fresh eyes are still very welcome, particularly on the workflow semantics that can only be proven by running them.


  • If this PR fixes a bug, searched tests/e2e-cucumber/expectations.toml for the fixed ticket ID and removed/narrowed any now-stale xfail rows. — no xfail rows relate to this change; the one existing e2e assertion on uninstall output (skipping running executable on Windows, lifecycle_steps.rs) still holds, because an installer-managed Windows binary is not detected as Python-managed and still reaches that branch.

Add a second distribution channel so `pip install rocm-cli` installs the
prebuilt `rocm` and `rocmd` binaries, alongside the existing install.sh and
install.ps1 bundles.

scripts/build_wheel.py packages binaries that release CI has already built and
signed, so the wheel payload stays byte-identical to the copy inside the signed
archive for the same tag. The binaries ship in the wheel's `.data/scripts`
directory as real executables rather than console-script shims, because
`std::env::current_exe()` has to resolve to the real binary: the CLI re-execs
itself to launch managed engine services and locates `rocmd` as a sibling file.
Wheels land in `dist/wheels/`, a subdirectory, so the exact-asset gate in
release_readiness.py keeps guarding the GitHub release asset set unchanged.

Only `manylinux_2_17_x86_64` and `win_amd64` wheels are produced and no source
distribution is published, so pip refuses to install on an unsupported platform
instead of attempting a build. The released Linux binaries already satisfy
manylinux_2_17: they link only allowlisted shared objects and need at most
GLIBC_2.16. Git tags are mapped onto PEP 440 exhaustively-or-error and
cross-checked against the workspace version, and both smoke tests assert that
the installed files are native executables reporting the expected version.

`rocm uninstall` previously deleted every rocm-named file beside the running
executable. Inside a pip-created environment that deletes files pip owns and
leaves the wheel RECORD dangling while pip still reports the package installed.
It now detects a Python-managed layout and skips binary removal, pointing at the
matching package manager instead. Probe failures resolve conservatively: a tree
that exists but cannot be read is treated as managed rather than deleted.

Publication is gated on the repository variable ROCM_CLI_PUBLISH_PYPI and stays
dormant until the PyPI project and its Trusted Publisher exist. Until then CI
builds, smoke-tests, and retains the wheels as artifacts without uploading.

Signed-off-by: Michael Roy <michael.roy@amd.com>
@michaelroy-amd
michaelroy-amd requested a review from a team as a code owner August 6, 2026 00:33

@volen-silo volen-silo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at 3f27beb. The wheel packaging itself holds up well — spec conformance, the .data/scripts/ rationale, the dormant publish gate, OIDC scoping, action pinning, and the dist/wheels/ subdirectory trick all check out, and the -Recursedist\* glob change is a genuine catch. Two blocking items below, both in the supporting fix and its coverage rather than in the packaging.

Blocking

1. is_python_managed_layout misses pip install --user on Windows — the delete path stays live

apps/rocm/src/main.rs:15133 (python_site_packages_dirs), reached from build_uninstall_plan.

The probe only considers <env_root>/Lib/site-packages and <env_root>/lib/python*/site-packages. CPython's nt_user install scheme is not shaped like that: scripts land in %APPDATA%\Python\Python3XX\Scripts and packages in %APPDATA%\Python\Python3XX\site-packagesno Lib\ level, unlike the nt scheme used for venvs and system installs. So env_root\Lib\site-packages does not exist, env_root\lib does not exist, the function returns Ok(vec![]) rather than Err(ProbeFailed), and the plan falls through to the delete branch.

That is the exact corruption this PR exists to prevent, on a supported platform. The running-executable skip spares rocm.exe, but rocmd.exe gets deleted, leaving pip's RECORD dangling while the package still reports as installed.

Smallest fix: add env_root.join("site-packages") to the candidate list. The RECORD row (..\Scripts\rocm.exe) already matches record_row_targets_script, so nothing else needs to change. Worth a unit test too — the existing coverage has a Scripts parent only in combination with a real pyvenv.cfg (is_python_managed_layout_detects_windows_scripts_directory) and the RECORD-without-pyvenv.cfg case only in a Unix-shaped tree, so this corner is precisely the untested one.

Two related gaps, lower severity, that I would at least see acknowledged in the doc comment's limitations paragraph rather than fixed here:

  • uv tool install / pipx install on Windows. Both symlink on Unix and copy on Windows into %USERPROFILE%\.local\bin. On Linux current_exe() resolves /proc/self/exe back into the tool venv, so detection works; on Windows GetModuleFileNameW returns the copy's own path, with no pyvenv.cfg and no site-packages above it. The user-facing message currently advertises uv tool uninstall as the remedy for a case that is not detected there.
  • Debian/Ubuntu dist-packages — the join at line 15157 hardcodes site-packages. Only reachable behind --break-system-packages, so minor.

The doc comment at :15054 documents one accepted limitation, and it is a false positive (the safe direction). These are false negatives, which is the direction the comment itself calls unacceptable — so they should not be silent.

2. scripts/build_wheel.py --self-test is not wired into CI

.github/workflows/ci.yml:214-228 (Linux) and :348-360 (Windows).

Every other release/acceptance script in scripts/ has its --self-test invoked in both CI lanes — release_readiness.py, wsl_preflight.py, the three *_therock_gpu_test.py harnesses, setup-wsl-portable-build-deps.sh. This adds 631 lines of new release-path code with a substantial self-test (tag mapping, workspace-version cross-check, both platform tags, member set, RECORD hashes, byte-identical rebuild, mode bits) and it runs only locally. build_wheel.py currently appears nowhere outside release.yml, which means its first CI execution would be during a real tag push, ahead of "Create release" — a regression there fails an otherwise-working release instead of failing at PR time. Two lines.

Non-blocking

  • .github/workflows/release.yml:415-431 — the "manual dispatch defaults to TestPyPI" property relies on GitHub backfilling github.event.inputs.pypi_repository from the YAML default:. It does, so this is not a live bug; but TARGET="${REQUESTED:-pypi}" conflates "tag push, no inputs" with "dispatch, field omitted", and the fallback for the latter is the irreversible index. Keying off github.event_name explicitly would make the guarantee provable from the workflow text alone. Cheap, and this is the one place worth being paranoid.
  • scripts/build_wheel.py:222 — the License-File: metadata lines are hardcoded independently of LICENSE_FILES (:44), which drives what actually gets packaged. They agree today; they will diverge silently the first time a notices file is added, and self_test_wheel does not cross-check them. Derive them from the constant.
  • No integration coverage for the uninstall guard. The release smoke test already builds a real venv and pip-installs the wheel; adding rocm uninstall --dry-run there and asserting it refuses would give the headline fix genuine end-to-end coverage for one line of YAML. Currently the only coverage is unit tests over synthetic temp dirs — which is where finding 1 slipped through.
  • New user-visible behavior is undocumented. Neither the new Python-package section nor the existing rocm uninstall block in README.md mentions that the CLI now declines to remove package-manager-owned binaries.
  • While you're here: the "More docs" index in README.md still doesn't link docs/release-trust.md, which this PR roughly doubles.
  • scripts/build_wheel.py:195 is the third byte-for-byte copy of the chunked SHA-256 helper (release_readiness.py, build_single_exe_release.py have the others). In-convention, just noting the count.
  • Gate allowlist (release.yml:405) accepts only 1/true case-sensitively where release_readiness.py's truthy() accepts {1,true,yes,on} case-insensitively. Deliberate and it cannot fail open — inconsistency only.

One tradeoff worth stating accurately

The design notes imply no standard tool could package prebuilt binaries without rebuilding. That is correct for maturin — every subcommand invokes cargo build, there is no package-a-prebuilt mode. It is not correct for hatchling: I built and pip-installed a [tool.hatch.build.targets.wheel.shared-scripts] wheel while reviewing this, and it preserved 0o755 and installed a real executable, no shim. That would replace roughly 330 of the 631 lines, leaving the tag mapping and Cargo cross-check as the domain logic no tool provides.

Hand-rolling is still defensible here — the repo has zero non-stdlib Python dependencies and build_single_exe_release.py already hand-rolls its archives with tarfile/zipfile, so this follows the house pattern. No change requested. The PR just shouldn't claim the alternative was impossible.

Also flagging that the always-on wheel build adds a new way for a real release to fail, ahead of "Create release". docs/release-trust.md says this outright rather than claiming nothing changed, which is the right call — just make sure that is a conscious acceptance and not a footnote.

Things worth reinforcing

  • ProbeFailed as a distinct type, with unreadable-but-exists separated from missing, makes the fail-closed property structural instead of conventional. The RECORD parser handles RFC 4180 quoting, doubled quotes, embedded commas, and CRLF correctly. Finding 1 is about which directories get probed, not this machinery.
  • The smoke tests are real: pip install --no-index into an actual venv, execute the installed binaries, check magic bytes, match the reported version against the wheel filename, $LASTEXITCODE checked after every native call. No || true, no glob that passes on zero matches.
  • The .data/scripts load-bearing claim holds end to end — plain venv, --target, and --prefix installs all place rocm and rocmd as siblings with the exec bit intact, and pip uninstall leaves no dangling RECORD.

One claim I could not verify live and am flagging rather than endorsing: docs/release-trust.md:137-141 on pip not verifying PEP 740 attestations at install time. It matches my understanding and the doc states it conservatively, but I did not confirm it against a current pip.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants