Repository navigation
[aux-overlay] Use install(CODE) for directory symlinks - #4301
FIM43-Redeye wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the aux-overlay install logic to correctly handle symlinks that point to directories (not supported by install(FILES)), addressing failures in installing the llvm and amdgcn links (Fixes #3714).
Changes:
- Replace
install(FILES ...)usage for directory symlinks withinstall(CODE ...)+file(CREATE_LINK ...). - Adjust installation of tool symlinks to install individual
bin/*links intoDESTINATION bininstead of trying to install the wholebindirectory as a “file”.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
575be24 to
03d212d
Compare
|
Quick ping, can someone approve CI? The change is pretty small and it fixes #3714. |
b462a95 to
cb1e546
Compare
install(FILES) rejects symlinks that point to directories, which causes the llvm and amdgcn symlinks to fail during installation. Replace with install(CODE) using file(CREATE_LINK) to create the symlinks directly at install time, with correct targets matching the build-time symlinks (llvm -> lib/llvm, amdgcn -> lib/llvm/amdgcn). Fixes ROCm#3714 Generated using Claude Code.
cb1e546 to
25da31d
Compare
|
Hi, following up on this 'cause it's been about a month and the PR is still sitting in triage TODO. This is a smalal CMake fix that resolves #3714 (broken amdclang symlincs affecting ROCm installation), I've kept it rebased to current main. |
stellaraccident
left a comment
stellaraccident
left a comment
There was a problem hiding this comment.
LGTM. In general, PRs without a reviewer go to the bottom of the pile.
@amd-shiraz we should have CODEOWNERS and auto assign teams on TheRock so that things don't get lost.
|
FYI - I've got a relatively large Fortran refactor (#3928) which will mid-air collide with this one. Since I'm doing a lot of testing of that, I'm incorporating the fixes here into that one. Will aim to land right after 7.13 tags (~this coming week). |
Changes: - Restore rocprofiler-systems to the mainline compiler/dependency shape and keep only the Fortran-disabled example filter. - Drop the split offload pre-hook and amd-llvm-offload runtime edge from rocprofiler-systems. - Incorporate the aux-overlay directory-symlink install fix from #4301, with an install-prefix guard for standalone installs. Claude Code
|
Makes sense to me, closing as to-be-implemented-elsewhere. |
Changes: - Restore rocprofiler-systems to the mainline compiler/dependency shape and keep only the Fortran-disabled example filter. - Drop the split offload pre-hook and amd-llvm-offload runtime edge from rocprofiler-systems. - Incorporate the aux-overlay directory-symlink install fix from #4301, with an install-prefix guard for standalone installs. Claude Code
Partial landing split from #3928. Changes: - Replace install(FILES) for the root llvm and amdgcn directory symlinks with install(CODE) that creates the symlinks at install time. - Create the install prefix inside the install code before creating root-level symlinks; local validation showed the closed #4301 form failed when the prefix did not exist yet. - Leave the existing amdflang symlink behavior unchanged; conditional Fortran symlink handling remains with the split compiler work. Rationale: - CMake install(FILES) rejects directory symlinks, which breaks aux-overlay installation for the SDK layout symlinks tracked by #3714. Validation: - cmake -S base/aux-overlay -B /tmp/therock-aux-overlay-test-build -GNinja -DTHEROCK_SOURCE_DIR=/srv/vm-shared/projects/therock-workspace1/worktrees/TheRock -DTHEROCK_PACKAGE_VERSION=0.0.0 -DGPU_TARGETS=gfx1100 - cmake --build /tmp/therock-aux-overlay-test-build - cmake --install /tmp/therock-aux-overlay-test-build --prefix /tmp/therock-aux-overlay-test-install - verified llvm, amdgcn, and bin/amdclang are symlinks in /tmp/therock-aux-overlay-test-install - git diff --cached --check Generated with Codex
Summary
install(FILES)rejects symlinks that point to directories, breakinginstallation of the
llvmandamdgcnsymlinksinstall(CODE)usingfile(CREATE_LINK)to create themat install time
Fixes #3714