Skip to content

SWDEV-516659 - Ensure the list of recommended and suggested packages are comma separated - #368

Merged
cgmb merged 1 commit into
ROCm:developfrom
raramakr:swdev-516659
Feb 26, 2025
Merged

cgmb merged 1 commit into
ROCm:developfrom
raramakr:swdev-516659

Conversation

@raramakr

Copy link
Copy Markdown
Contributor

A list in cmake is a semicolon(;) separated group of strings. The cpack recommends and suggests variables should have comma separated list, otherwise it will result in package installation failure.

…are comma separated

A list in cmake is a semicolon(;) separated group of strings. The cpack recommends and suggests variables should have comma separated list, otherwise it will result in package installation failure.
@pramenku

Copy link
Copy Markdown

We need to CP to 6.4 also

@raramakr
raramakr requested a review from nunnikri February 24, 2025 19:33
@qjojo

qjojo commented Feb 24, 2025

Copy link
Copy Markdown
Collaborator

I may be stepping on a land mine by asking this, but CMake 3.12 added a more idiomatic way to do this https://cmake.org/cmake/help/latest/command/list.html#join

list(JOIN CPACK_DEBIAN_RUNTIME_PACKAGE_RECOMMENDS "," CPACK_DEBIAN_RUNTIME_PACKAGE_RECOMMENDS)

We would have to bump the minimum version to use it, but I can see that rocBLAS targets 3.16, and I believe the cmake version shipped with the OSes we support is much higher than that.

@raramakr

Copy link
Copy Markdown
Contributor Author

I may be stepping on a land mine by asking this, but CMake 3.12 added a more idiomatic way to do this https://cmake.org/cmake/help/latest/command/list.html#join

list(JOIN CPACK_DEBIAN_RUNTIME_PACKAGE_RECOMMENDS "," CPACK_DEBIAN_RUNTIME_PACKAGE_RECOMMENDS)

We would have to bump the minimum version to use it, but I can see that rocBLAS targets 3.16, and I believe the cmake version shipped with the OSes we support is much higher than that.

Since the current cmake version is set to 3.7 didn't use cmake list(JOIN ..) in this patch. Also this need to be cherry-picked to 6.4 release branch.
Maybe we can bump the cmake version and use cmake list(join ... ) in another patch

@cgmb cgmb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

IMO, you might as well just directly set the *_RECOMMENDS and *_SUGGESTS with the string command, rather than using list and then string. With that said, I can live with it as-is if you just want to merge something that you've tested ASAP. It's not like the rest of our CMake is beautiful anyway.

#364 is related and is also desired for a cherry-pick, once reviewed and approved.

@cgmb

cgmb commented Feb 25, 2025

Copy link
Copy Markdown
Collaborator

Maybe we can bump the cmake version and use cmake list(join ... ) in another patch

Sure. Bumping the minimum requirement for hipSOLVER to CMake 3.12 is fine. The rocSOLVER library requires 3.13, and rocBLAS requires 3.16.

@pramenku

Copy link
Copy Markdown

Can we proceed to merge this PR and put in "staging" branch for further CQE cycle?

@cgmb
cgmb merged commit 434fd22 into ROCm:develop Feb 26, 2025
ammallya pushed a commit that referenced this pull request Oct 28, 2025
…ated (#368)

A list in cmake is a semicolon(;) separated group of strings. The cpack recommends and suggests variables should have comma separated list, otherwise it will result in package installation failure.

[ROCm/hipSOLVER commit: 434fd22]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants