Skip to content

Add depenency on rocm-core package#36

Open
icarus-sparry wants to merge 1 commit intoROCm:masterfrom
icarus-sparry:master
Open

Add depenency on rocm-core package#36
icarus-sparry wants to merge 1 commit intoROCm:masterfrom
icarus-sparry:master

Conversation

@icarus-sparry
Copy link

The new rocm-core package is designed to help remove all of rocm by
being a dependancy of all rocm related packages.

To aid in the transition this is only enabled if -DROCM_DEP_ROCMCORE
is passed to cmake.

Signed-off-by: Icarus Sparry icarus.sparry@amd.com

The new rocm-core package is designed to help remove all of rocm by
being a dependancy of all rocm related packages.

To aid in the transition this is only enabled if -DROCM_DEP_ROCMCORE
is passed to cmake.

Signed-off-by: Icarus Sparry <icarus.sparry@amd.com>
@icarus-sparry icarus-sparry requested review from frepaul and pfultz2 July 20, 2021 20:09
set(CPACK_RPM_PACKAGE_REQUIRES "rocm-llvm, rocm-opencl-devel")
set(CPACK_DEBIAN_PACKAGE_DEPENDS "rocm-llvm, rocm-opencl-dev, rocm-core")
set(CPACK_RPM_PACKAGE_REQUIRES "rocm-llvm, rocm-opencl-devel, rocm-core")
# Remove dependency on rocm-core if -DROCM_DEP_ROCMCORE=ON not given to cmake
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need a flag to disable this?

Copy link
Author

Choose a reason for hiding this comment

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

Because "requirements"! The argument is that rocm-core has not yet been open sourced, so people who don't have access to the internal source trees will be stopped from building. This is also the reason why the dependency defaults to OFF, so there is no change to their existing workflows.

set(CPACK_DEBIAN_PACKAGE_DEPENDS "rocm-llvm, rocm-opencl-dev, rocm-core")
set(CPACK_RPM_PACKAGE_REQUIRES "rocm-llvm, rocm-opencl-devel, rocm-core")
# Remove dependency on rocm-core if -DROCM_DEP_ROCMCORE=ON not given to cmake
if(NOT ROCM_DEP_ROCMCORE)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to do if(ROCM_DEP_ROCMCORE) and then do string(APPEND) to add rocm-core rather then trying to remove it with regexes.

Copy link
Author

Choose a reason for hiding this comment

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

As we discussed briefly in IM, there are special cases, in particular if CPACK_DEBIAN_PACKAGE_DEPENDS is empty then you want to set it to rocm-core, whilst if it is not you want to append a command and then rocm-core. Using regular expressions allows us to do this concisely.

If we knew we were using cmake >= 3.12 we could use
string(JOIN ", " .CPACK_DEBIAN_PACKAGE_DEPENDS "rocm-core" $CPACK_DEBIAN_PACKAGE_DEPENDS)
to prepend the value correctly. It doesn't seem worthwhile to write our own function to do this, with alternative implementations depending on the cmake version when we have an existing one line solution.

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.

3 participants