[PoC] Added MSI package building process#5712
Conversation
e4b2849 to
0ff4ac9
Compare
|
@ScottTodd @idass1990 while we are struggling with CI you can take a look at draft MSI artifacts here: Thank you @gkrivor ! |
save results as an artifacts
cd6e836 to
56bdfb5
Compare
Replaced unicode symbols to satisfy pre-commit check Fixed more stupid pre-commit requirements
56bdfb5 to
d9e6167
Compare
|
@copilot resolve the merge conflicts in this pull request |
…ll-v2 # Conflicts: # .github/workflows/release_windows_packages.yml
Resolved. The only conflict was a modify/delete on |
There was a problem hiding this comment.
Pull request overview
Adds a proof-of-concept Windows MSI packaging pipeline to TheRock, including WiX source definitions for multiple ROCm component MSIs, build scripts to generate them, and workflow automation to produce MSIs from CI artifacts. It also includes RFC docs capturing package file mappings and MSI-vs-DEB comparisons.
Changes:
- Introduce WiX v4 packaging definitions (
.wxs/.wxi) and per-package build scripts for multiple ROCm MSI packages. - Add a GitHub Actions job to fetch artifacts and build/upload MSI outputs on Windows runners.
- Add RFC0012 documentation artifacts (brief manifests + full MSI/DEB comparisons) for several packages.
Reviewed changes
Copilot reviewed 98 out of 108 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/rfcs/RFC0012/amdrocm-sparse-dev_full_cmp.md | Adds generated MSI vs DEB file comparison for amdrocm-sparse-dev. |
| docs/rfcs/RFC0012/amdrocm-sparse_full_cmp.md | Adds generated MSI vs DEB file comparison for amdrocm-sparse. |
| docs/rfcs/RFC0012/amdrocm-sparse_brief.md | Adds brief source-to-destination mapping for amdrocm-sparse.msi. |
| docs/rfcs/RFC0012/amdrocm-runtime-dev_brief.md | Adds brief source-to-destination mapping for amdrocm-runtime-dev.msi. |
| docs/rfcs/RFC0012/amdrocm-runtime_brief.md | Adds brief source-to-destination mapping for amdrocm-runtime.msi. |
| docs/rfcs/RFC0012/amdrocm-rand_full_cmp.md | Adds generated MSI vs DEB file comparison for amdrocm-rand. |
| docs/rfcs/RFC0012/amdrocm-rand_brief.md | Adds brief source-to-destination mapping for amdrocm-rand.msi. |
| docs/rfcs/RFC0012/amdrocm-math-common_full_cmp.md | Adds generated MSI vs DEB file comparison for amdrocm-math-common. |
| docs/rfcs/RFC0012/amdrocm-math-common_brief.md | Adds brief source-to-destination mapping for amdrocm-math-common.msi. |
| docs/rfcs/RFC0012/amdrocm-fft-dev_full_cmp.md | Adds generated MSI vs DEB file comparison for amdrocm-fft-dev. |
| docs/rfcs/RFC0012/amdrocm-fft-dev_brief.md | Adds brief source-to-destination mapping for amdrocm-fft-dev.msi. |
| docs/rfcs/RFC0012/amdrocm-fft_full_cmp.md | Adds generated MSI vs DEB file comparison for amdrocm-fft. |
| docs/rfcs/RFC0012/amdrocm-fft_brief.md | Adds brief source-to-destination mapping for amdrocm-fft.msi. |
| docs/rfcs/RFC0012/amdrocm-dnn_brief.md | Adds brief source-to-destination mapping for amdrocm-dnn.msi. |
| docs/rfcs/RFC0012/amdrocm-blas-dev_full_cmp.md | Adds generated MSI vs DEB file comparison for amdrocm-blas-dev. |
| docs/rfcs/RFC0012/amdrocm-blas-dev_brief.md | Adds brief source-to-destination mapping for amdrocm-blas-dev.msi. |
| docs/rfcs/RFC0012/amdrocm-blas_full_cmp.md | Adds generated MSI vs DEB file comparison for amdrocm-blas. |
| docs/rfcs/RFC0012/amdrocm-blas_brief.md | Adds brief source-to-destination mapping for amdrocm-blas.msi. |
| build_tools/packaging/windows/common.wxi | Defines shared WiX preprocessor variables (product/version/build root). |
| build_tools/packaging/windows/build-all.bat | Adds a batch driver to build all MSI packages. |
| build_tools/packaging/windows/amdrocm-sparse.wxs | WiX package definition for amdrocm-sparse. |
| build_tools/packaging/windows/amdrocm-sparse-structure.wxi | Directory structure for amdrocm-sparse. |
| build_tools/packaging/windows/amdrocm-sparse-feature.wxi | Feature/component group refs for amdrocm-sparse. |
| build_tools/packaging/windows/amdrocm-sparse-components.wxi | Component definitions for amdrocm-sparse. |
| build_tools/packaging/windows/amdrocm-sparse-build.bat | Build script for amdrocm-sparse.msi. |
| build_tools/packaging/windows/amdrocm-sparse-dev.wxs | WiX package definition for amdrocm-sparse-dev. |
| build_tools/packaging/windows/amdrocm-sparse-dev-structure.wxi | Directory structure for amdrocm-sparse-dev. |
| build_tools/packaging/windows/amdrocm-sparse-dev-feature.wxi | Feature/component group refs for amdrocm-sparse-dev. |
| build_tools/packaging/windows/amdrocm-sparse-dev-components.wxi | Component definitions for amdrocm-sparse-dev. |
| build_tools/packaging/windows/amdrocm-sparse-dev-build.bat | Build script for amdrocm-sparse-dev.msi. |
| build_tools/packaging/windows/amdrocm-runtime.wxs | WiX package definition for amdrocm-runtime. |
| build_tools/packaging/windows/amdrocm-runtime-structure.wxi | Directory structure for amdrocm-runtime. |
| build_tools/packaging/windows/amdrocm-runtime-feature.wxi | Feature/component group refs for amdrocm-runtime. |
| build_tools/packaging/windows/amdrocm-runtime-components.wxi | Component definitions for amdrocm-runtime. |
| build_tools/packaging/windows/amdrocm-runtime-build.bat | Build script for amdrocm-runtime.msi. |
| build_tools/packaging/windows/amdrocm-runtime-dev.wxs | WiX package definition for amdrocm-runtime-dev. |
| build_tools/packaging/windows/amdrocm-runtime-dev-structure.wxi | Directory structure for amdrocm-runtime-dev. |
| build_tools/packaging/windows/amdrocm-runtime-dev-feature.wxi | Feature/component group refs for amdrocm-runtime-dev. |
| build_tools/packaging/windows/amdrocm-runtime-dev-components.wxi | Component definitions for amdrocm-runtime-dev. |
| build_tools/packaging/windows/amdrocm-runtime-dev-build.bat | Build script for amdrocm-runtime-dev.msi. |
| build_tools/packaging/windows/amdrocm-rand.wxs | WiX package definition for amdrocm-rand. |
| build_tools/packaging/windows/amdrocm-rand-structure.wxi | Directory structure for amdrocm-rand. |
| build_tools/packaging/windows/amdrocm-rand-feature.wxi | Feature/component group refs for amdrocm-rand. |
| build_tools/packaging/windows/amdrocm-rand-components.wxi | Component definitions for amdrocm-rand. |
| build_tools/packaging/windows/amdrocm-rand-build.bat | Build script for amdrocm-rand.msi. |
| build_tools/packaging/windows/amdrocm-math-common.wxs | WiX package definition for amdrocm-math-common. |
| build_tools/packaging/windows/amdrocm-math-common-structure.wxi | Directory structure for amdrocm-math-common. |
| build_tools/packaging/windows/amdrocm-math-common-feature.wxi | Feature/component group refs for amdrocm-math-common. |
| build_tools/packaging/windows/amdrocm-math-common-components.wxi | Component definitions for amdrocm-math-common. |
| build_tools/packaging/windows/amdrocm-math-common-build.bat | Build script for amdrocm-math-common.msi. |
| build_tools/packaging/windows/amdrocm-fft.wxs | WiX package definition for amdrocm-fft. |
| build_tools/packaging/windows/amdrocm-fft-structure.wxi | Directory structure for amdrocm-fft. |
| build_tools/packaging/windows/amdrocm-fft-feature.wxi | Feature/component group refs for amdrocm-fft. |
| build_tools/packaging/windows/amdrocm-fft-components.wxi | Component definitions for amdrocm-fft. |
| build_tools/packaging/windows/amdrocm-fft-build.bat | Build script for amdrocm-fft.msi. |
| build_tools/packaging/windows/amdrocm-fft-dev.wxs | WiX package definition for amdrocm-fft-dev. |
| build_tools/packaging/windows/amdrocm-fft-dev-structure.wxi | Directory structure for amdrocm-fft-dev. |
| build_tools/packaging/windows/amdrocm-fft-dev-feature.wxi | Feature/component group refs for amdrocm-fft-dev. |
| build_tools/packaging/windows/amdrocm-fft-dev-components.wxi | Component definitions for amdrocm-fft-dev. |
| build_tools/packaging/windows/amdrocm-fft-dev-build.bat | Build script for amdrocm-fft-dev.msi. |
| build_tools/packaging/windows/amdrocm-llvm.wxs | WiX package definition for amdrocm-llvm. |
| build_tools/packaging/windows/amdrocm-llvm-structure.wxi | Directory structure for amdrocm-llvm. |
| build_tools/packaging/windows/amdrocm-llvm-feature.wxi | Feature/component group refs for amdrocm-llvm. |
| build_tools/packaging/windows/amdrocm-llvm-build.bat | Build script for amdrocm-llvm.msi. |
| build_tools/packaging/windows/amdrocm-llvm-dev.wxs | WiX package definition for amdrocm-llvm-dev. |
| build_tools/packaging/windows/amdrocm-llvm-dev-structure.wxi | Directory structure for amdrocm-llvm-dev. |
| build_tools/packaging/windows/amdrocm-llvm-dev-feature.wxi | Feature/component group refs for amdrocm-llvm-dev. |
| build_tools/packaging/windows/amdrocm-llvm-dev-build.bat | Build script for amdrocm-llvm-dev.msi. |
| build_tools/packaging/windows/amdrocm-dnn.wxs | WiX package definition for amdrocm-dnn. |
| build_tools/packaging/windows/amdrocm-dnn-structure.wxi | Directory structure for amdrocm-dnn. |
| build_tools/packaging/windows/amdrocm-dnn-feature.wxi | Feature/component group refs for amdrocm-dnn. |
| build_tools/packaging/windows/amdrocm-dnn-components.wxi | Component definitions for amdrocm-dnn. |
| build_tools/packaging/windows/amdrocm-dnn-build.bat | Build script for amdrocm-dnn.msi. |
| build_tools/packaging/windows/amdrocm-dnn-dev.wxs | WiX package definition for amdrocm-dnn-dev. |
| build_tools/packaging/windows/amdrocm-dnn-dev-structure.wxi | Directory structure for amdrocm-dnn-dev. |
| build_tools/packaging/windows/amdrocm-dnn-dev-feature.wxi | Feature/component group refs for amdrocm-dnn-dev. |
| build_tools/packaging/windows/amdrocm-dnn-dev-components.wxi | Component definitions for amdrocm-dnn-dev. |
| build_tools/packaging/windows/amdrocm-dnn-dev-build.bat | Build script for amdrocm-dnn-dev.msi. |
| build_tools/packaging/windows/amdrocm-ck.wxs | WiX package definition for amdrocm-ck. |
| build_tools/packaging/windows/amdrocm-ck-structure.wxi | Directory structure for amdrocm-ck. |
| build_tools/packaging/windows/amdrocm-ck-feature.wxi | Feature/component group refs for amdrocm-ck. |
| build_tools/packaging/windows/amdrocm-ck-components.wxi | Component definitions for amdrocm-ck. |
| build_tools/packaging/windows/amdrocm-ck-build.bat | Build script for amdrocm-ck.msi. |
| build_tools/packaging/windows/amdrocm-blas.wxs | WiX package definition for amdrocm-blas. |
| build_tools/packaging/windows/amdrocm-blas-structure.wxi | Directory structure for amdrocm-blas. |
| build_tools/packaging/windows/amdrocm-blas-feature.wxi | Feature/component group refs for amdrocm-blas. |
| build_tools/packaging/windows/amdrocm-blas-components.wxi | Component definitions for amdrocm-blas. |
| build_tools/packaging/windows/amdrocm-blas-build.bat | Build script for amdrocm-blas.msi. |
| build_tools/packaging/windows/amdrocm-blas-dev.wxs | WiX package definition for amdrocm-blas-dev. |
| build_tools/packaging/windows/amdrocm-blas-dev-structure.wxi | Directory structure for amdrocm-blas-dev. |
| build_tools/packaging/windows/amdrocm-blas-dev-feature.wxi | Feature/component group refs for amdrocm-blas-dev. |
| build_tools/packaging/windows/amdrocm-blas-dev-components.wxi | Component definitions for amdrocm-blas-dev. |
| build_tools/packaging/windows/amdrocm-blas-dev-build.bat | Build script for amdrocm-blas-dev.msi. |
| .github/workflows/multi_arch_release_windows.yml | Adds a Windows job to build MSI packages via WiX and upload them as artifacts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for /f %%I IN ('dir /b "%~dp0*-build.bat"') do ( | ||
| call "%~dp0%%I" /no_pause | ||
| if !ERRORLEVEL! neq 0 ( | ||
| rem popd | ||
| rem echo Restored directory | ||
| rem exit /b 1 | ||
| ) | ||
| ) | ||
| if not "%~1" == "" ( | ||
| popd | ||
| echo Restored directory | ||
| ) | ||
| echo Finished | ||
| if not "%~2" == "/no_pause" ( | ||
| pause | ||
| ) | ||
| @echo on | ||
| exit /b 0 |
| permissions: | ||
| id-token: write |
| python build_tools/artifact_manager.py fetch \ | ||
| --run-id="${{ github.run_id }}" \ | ||
| --stage=all \ | ||
| --amdgpu-families="${{ matrix.family_info.amdgpu_family }}" \ | ||
| --expand-family-to-targets \ | ||
| --flatten \ | ||
| --output-dir="${{ env.DIST_DIR }}" |
| <?define ProductName = "ROCm" ?> | ||
| <?define ProductVersion = "7.14" ?> | ||
| <?define UpgradeCode = "9C8E3A7E-9B6D-4A9D-A6E1-AB3A2C1B7F11" ?> | ||
| <?define BuildRoot = "B:\build" ?> |
| <StandardDirectory Id="ProgramFiles64Folder"> | ||
| <Directory Id="INSTALLFOLDER" Name="AMD\$(var.ProductName)\$(var.ProductVersion)"> | ||
| <?include amdrocm-sparse-structure.wxi ?> | ||
| </Directory> | ||
| </StandardDirectory> |
| <StandardDirectory Id="ProgramFiles64Folder"> | ||
| <Directory Id="INSTALLFOLDER" Name="AMD\$(var.ProductName)\$(var.ProductVersion)"> | ||
| <?include amdrocm-ck-structure.wxi ?> | ||
| </Directory> | ||
| </StandardDirectory> |
| <StandardDirectory Id="ProgramFiles64Folder"> | ||
| <Directory Id="INSTALLFOLDER" Name="AMD\$(var.ProductName)\$(var.ProductVersion)"> | ||
| <?include amdrocm-dnn-structure.wxi ?> | ||
| </Directory> | ||
| </StandardDirectory> |
| <StandardDirectory Id="ProgramFiles64Folder"> | ||
| <Directory Id="INSTALLFOLDER" Name="AMD\$(var.ProductName)\$(var.ProductVersion)"> | ||
| <?include amdrocm-dnn-dev-structure.wxi ?> | ||
| </Directory> | ||
| </StandardDirectory> |
| <StandardDirectory Id="ProgramFiles64Folder"> | ||
| <Directory Id="INSTALLFOLDER" Name="AMD\$(var.ProductName)\$(var.ProductVersion)"> | ||
| <?include amdrocm-blas-structure.wxi ?> | ||
| </Directory> | ||
| </StandardDirectory> |
| <StandardDirectory Id="ProgramFiles64Folder"> | ||
| <Directory Id="INSTALLFOLDER" Name="AMD\$(var.ProductName)\$(var.ProductVersion)"> | ||
| <?include amdrocm-blas-dev-structure.wxi ?> | ||
| </Directory> | ||
| </StandardDirectory> |
idass1990
left a comment
There was a problem hiding this comment.
Code Review: PR #5712 — [PoC] Added MSI package building process (Updated)
Author: gkrivor | Base: main | Files: 108 changed | +33,276 / −0
Reviewed against PR #4803 commit f5339ec (June 12, 2026)
What Changed in PR #4803 Since the Original Review
PR #4803 received two significant commits on June 12 that are directly relevant:
Commit 8fe9dd6 — "packaging: rewrite MSI generator with artifact TOML support"
- Replaces the static allowlist (
amdrocm-runtimes-artifacts.txt) with TOML descriptor-driven generation - Stage-dir scoping: globs run against per-component stage dirs (
build/<basedir>/stage/) preventing cross-artifact bleed --artifacts-urlflag to pull.tar.zstartifacts from S3 without a local build- Deterministic WiX element IDs via SHA-256
- Full unit test suite (
generate_msi_wxs_test.py, 509 lines) - Flat install layout (
bin/,lib/,share/directly underInstallDir) INSTALLFOLDERuser-overridable atmsiexecinvocation time
Commit f5339ec — "packaging: remove generated wxs, artifacts list, and wixpdb"
- Removes
amdrocm-runtimes.wxs,amdrocm-runtimes-artifacts.txt,amdrocm-runtimes.wixpdbfrom the repo - Addresses ScottTodd's binary-file and drift concerns directly
Updated ScottTodd Concern Tracking
| ScottTodd concern (PR #4803) | Status in PR #4803 now | Status in PR #5712 |
|---|---|---|
| Land RFC #3973 first | Unresolved — RFC still open | Not addressed |
| System32 DLL deletion needs plan | Removed from scope (commit 2026-05-13) | Not applicable |
Why only bin/ and lib/? |
Partially addressed — TOML descriptors now define scope per artifact | Persists — blas runtime still only ships bin/ |
| Document prebuilt artifact usage | Addressed in rewritten msi-generator-usage.md (--artifacts-url) |
Partially addressed via CI, no dev docs |
| Long-paths registry key on uninstall | Still open (promised but not yet committed) | Not applicable |
.wixpdb in .gitignore |
.wixpdb removed from commit; no .gitignore update yet |
Not addressed |
| Artifact list drift vs Linux packages | Addressed — TOML descriptors are shared source of truth with build system | Regressed — 108 hand-maintained files |
| Hardcoded versions → use generator | Addressed — _read_rocm_version() extracts version from build tree |
Regressed — 1.0.0.0 / 7.14 hardcoded |
| Generate component lists from BOM | Addressed — generate_msi_wxs.py fully replaces manual lists |
Regressed — abandoned generator entirely |
Don't hardcode build/dist paths |
Addressed — paths are relative to stage dirs or dist_root arg |
Critical regression — B:\build hardcoded |
| Version-specific registry key | Still open (not committed yet) | Not applicable |
The approaches in PR #4803 and PR #5712 have fundamentally diverged. #4803 chose a generator-script approach that addresses nearly all of ScottTodd's concerns. #5712 chose a hand-authored static approach that regresses on all of them.
New Findings from Updated Review
CRITICAL — WiX Directory/@Name backslash bug (confirmed by Copilot, all 11 packages affected)
Every .wxs file in PR #5712 has this pattern:
<Directory Id="INSTALLFOLDER" Name="AMD\$(var.ProductName)\$(var.ProductVersion)">In WiX v4, Directory/@Name must be a single directory segment — backslashes are not path separators, they are literal characters in the directory name. WiX will either error or create a directory literally named AMD\ROCm\7.14 as a single folder rather than a three-level hierarchy. This is confirmed by Copilot in 11 separate inline comments. All 11 packages are affected. The correct form is:
<StandardDirectory Id="ProgramFiles64Folder">
<Directory Id="dir_amd" Name="AMD">
<Directory Id="dir_rocm" Name="$(var.ProductName)">
<Directory Id="INSTALLFOLDER" Name="$(var.ProductVersion)">This is a WiX correctness bug that will either fail at compile time or produce broken installs. This alone blocks the PR.
CRITICAL — Flattened artifact layout vs. deep build-tree paths (Copilot confirmed)
The CI workflow in multi_arch_release_windows.yml calls:
python build_tools/artifact_manager.py fetch --flatten --output-dir="${{ env.DIST_DIR }}"But common.wxi defines:
<?define BuildRoot = "B:\build" ?>And component files reference deep build-tree paths like:
Source="$(var.BuildRoot)/math-libs/BLAS/hipBLAS/dist/bin/hipblas.dll"With --flatten, artifacts land flat in DIST_DIR — there are no math-libs/BLAS/hipBLAS/dist/ subdirectories. The source paths will not exist. Every MSI build in the CI job will fail even if BuildRoot were corrected. The WiX source files would need to be rewritten to reference flat paths, or the artifact fetch would need to not use --flatten.
CRITICAL — Hardcoded BuildRoot = "B:\build" — fatal portability defect
common.wxi:
<?define BuildRoot = "B:\build" ?>This hardcodes an absolute path to a specific build machine's drive letter. It will fail on every other machine. ScottTodd raised this exact issue on PR #4803. The fix is to pass BuildRoot as a -d BuildRoot=... define argument to wix build, sourced from the calling .bat or CI workflow, not hardcoded in the shared include.
CRITICAL — Hardcoded version 1.0.0.0 and ProductVersion = "7.14" throughout
Every .wxs has Version="1.0.0.0" and common.wxi has ProductVersion = "7.14". These break upgrade detection and silent installs. The version must come from the build system. PR #4803 now has _read_rocm_version() that extracts the version from the build tree — the same mechanism should be used here.
HIGH — contents: read missing from CI job permissions (Copilot confirmed)
build_msi_packages:
permissions:
id-token: write # only permission setWhen a job sets any permission explicitly, all others default to none. actions/checkout requires contents: read. Without it, the checkout step will fail on repositories with restricted default permissions. Fix:
permissions:
id-token: write
contents: readHIGH — build-all.bat silently swallows failures (Copilot confirmed)
for /f %%I IN ('dir /b "%~dp0*-build.bat"') do (
call "%~dp0%%I" /no_pause
if !ERRORLEVEL! neq 0 (
rem popd
rem exit /b 1 ← COMMENTED OUT
)
)Individual package build failures are silently ignored. The CI step will report success even if every MSI fails to build. The exit /b 1 must be uncommented, or at minimum the loop must accumulate failures and exit non-zero at the end.
HIGH — MSI S3 upload logic silently lost in merge conflict resolution
Copilot resolved the merge conflict on June 13 by accepting main's deletion of release_windows_packages.yml (removed in main PR #5790). The S3 upload sections that were added to that file in PR #5712 are now silently gone. If S3 upload of MSIs is required for the release pipeline, that logic needs to be added to whichever workflow now handles releases.
MEDIUM — No inter-package dependency declarations (dep:Requires)
Each .wxs declares dep: namespace but never uses <dep:Requires>. For example, amdrocm-blas-dev has no declared dependency on amdrocm-blas. Users can install dev headers without the runtime DLLs and get a broken environment.
MEDIUM — WiX extension versions not locked
dotnet tool install --version 6.0.0 --global wix
wix extension add -g WixToolset.UI.wixext/6.0.0
wix extension add -g WixToolset.Dependency.wixext/6.0.0The WiX tool itself is pinned, but extensions are fetched from NuGet at install time. Use a dotnet-tools.json manifest or checked-in wix.json to lock extension versions.
LOW — build-all.bat script discovery is fragile
The script discovers build scripts by globbing *-build.bat with no defined ordering. Any temporary file matching that pattern will be invoked. An explicit ordered list is safer.
LOW — Missing .gitignore for WiX build artifacts
No .gitignore entry for *.msi, *.wixpdb, or *.wixobj. ScottTodd called this out on #4803. PR #4803 already removed a committed .wixpdb; without .gitignore the problem will recur.
LOW — PR description is entirely blank
The PR template sections (Motivation, Technical Details, Test Plan, Test Result) are all unfilled. This should be filled before requesting review.
Comparison: PR #4803 Approach vs. PR #5712 Approach
| Dimension | PR #4803 (generate_msi_wxs.py) |
PR #5712 (static .wxs) |
|---|---|---|
| File list source | TOML artifact descriptors (build system) | Hand-authored per-file in .wxi includes |
| Versioning | _read_rocm_version() from build tree |
Hardcoded 1.0.0.0 / 7.14 |
| Path portability | Relative to stage dir / dist_root arg |
Hardcoded B:\build |
| Maintenance burden | Low — add a file to TOML, regenerate | High — manually add component to .wxi |
| Test coverage | 509-line unit test suite | None |
| Build output committed | No (removed in latest commit) | N/A |
.gitignore |
Not yet updated | Not updated |
| Inter-package deps | Not implemented | Not implemented |
WiX Directory correctness |
Correct (flat layout, no backslash in Name) | Incorrect — backslash in Directory/@Name on all 11 packages |
| CI artifact path alignment | Designed to work with --artifacts-url |
Incompatible with --flatten CI fetch |
Recommendation
PR #5712 should not be merged. Beyond the original blockers, there are now two additional correctness bugs confirmed by Copilot's own review — invalid Directory/@Name syntax affecting all 11 packages, and fundamental incompatibility between the CI artifact fetch mode (--flatten) and the deep build-tree source paths in the WiX files. The S3 upload work was silently lost in the merge conflict resolution.
The more viable path for this work is PR #4803, which now has:
- A complete generator script addressing all of ScottTodd's architectural concerns
- Unit test coverage (509 lines)
- Correct portability (no hardcoded paths or versions)
- TOML-driven artifact sourcing aligned with the build system
What PR #4803 still needs before it can merge:
- RFC #3973 landing (or explicit team waiver)
.gitignoreupdate for*.msi,*.wixpdb,*.wixobj- Long-paths registry key uninstall behavior clarified and implemented
dep:Requiresinter-package dependency declarations- CI integration (workflow jobs could be adapted from PR #5712's approach, using the generator script)
idass1990
left a comment
There was a problem hiding this comment.
see my review breakdown in comments
ScottTodd
left a comment
There was a problem hiding this comment.
This PR has no description or documentation for adding +33k LOC. Please read https://github.com/ROCm/TheRock/blob/main/CONTRIBUTING.md (in particular https://github.com/ROCm/TheRock/blob/main/CONTRIBUTING.md#new-feature-development - large PRs without prior communication are unlikely to be accepted, especially if another PR is already in review for the same feature)
There was a problem hiding this comment.
This overlaps with work that @idass1990 has been doing on #4803. Please coordinate on
| <!-- Contents of directory include\hipdnn\frontend\hipdnn_frontend --> | ||
| <ComponentGroup Id="cg_amdrocm_dnn_dev_ihfhf" Directory="amdrocm_dnn_dev_ihfhf"> | ||
| <Component Id="c_amdrocm_dnn_dev_ihfhf_E.hpp" Guid="*"> | ||
| <File Id="f_amdrocm_dnn_dev_ihfhf_E.hpp" Source="$(var.BuildRoot)/ml-libs/hipDNN/dist/include/hipdnn/frontend/hipdnn_frontend/Error.hpp" KeyPath="yes" /> | ||
| </Component> | ||
| <Component Id="c_amdrocm_dnn_dev_ihfhf_G.hpp" Guid="*"> | ||
| <File Id="f_amdrocm_dnn_dev_ihfhf_G.hpp" Source="$(var.BuildRoot)/ml-libs/hipDNN/dist/include/hipdnn/frontend/hipdnn_frontend/Graph.hpp" KeyPath="yes" /> | ||
| </Component> | ||
| <Component Id="c_amdrocm_dnn_dev_ihfhf_H.hpp" Guid="*"> | ||
| <File Id="f_amdrocm_dnn_dev_ihfhf_H.hpp" Source="$(var.BuildRoot)/ml-libs/hipDNN/dist/include/hipdnn/frontend/hipdnn_frontend/Handle.hpp" KeyPath="yes" /> | ||
| </Component> | ||
| <Component Id="c_amdrocm_dnn_dev_ihfhf_L.hpp" Guid="*"> | ||
| <File Id="f_amdrocm_dnn_dev_ihfhf_L.hpp" Source="$(var.BuildRoot)/ml-libs/hipDNN/dist/include/hipdnn/frontend/hipdnn_frontend/Logging.hpp" KeyPath="yes" /> | ||
| </Component> |
There was a problem hiding this comment.
These files need to be generated dynamically based on build system outputs, as the already-in-review #4803 does. We're not going to check in 30k LOC of generated code that needs to be continually updated whenever someone adds a new Logging.hpp to any one of our repositories.
There was a problem hiding this comment.
You have a wrong interpretation. Original expectations were: I just take dist/* data and pack it into a package using **/. mask. But current build process produces about 200k+ files where only about 12k+ are unique. Such parts highlight a component/build system issue when it produces a wrong structure.
But for now it is enough to build a package without unused files.
| - name: Upload MSI Packages as artifacts | ||
| uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 | ||
| if: ${{ !cancelled() }} | ||
| with: | ||
| name: msi-packages-${{ matrix.family_info.amdgpu_family }} | ||
| path: ${{ env.DIST_DIR }}\*.msi | ||
| if-no-files-found: warn |
There was a problem hiding this comment.
We don't use github artifacts on this project. We use AWS S3 buckets. See
There was a problem hiding this comment.
GitHub artifacts used because AWS S3 wasn't working correct in Release Windows action.
Would be great if you could cover this part or add someone who could.
| --release-type="${{ inputs.release_type }}" \ | ||
| --kpack-split="${{ needs.build_python_packages.outputs.kpack_split }}" | ||
| build_msi_packages: |
There was a problem hiding this comment.
Start with the ability to build packages locally in one PR, then connect to release workflows in another PR. This is too risky to review and land in a single PR, we need to be able to test it locally.
There was a problem hiding this comment.
Good to know.
But I'm not sure it is good for this case. When you separate the logic, you may get an infinity loop with merge conflicts/fixes in a second part. This PR may/will also include tests which will prevent any unexpected changes in Windows build/packaging process.
So, it depends on an actual state of repositories and how flexible/robust they will be.
There was a problem hiding this comment.
Especially regarding misunderstanding of CI-based painful development process :)
You can do MUCH better than a "CI-based painful development process" for this. See https://github.com/ROCm/TheRock/blob/main/docs/development/development_guide.md#overall-build-architecture and https://github.com/ROCm/TheRock/blob/main/docs/development/github_actions_debugging.md#working-effectively-from-forks
- Put the package construction code in a script that operates on artifacts
- Build packages locally, do most of your iteration there
- Add a standalone workflow that downloads artifacts, produces packages, then uploads packages. most of these are even runnable from personal fork repositories on github-hosted runners and complete in < 10 minutes.:
- Call that workflow from release workflows
Adding to the release workflow is the final step after all other testing is complete. You shouldn't have to do much iteration at all on that point, so 4hour+ runs like https://github.com/ROCm/TheRock/actions/runs/27404516888 are just an inefficient use of CI and developer time. The risk of "an infinity loop with merge conflicts/fixes in a second part" is low since the code is all simple plumbing: scripts -> call scripts from standalone workflow -> call standalone workflow from release workflow. See also https://github.com/ROCm/TheRock/blob/main/docs/development/style_guides/github_actions_style_guide.md#prefer-python-scripts-over-inline-bash
|
@ScottTodd it is a "prior communication" process right now :) I just shared an object of the discussion, not an actual code review request :) |
|
@idass1990, @andrei-kochin please, do not request Copilot at this stage. It does a wrong interpretation of the code. Especially regarding misunderstanding of CI-based painful development process :) |
Motivation
Adds a proof-of-concept Windows MSI packaging pipeline to TheRock, including WiX source definitions for multiple ROCm component MSIs, build scripts to generate them, and workflow automation to produce MSIs from CI artifacts. It also includes RFC docs capturing package file mappings and MSI-vs-DEB comparisons.
Technical Details
Introduce WiX v4 packaging definitions (.wxs/.wxi) and per-package build scripts for multiple ROCm MSI packages.
Add a GitHub Actions job to fetch artifacts and build/upload MSI outputs on Windows runners.
Add RFC0012 documentation artifacts (brief manifests + full MSI/DEB comparisons) for several packages.
Test Plan
Test Result
Submission Checklist