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.

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

CMakeLists.txt: remove rpath handling #17

Merged
merged 1 commit into from
Dec 4, 2024

Conversation

haampie
Copy link
Contributor

@haampie haampie commented Dec 4, 2024

Given the various issues...

  1. Bug fix for package [email protected]: set install rpath for cprnc executable spack/spack#47913
  2. Bug fix for package cprnc: set install rpath for cprnc executable spack/spack#47505

And since

  • CMAKE_MACOSX_RPATH=ON is the default.
  • CMAKE_INSTALL_RPATH_USE_LINK_PATH is used, making manually specified install rpaths redundant
  • typically users want to set CMAKE_INSTALL_RPATH_USE_LINK_PATH externally, especially when redistributing binaries, to avoid registering rpaths that only exist on a build node

it's better to remove this rpath logic and let users set it from the command line if they really need it (which spack already does).

@haampie
Copy link
Contributor Author

haampie commented Dec 4, 2024

Hm, looks like CMAKE_INSTALL_RPATH_USE_LINK_PATH=ON is not working because target_link_libraries is used incorrectly.

It should contain paths to libraries instead of linker flags -L<dir> -l<lib>.

I would suggest to use pkg_search_module instead of this custom nc-config, since CMake has the logic to locate the libraries from linker flags from pkg-config, so you can properly use them in target_link_libraries.

@jedwards4b jedwards4b merged commit fec7c42 into ESMCI:main Dec 4, 2024
1 check passed
@haampie haampie deleted the remove-rpath-handling branch December 4, 2024 13:15
@haampie
Copy link
Contributor Author

haampie commented Dec 4, 2024

@jedwards4b I think this was merged too early given #17 (comment)

CMAKE_INSTALL_RPATH_USE_LINK_PATH does not work when target_link_libraries is used incorrectly like in this package

@jedwards4b
Copy link
Collaborator

@haampie Please review #19
Thanks,

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.

2 participants