Repository navigation
ci: Windows MSI build workflow + dev-bucket upload (Milestone 2) - #8097
Conversation
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
c6226a9 to
867640a
Compare
| # TODO(#1987): follow-up PR to add a --output-dir flag to | ||
| # generate_msi_wxs.py so it writes <dir>/<output_stem>.wxs itself and | ||
| # the workflow no longer needs to know the filename. | ||
| stem="$(python -c "import sys; sys.path.insert(0, 'build_tools/packaging/windows'); from generate_msi_wxs import PACKAGES; print(PACKAGES['${{ matrix.package }}'].output_stem)")" |
There was a problem hiding this comment.
could we just call the script that does this? what if the function was updated, we would need to update here
There was a problem hiding this comment.
sure will do , this was a temp addition to test some things while in draft
There was a problem hiding this comment.
Agreed. The clean fix is the --output-dir flag noted in the TODO here — the generator would write <dir>/<output_stem>.wxs itself so the workflow never imports PACKAGES. Since that's a change to the (separately-owned) generator plus tests, I've left it as a tracked follow-up to keep this PR scoped to the workflow. TODO comment retained.
There was a problem hiding this comment.
No complex inline code in workflow files, needs to be moved into an existing script or a new script needs to be added instead.
Inline python code in particular should be banned, will amend the style guide to make that clear.
There was a problem hiding this comment.
Done — the stem/PACKAGES import is gone; the generator names files itself via --output-dir. Only a small bash wix build loop remains.
Add multi_arch_build_native_windows_packages.yml, a standalone build workflow (workflow_dispatch + workflow_call) that packages already-built Windows artifacts into MSI installers. It fetches artifacts via --artifacts-url, runs generate_msi_wxs.py, and compiles with wix build -arch x64, uploading each MSI as a workflow artifact. Decoupled from CI/release so downstream test and release workflows can consume its output from any run. WiX is pinned to v4 because v7 gates every command behind the OSMF EULA (WIX7015), which fails unattended CI. The packages input is a JSON list defaulting to ["runtime"]; core and other packages can be added once their Windows artifacts publish. Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
Implements Milestone 2 step ii of #1987: get the built MSI into the dev release bucket, mirroring the native-Linux path with pkg_type "msi". Upload hop (MSI -> dev artifacts bucket): - WorkflowOutputRoot.native_windows_packages() returns {run_id}-windows/packages/msi - new upload_msi_packages.py uploads *.msi via backend.upload_directory; the bucket is auto-selected from RELEASE_TYPE - the build workflow gains a release_type input (default dev), id-token: write, and Configure-AWS + upload steps Publish hop (dev artifacts -> dev release bucket): - publish_native_windows_packages() copies {run_id}-windows/packages/msi to v5/rocm/core/packages/msi/{date}-{run_id}, wired into the windows dispatch in publish_rocm_to_release_buckets.py Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
Two CI unit tests failed on the Milestone 2 change: - test_windows_skips_native_packages asserted Windows publishes nothing native (3 copy calls). That premise is now obsolete: Windows publishes the MSI. Renamed to test_windows_copies_native_msi_packages and assert 4 calls plus the msi source/dest locations. - workflow_step_timeouts_test flagged the new workflow's actions/checkout step as missing timeout-minutes. Added timeout-minutes: 15. Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
Per the GitHub Actions style guide's "safe defaults for inputs" rule and matching the sibling build helpers (build_windows_python_packages.yml), default release_type to "ci" rather than "dev" so a bare workflow_dispatch does not publish to the dev bucket. Callers pass "dev"/"nightly" explicitly. Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
The Configure-AWS and upload steps need OIDC credentials whose IAM role trust is scoped to ROCm/TheRock, so they fail on forks. Gate both on github.repository_owner == 'ROCm', matching the pattern used by test_native_linux_packages_install.yml and the pytorch wheel workflows. This lets the workflow run from personal forks (exercising the full generate + wix build path and keeping the GHA artifact) for pre-merge testing, while the S3 upload runs only on the canonical repo. Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
The fork test run revealed the legacy System32 DLLs amdhip64_6.dll and amd_comgr_2.dll were silently dropped from the MSI: they are tracked by DVC under rocm-systems/shared/amdgpu-windows-interop/legacy/, but the checkout uses submodules: false so the .dvc pointer files were absent and the generator skipped them. Add a step that sparse-fetches only that legacy directory (blobless, shallow) at the rocm-systems SHA recorded in the artifact set's own therock_manifest.json (submodules[].pin_sha) — NOT this checkout's submodule gitlink, which can differ from the commit the artifacts were built from and would version-skew the legacy DLLs against the rest of the MSI. The generator then fetches the actual DLLs over anonymous HTTPS from the DVC remote, so all five legacy DLLs are included. TODO(#1987): move this into generate_msi_wxs.py (it already fetches the manifest in --artifacts-url mode) in a follow-up and drop this step. Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
The workflow named the .wxs/.msi after the --package selector (runtime.msi) instead of the package's output_stem (amdrocm-runtime.msi) — the documented distributable filename used throughout msi-generator-usage.md and the install commands. Read output_stem from the generator's PACKAGES table so the name has a single source of truth. TODO(#1987): follow-up PR to add a --output-dir flag to generate_msi_wxs.py so the generator writes <dir>/<output_stem>.wxs itself and the workflow no longer needs to know or compute the filename. Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
Publish the WiX source (.wxs) alongside the compiled MSI in the same workflow artifact, so the exact source that produced each installer is available for inspection and reproducible rebuilds. Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
867640a to
7ab433f
Compare
|
Are you looking for a review from me here now? I generally exclude draft PRs from my code review queue. |
7ab433f to
6ce4eeb
Compare
The MSI generator changed on main: #8200 replaced --artifacts-url with --run-id (bucket resolved from run id + RELEASE_TYPE), and #8194 removed the generator's built-in DVC legacy-DLL fetch, making resolve_legacy_dlls() read the driver DLLs (amdhip64_6, amd_comgr_2) from the rocm-systems source checkout and fail fatally if any are missing. This workflow still targeted the old interface and only sparse-checked-out the .dvc pointers, so it would now crash in the generate step. - Replace the artifacts_url input with run_id (+ run_github_repo for fork runs) in both workflow_dispatch and workflow_call; update run-name and env. - Derive the artifact manifest URL from the run id via WorkflowOutputRoot.from_workflow_run(...).manifest_root().https_url instead of ${ARTIFACTS_URL}/manifests/..., keeping the rocm-systems pin_sha read so the legacy DLLs stay version-matched to the artifacts (ref and run_id are independent inputs, so the checkout gitlink can differ from the build). - Materialize the actual legacy DLLs: add .dvc/ to the sparse checkout (for the anonymous S3 remote config) and pull them via fetch_dvc_artifacts.pull, called directly to avoid the outer checkout's submodule/topology logic. The DLLs are driver-supplied binaries in DVC/S3, so a git checkout alone only yields the .dvc pointers. - Update the generate step's generator call accordingly: --run-id and an optional --run-github-repo (passed only when set) in place of the removed --artifacts-url / --platform flags. Verified: actionlint + workflow unit tests pass; manifest URL resolves to the nightly bucket and the rocm-systems pin parses for a real retained run. Co-Authored-By: Claude Opus 4 (1M context) <noreply@anthropic.com>
6ce4eeb to
4d21ca8
Compare
just did a rebase and refactor after merging the other 2 PRs, will push some commits to address @geomin12 comments then mark as ready to review |
…onfig runner Addresses geomin12's review on #8097: - Rename build_tools/packaging/windows/upload_msi_packages.py -> upload_package_repo.py (and its function) to match the Linux native-package uploader build_tools/packaging/linux/upload_package_repo.py. Update the workflow call site and docstring references in workflow_outputs.py and publish_rocm_to_release_buckets.py. - Drop the actions/upload-artifact step so the Windows flow matches Linux: S3-only, no transient GitHub artifact. The build job now uploads solely to the run's artifacts bucket via upload_package_repo.py (RELEASE_TYPE selects dev/nightly/etc.), which publish_rocm_to_release_buckets.py later copies to the release bucket. Also drops the now-dead MSI_STEM env export. - Resolve the Windows build runner label from ROCm/therock-ci-config instead of hardcoding it. A new lightweight setup job checks out therock-ci-config and computes the label via amdgpu_family_matrix.get_build_runner_labels() (the shared helper other CI uses), exposing it as an output the build job consumes through needs.setup.outputs.runs_on. Falls back to the helper's local default when ci-config is absent, and to windows-2022 on forks. Deferred to a follow-up (per review): the --output-dir generator flag so the workflow stops inline-importing PACKAGES to name the .wxs/.msi (TODO left in the generate step). Verified: actionlint + workflow/generator/publish unit tests pass; runner-label resolution returns azure-windows-scale-rocm locally. Co-Authored-By: Claude Opus 4 (1M context) <noreply@anthropic.com>
…onfig runner Addresses geomin12's review on #8097: - Rename build_tools/packaging/windows/upload_msi_packages.py -> upload_package_repo.py (and its function) to match the Linux native-package uploader build_tools/packaging/linux/upload_package_repo.py. Update the workflow call site and docstring references in workflow_outputs.py and publish_rocm_to_release_buckets.py. - Drop the actions/upload-artifact step so the Windows flow matches Linux: S3-only, no transient GitHub artifact. The build job now uploads solely to the run's artifacts bucket via upload_package_repo.py (RELEASE_TYPE selects dev/nightly/etc.), which publish_rocm_to_release_buckets.py later copies to the release bucket. Also drops the now-dead MSI_STEM env export. - Resolve the Windows build runner label from ROCm/therock-ci-config instead of hardcoding it. A new lightweight setup job checks out therock-ci-config and computes the label via amdgpu_family_matrix.get_build_runner_labels() (the shared helper other CI uses), exposing it as an output the build job consumes through needs.setup.outputs.runs_on. Falls back to the helper's local default when ci-config is absent, and to windows-2022 on forks. Deferred to a follow-up (per review): the --output-dir generator flag so the workflow stops inline-importing PACKAGES to name the .wxs/.msi (TODO left in the generate step). Verified: actionlint + workflow/generator/publish unit tests pass; runner-label resolution returns azure-windows-scale-rocm locally. Co-Authored-By: Claude Opus 4 (1M context) <noreply@anthropic.com>
468614a to
8611dda
Compare
ScottTodd
left a comment
There was a problem hiding this comment.
LGTM at a high level. Main comments are:
- Being able to test the workflow from a fork is great, thanks for following the design there and linking to the test runs
- Let's trim inline code from the workflow file (some sites can use existing scripts, some may need code changes to the new windows packaging scripts)
- Please keep docs up to date with new package uploading code
| dev/nightly example: | ||
| s3://therock-dev-artifacts/12345-windows/packages/msi/ | ||
| -> s3://therock-repo-amd-dev-core/v5/rocm/core/packages/msi/20250101-12345/ | ||
|
|
||
| prerelease example: | ||
| s3://therock-prerelease-artifacts/12345-windows/packages/msi/ | ||
| -> s3://therock-repo-amd-rc-core/v5/rocm/core/packages/msi/ |
There was a problem hiding this comment.
@nunnikri I'm not sure what we should do here. The date subdirectories for dev/nightly were needed for native Linux packages to work better with package managers, right? Is there a similar requirement for native Windows packages? Otherwise I'd prefer to follow what we do for python packages and tarballs and use the same directory path for dev/nightly/prerelease/stable.
There was a problem hiding this comment.
I'm still not sure on this point. Can you at least leave a TODO or some other comment to suggest that we're still deciding which paths we should use in each release channel?
There was a problem hiding this comment.
Lets callers build several packages in one invocation and have the generator name each output file, so the CI workflow no longer runs a matrix job per package or inline-imports the PACKAGES table to compute filenames. - --packages accepts a comma-separated list; mutually exclusive with --package. - --output-dir writes <dir>/<output_stem>.wxs per package; mutually exclusive with --output (which names a single file and is rejected for >1 package). - build_wxs stays single-package and unchanged; a thin build_all loop resolves the artifacts once (the fetch is not package-specific) and iterates packages. - Post-parse validation rejects incompatible flag combinations; --list still works. 57 existing tests unchanged and passing; 15 new tests cover multi-package output, --output-dir naming, and the validation matrix. Co-Authored-By: Claude Opus 4 (1M context) <noreply@anthropic.com>
Address ScottTodd's review on #8097, trimming inline code and matching the conventions of the sibling packaging workflows. - Remove all inline Python from the workflow. The runner-label setup job (with its inline label resolution) is gone; the build runs on a GitHub-hosted windows-2022 runner since it is short and cheap. The DVC step is now a plain submodule init + fetch_dvc_artifacts.py pull, using this checkout's pinned rocm-systems commit rather than resolving the artifacts' manifest inline. - Drop the per-package matrix; the generate step calls generate_msi_wxs.py once with --packages and --output-dir, then loops over the produced .wxs files to run wix build. No more inline PACKAGES import. - Rename inputs to match build_windows_python_packages.yml: artifact_run_id and artifact_github_repo (default ROCm/TheRock); make rocm_version required so no child job re-derives a version. This also fixes a latent bug where the env block referenced the old input names. - Trim the header comment and enrich run-name with packages/version/release_type. - upload_package_repo.py: raise FileNotFoundError instead of sys.exit for a missing package dir, and add unit tests mirroring the Linux uploader's. - Document the {run_id}-windows/packages/msi path in workflow_outputs.md. Co-Authored-By: Claude Opus 4 (1M context) <noreply@anthropic.com>
ScottTodd
left a comment
There was a problem hiding this comment.
LGTM ~ the detail about the s3://therock-repo-amd-dev-core/v5/rocm/core/packages/msi/20250101-12345/ vs s3://therock-repo-amd-dev-core/v5/rocm/core/packages/msi/ folder path for release publishing
| Run:: | ||
|
|
||
| python3.12 -m pytest build_tools/packaging/windows/tests/upload_package_repo_test.py |
There was a problem hiding this comment.
nit: for tests that don't have any special requirements, comments like this can be omitted.
I'm also adjusting how pytest is run in #8466 so a simple pytest from the repository root just works, then anything beyond that will follow standard instructions like https://docs.pytest.org/en/stable/how-to/usage.html
There was a problem hiding this comment.
Done — removed the "Run::" block. Thanks, that will be nicer once plain pytest from the root works via #8466.
| # --package and --packages are mutually exclusive; exactly one is required. | ||
| if args.package and args.packages: | ||
| parser.error("use either --package or --packages, not both") | ||
| if not args.package and not args.packages: | ||
| parser.error("--package or --packages is required (use --list to see options)") |
There was a problem hiding this comment.
(no action needed)
argparse has built in support for mutually exclusive arguments that handles this for us, see https://docs.python.org/3/library/argparse.html#mutual-exclusion
however, since this also has mutually exclusive pairs of arguments (--package --output and --packages --output-dir), what you have is probably fine
There was a problem hiding this comment.
Thanks — agreed. add_mutually_exclusive_group handles a single pair cleanly, but with two pairs (--package/--output and --packages/--output-dir) plus the "output-dir required for >1 package" rule, the explicit checks read more clearly, so I left them as-is.
| dev/nightly example: | ||
| s3://therock-dev-artifacts/12345-windows/packages/msi/ | ||
| -> s3://therock-repo-amd-dev-core/v5/rocm/core/packages/msi/20250101-12345/ | ||
|
|
||
| prerelease example: | ||
| s3://therock-prerelease-artifacts/12345-windows/packages/msi/ | ||
| -> s3://therock-repo-amd-rc-core/v5/rocm/core/packages/msi/ |
There was a problem hiding this comment.
I'm still not sure on this point. Can you at least leave a TODO or some other comment to suggest that we're still deciding which paths we should use in each release channel?
- publish_rocm_to_release_buckets.py: add a TODO noting the Windows MSI release
destination path scheme is not yet settled per channel (mirrors Linux today
with a {date}-{run_id} subdir, but may follow python/tarballs' single path).
Requested by reviewer, who is still deciding.
- upload_package_repo_test.py: drop the redundant "Run::" usage comment; the
tests need no special invocation.
Co-Authored-By: Claude Opus 4 (1M context) <noreply@anthropic.com>
The whole-tree `pytest build_tools/` run (Unit Tests check) aborted collection because build_tools/packaging/windows/tests/upload_package_repo_test.py shared a basename with the existing linux one, and both did a bare `import upload_package_repo` that resolved to whichever dir was first on sys.path. - Rename to windows_upload_package_repo_test.py so pytest can import both test files (no __init__.py packages in these dirs). - Load the module under test by path under a unique name via importlib, matching the pattern in packaging/linux/tests/build_package_test.py, so it never collides with the linux upload_package_repo module in sys.modules. Co-Authored-By: Claude Opus 4 (1M context) <noreply@anthropic.com>
|
@idass1990 @ScottTodd Fyi, this broke releases, see #8617. |
| dev/nightly example: | ||
| s3://therock-dev-artifacts/12345-windows/packages/msi/ | ||
| -> s3://therock-repo-amd-dev-core/v5/rocm/core/packages/msi/20250101-12345/ | ||
|
|
||
| prerelease example: | ||
| s3://therock-prerelease-artifacts/12345-windows/packages/msi/ | ||
| -> s3://therock-repo-amd-rc-core/v5/rocm/core/packages/msi/ |
There was a problem hiding this comment.
Post review:
Locations here does not match what RFC0012 specifies. These should go to core/windows-installers: https://github.com/ROCm/TheRock/blob/main/docs/rfcs/RFC0012-Repo-Structure.md#structure-on-each-streamrepoamdcom-subdomain
…ng (#8676) ## Motivation * Progress on #8272 * Follow-up to #8097 * Follow-up to #8361 See https://docs.zizmor.sh/audits/#artipacked and https://github.com/ROCm/TheRock/blob/main/docs/development/style_guides/github_actions_style_guide.md#security---disable-checkout-credential-persistence > By default, using [actions/checkout](https://github.com/actions/checkout) causes a credential to be persisted on disk. Versions below `v6.0.0` store the credential directly in the checked-out repo's `.git/config`, while `v6.0.0` and later store it under `$RUNNER_TEMP`. > > Subsequent steps may accidentally publicly persist the credential, e.g. by including it in a publicly accessible artifact via [actions/upload-artifact](https://github.com/actions/upload-artifact). ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/TheRock/blob/main/CONTRIBUTING.md.
Re-adds publish_native_windows_packages() to the release publish script, previously reverted in #8629 to fix the #8617 regression. The original #8097 implementation published to core/packages/msi/, which does not match RFC0012: per the repo-structure RFC, Windows installer artifacts (.exe/.msi) belong under core/windows-installers/, not core/packages/. Addresses marbre's post-merge review comment on #8097 (discussion_r4145328177). The current MSI is a single OS-agnostic installer, so it is published to a flat windows-installers/ prefix (dated subdir for dev/nightly, stable path for prerelease) rather than the per-OS subfolders (win11/, server2022/, server2025/) RFC0012 reserves for OS-specific installers — our MSI payload is identical across Windows versions. NOTE: do not merge until the MSI build workflow (multi_arch_build_native_windows_packages.yml) is wired into the release pipeline (Milestone 4, #1987). Publishing hard-fails on zero MSI files; merging this alone, while the build has no callers, reintroduces #8617. Co-Authored-By: Claude Opus 4 (1M context) <noreply@anthropic.com>
Summary
Adds a standalone Windows MSI build workflow and wires MSI publishing into the release path, tracked in #1987 and building on the merged
generate_msi_wxs.pygenerator (#4803) and its--run-idinterface (#8194, #8200).multi_arch_build_native_windows_packages.yml, a standaloneworkflow_dispatch+workflow_callworkflow that runsgenerate_msi_wxs.pyandwix build -arch x64to produce MSI installers for the packages named in a comma-separatedpackagesinput (defaultruntime). Decoupled from CI/release so downstream test/release workflows can consume its output from any run.pkg_type = "msi".What's in each hop
Upload hop (MSI → artifacts bucket, e.g.
therock-dev-artifacts):build_tools/packaging/windows/upload_package_repo.pyuploads*.msiviabackend.upload_directoryto{run_id}-windows/packages/msi(WorkflowOutputRoot.native_windows_packages("msi")); bucket auto-selected fromRELEASE_TYPE. Named to mirror Linux'supload_package_repo.py.if: github.repository_owner == 'ROCm'.Publish hop (artifacts → release bucket, e.g.
therock-repo-amd-dev-core):publish_native_windows_packages()inpublish_rocm_to_release_buckets.pycopies{run_id}-windows/packages/msi→v5/rocm/core/packages/msi/{date}-{run_id}(S3→S3copy_directory), wired into theplatform == "windows"dispatch. Runs from the release workflow, not this one — same split as Linux.Key mechanics
--run-id+RELEASE_TYPEand delegates toartifact_manager.py. The workflow forwardsartifact_run_idandartifact_github_repo(for packaging another repo's run, e.g. fork pre-merge testing against a ROCm run) andrelease_type.--packages(comma list) and writes one<output_stem>.wxsper package into--output-dir, so the workflow calls it once (no matrix) and loopswix buildover the produced.wxsfiles. The generator fetches the (package-independent) artifacts once and iterates the packages.amdhip64_6,amd_comgr_2) are DVC-tracked, not in git. The workflow initializes therocm-systemssubmodule at this checkout's pinned commit and materializes its DVC files withfetch_dvc_artifacts.py pull. The generator reads them from that source checkout and fails if any are missing.windows-2022runner; it is short and cheap, so no self-hosted scale-set is used.Scope
release_typedefaults toci(safe default); the dev flow is the first real destination. Nightly / prerelease / public destinations are intentionally out of scope for now.Remaining milestones (follow-ups, per #1987)
multi_arch_ci_windows.yml/multi_arch_release_windows.ymltest_native_linux_packages_install.yml)Test plan
generate_msi_wxs_test.py, 72) and the newupload_package_repo_test.pypass--packages/--output-dirgenerate + DVC materialization + thewix buildloop on awindows-2022runnerpre-commit(black + actionlint) and the workflow/publish unit tests pass on all changed filesworkflow_dispatchonmainto exercise the ROCm-only S3 upload +publish_rocm_to_release_buckets.py --platform windowspublish hopTracking: #1987
Conventions
Audited against
docs/development/style_guides/github_actions_style_guide.md,python_style_guide.md, anddocs/development/github_actions_debugging.md. No inline Python in the workflow; SHA-pinned actions,timeout-minuteson checkout/fetch steps, OIDCid-token: write,configure_aws_artifacts_credentials+release_type(defaultci), and the Python scripts' pathlib/type-hints/argparse style. Inputs match the siblingbuild_windows_python_packages.yml(artifact_run_id,artifact_github_repo).Pre-merge testing (fork dispatch)
Per
TESTING.md(linkworkflow_dispatchruns when a workflow has nopull_requesttrigger), validated end-to-end from a personal fork (idass1990/TheRock), which exercises the full generate + DVC +wix buildpath on awindows-2022runner. The S3 upload/publish steps skip on forks (if: github.repository_owner == 'ROCm'); those are covered by thepublish_rocm_to_release_buckets_testunit test and run for real viaworkflow_dispatchonmainpost-merge.--packages/--output-dir, submodule DVC fetch): https://github.com/idass1990/TheRock/actions/runs/36054025763 — ✅ greenruntimeMSI from nightly artifacts (35670146294-windows)amdhip64_7,amd_comgr,rocm_kpack(from artifacts) plusamdhip64_6,amd_comgr_2(materialized from the rocm-systems DVC files).wxs/.msinamed after the packageoutput_stem(amdrocm-runtime.msi) by the generator