[AIMIGRAPHX-1186] TheRock packaging: amdrocm-migraphx package name (RFC0009) - #5086
Merged
Conversation
…ng weak deps(recommend/suggest) to hard deps
…packaging (rfc0009)
kentqian
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request updates the TheRock packaging path in CMakeLists.txt (MIGRAPHX_PACKAGE_BACKEND=therock) to shift per-GPU dependency selection to per-arch meta packages with hard dependencies, and aligns the TheRock package naming with the amdrocm-* ecosystem (RFC0009). The default (non-TheRock) ROCm packaging path is intended to remain functionally unchanged.
Changes:
- Switch TheRock package naming from
MIGraphXtoamdrocm-migraphxand plumb the package name intorocm_create_package(NAME ...). - For per-arch mode, create per-arch meta package components (e.g.,
gfx90a) plus anallmeta component, installing a small README into each component’s doc dir. - Retain the existing device-all dependency behavior when
MIGRAPHX_THEROCK_GPU_ARCHis empty.
Comment on lines
+475
to
+477
| install(FILES ${_MGX_META_DOC} | ||
| DESTINATION share/doc/${_MGX_PKG_NAME}-${mgx_gpu_arch} | ||
| COMPONENT ${mgx_gpu_arch}) |
Comment on lines
+485
to
+487
| install(FILES ${_MGX_META_DOC} | ||
| DESTINATION share/doc/${_MGX_PKG_NAME}-all | ||
| COMPONENT all) |
Comment on lines
+502
to
+503
| # Legacy ROCm packaging. | ||
| set(_MGX_PKG_NAME "MIGraphX") |
Check flagged results 🔆 |
|
…move per-arch to UFB
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #5086 +/- ##
========================================
Coverage 93.26% 93.26%
========================================
Files 623 623
Lines 32969 32969
========================================
Hits 30747 30747
Misses 2222 2222 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
CMakeLists.txt:475
- The else-branch comment was changed to "Legacy ROCm packaging", but this is still the default/traditional packaging backend. Calling it "legacy" is potentially misleading in configuration output and future maintenance.
# Legacy ROCm packaging.
set(_MGX_PKG_NAME "MIGraphX")
Comment on lines
451
to
454
| set(MIGRAPHX_THEROCK_GPU_ARCH "" CACHE STRING | ||
| "TheRock GPU arch(es) for per-GPU package dependencies. \ | ||
| Semicolon-separated list (e.g. gfx90a;gfx942;gfx950) for per-GPU deps, \ | ||
| or empty for device-all meta-package deps.") |
Comment on lines
+457
to
+461
| # Self-contained per-arch packages (amdrocm-migraphx-gfxNNN) and their per-GPU | ||
| # amdrocm-* deps are produced by the UFB Dockerfile cpack loop: one fat build, | ||
| # repackaged once per arch. Here we set only the base name + arch-neutral | ||
| # runtime dep; the device-all deps below are the standalone default and are | ||
| # overridden per-arch (amdrocm-dnn/blas-gfxNNN) at cpack time. |
kentqian
changed the title
Comment on lines
+453
to
+457
| # Only arch-neutral runtime deps are declared here (amdrocm-runtime), since | ||
| # that is the only component published as an arch-neutral package. | ||
| # The device libraries (MIOpen/rocBLAS) exist ONLY as per-arch packages | ||
| # (amdrocm-dnn-gfxNNN / amdrocm-blas-gfxNNN) with no arch-neutral variant, | ||
| # so they are intentionally omitted here. To produce a package with device |
eddieliao
approved these changes
Jul 24, 2026
causten
approved these changes
Jul 24, 2026
The TheRock package name and the .deb/.rpm filename disagree: the filename is already amdrocm-migraphx (set in UFB) while rocm_create_package still emits migraphx, so apt/dnf see a different name than the file advertises. Fixing that only needs _MGX_PKG_NAME plumbed into rocm_create_package. Dropping the per-arch dependency block is a separate design question that overlaps with #4952, and it was holding up a rename that is otherwise independent. That work moves to kqian1/therock-drop-per-arch-deps. Co-authored-by: Cursor <cursoragent@cursor.com>
kentqian
changed the title
kahmed10
approved these changes
Jul 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
amdrocm-migraphxper RFC0009 (Package name now matches the .deb/.rpm filename and theamdrocm-*ecosystem).Drop the weak Recommends/Suggests used for per-GPU ROCm libs.→ moved out, see Bump CI to TheRock 7.14 #4952Technical Details
The
.deb/.rpmfilename is alreadyamdrocm-migraphx(set at packagingtime in UFB), but
rocm_create_package()still emitsmigraphxas the packagename, so
apt/dnfreport a different name than the file advertises. Thisplumbs
_MGX_PKG_NAMEintorocm_create_package(NAME ...)so the two agree.This mismatch is what required the 7.14 release RPM's
NAMEtag to be rewrittenby hand, and it is the inconsistency DevOps is currently tracking.
One fat compile with multi-archs; UFB re-runs cpack once per GPU arch →amdrocm-migraphx-gfxNNN, each hard-depending on that arch'samdrocm-dnn/blas-gfxNNN+amdrocm-runtime.Compatibility / scope
migraphx's own device code stays a single fat ELF (one compile); only naming/deps change.Only the package name changes; no build or dependency behaviour is touched.Arch-specific dependencies (moved out of CMake)Per-GPU device-library dependencies are no longer declared in CMake. In the TheRock repo, MIOpen/rocBLAS ship only as per-arch packages (amdrocm-dnn-gfxNNN/amdrocm-blas-gfxNNN) with no arch-neutral variant, so CMake now declares only the arch-neutralamdrocm-runtimedependency.The per-arch device deps are injected externally at packaging time (one fat build, repackaged per arch). To produce a package with device dependencies, they must be added externally per target arch (amdrocm-dnn-<arch>,amdrocm-blas-<arch>).Encoding per-arch dependency selection inside CMake would be cumbersome and redundant, and no longer fits the original single-package ROCm layout — hence it is intentionally kept out of the migraphx build.Relationship to #4952
#4952 (Bump CI to TheRock 7.14) touches the same file but does not change the
package name (
NAME MIGraphXis unchanged there), so it does not address thisinconsistency and neither PR blocks the other. After narrowing, a test merge
between the two produces a single mechanical conflict in
CMakeLists.txt,resolved by taking #4952's side and keeping one line: