Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Minor CMake Changes #303

Merged
merged 10 commits into from
Dec 10, 2024
Merged

Minor CMake Changes #303

merged 10 commits into from
Dec 10, 2024

Conversation

dlangbe
Copy link
Collaborator

@dlangbe dlangbe commented Nov 27, 2024

-Changed AMDGPU_TARGETS to GPU_TARGETS. AMDGPU_TARGETS will be deprecated in the future, but will work the same for the time being.
-Removed the use of CMAKE_NO_BUILTIN_CHRPATH when --offload-compress is supported.

@amd-jnovotny
Copy link
Contributor

@dlangbe Should the build configuration table be updated in docs/install/installation.rst too?

Change log changes look good.

amd-jnovotny
amd-jnovotny previously approved these changes Nov 27, 2024
View reviewed changes
Copy link
Contributor

@amd-jnovotny amd-jnovotny left a comment

Choose a reason for hiding this comment

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

Looks good now from a docs perspective.

@dlangbe
Copy link
Collaborator Author

dlangbe commented Nov 27, 2024

@dlangbe Should the build configuration table be updated in docs/install/installation.rst too?

Good catch, it's updated now.

cgmillette
cgmillette reviewed Nov 27, 2024
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@amd-jnovotny
Copy link
Contributor

@dlangbe I just wanted to clarify: is this for 6.3.0 or 6.4.0? It currently looks to be in the 6.3.0 log, but I think Chris said it should be for 6.4.0. If it is going into 6.3.0, do I need to add this to the current Release notes?

@cgmillette
Copy link
Collaborator

@dlangbe I just wanted to clarify: is this for 6.3.0 or 6.4.0? It currently looks to be in the 6.3.0 log, but I think Chris said it should be for 6.4.0. If it is going into 6.3.0, do I need to add this to the current Release notes?

Yes, this needs to be labeled for ROCm 6.4

amd-jnovotny
amd-jnovotny previously approved these changes Dec 3, 2024
View reviewed changes
Copy link
Contributor

@amd-jnovotny amd-jnovotny left a comment

Choose a reason for hiding this comment

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

Just one edit I overlooked before to use the new headings (changed, added, etc.) for the changelog. Looks good otherwise now.

CHANGELOG.md Outdated Show resolved Hide resolved
cgmillette
cgmillette previously approved these changes Dec 3, 2024
View reviewed changes
amd-jnovotny
amd-jnovotny previously approved these changes Dec 3, 2024
View reviewed changes
Copy link
Contributor

@amd-jnovotny amd-jnovotny left a comment

Choose a reason for hiding this comment

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

looks good

@dlangbe dlangbe changed the base branch from develop to release-staging/rocm-rel-6.4 December 3, 2024 16:37
@dlangbe dlangbe dismissed stale reviews from amd-jnovotny and cgmillette December 3, 2024 16:37

The base branch was changed.

@dlangbe dlangbe changed the base branch from release-staging/rocm-rel-6.4 to develop December 3, 2024 16:47
CongMa13
CongMa13 previously approved these changes Dec 9, 2024
View reviewed changes
cgmillette
cgmillette reviewed Dec 9, 2024
View reviewed changes
CMakeLists.txt Outdated Show resolved Hide resolved
@dlangbe @cgmillette
Update CMakeLists.txt
df6ac95 
Co-authored-by: Christopher Millette <[email protected]>
cgmillette
cgmillette previously approved these changes Dec 9, 2024
View reviewed changes
amd-jnovotny
amd-jnovotny previously approved these changes Dec 9, 2024
View reviewed changes
@cgmillette
Update CMakeLists.txt
0176ca5 
Fixes cmake error
@cgmillette cgmillette dismissed stale reviews from amd-jnovotny and themself via 0176ca5 December 10, 2024 16:05
@cgmillette cgmillette merged commit 4ab2a82 into ROCm:develop Dec 10, 2024
2 of 5 checks passed
CongMa13 pushed a commit that referenced this pull request Dec 16, 2024
@dlangbe @cgmillette @CongMa13
Minor CMake Changes (#303)
4c328a6 
* changed gpu target name and builtin_chrpath

* Updated changelog

* Updated installation.rst

* updated comment

* updated logic

* Updated changelog

* Addressed final comments

* Update CMakeLists.txt

Co-authored-by: Christopher Millette <[email protected]>

* Update CMakeLists.txt

Fixes cmake error

---------

Co-authored-by: Christopher Millette <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cgmillette cgmillette cgmillette left review comments

@CongMa13 CongMa13 CongMa13 left review comments

@amd-jnovotny amd-jnovotny amd-jnovotny left review comments

@bragadeesh bragadeesh Awaiting requested review from bragadeesh bragadeesh is a code owner

@afanfa afanfa Awaiting requested review from afanfa afanfa is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dlangbe @amd-jnovotny @cgmillette @CongMa13