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

Dependency target install paths and expanding INSTALL_RPATH #133

Merged
merged 11 commits into from
Nov 17, 2023

Conversation

zachcran
Copy link
Contributor

@zachcran zachcran commented Nov 3, 2023

Is this pull request associated with an issue(s)?
I'm not sure about a specific issue, but there are RPATH issues with CMaize project installations right now and a fix was attempted in #110 but ultimately it was closed without a definitive solution.

Description
This PR lays the groundwork to append the installation paths of dependency targets to the rpath of the main target(s) that the CMaize project is defining. It adds an install_path attribute to the CMaizeTarget class and a build_path attribute to the BuildTarget class (not actually used for this, but useful down the road for helping Python tests find things). When cmaize_add_package() is called at the end of the CMakeLists.txt file, the install_paths for all of a target's dependencies will be appended to its INSTALL_RPATH property, which translates into the rpath added to the resulting library or executable.

Currently, the install_path attributes are not being filled, so it essentially results in a no-op that maintains the status quo of CMaize. However, if the install_path attributes can be populated correctly, the install locations of all dependency targets should be added to the project's main targets rpaths, fixing our long-standing rpath issues. Using the CMaizePublicDepend test repo, I set the install path of the dependency, CMakePublic, to /example/install/path/to/CMakePublic/lib manually and the resulting INSTALL_RPATH value for CMaizePublicDepend included this new install path, showing that this implementation seems to be working:

-- INSTALL_RPATH: 
 ORIGIN;$ORIGIN/external/tmp;$ORIGIN;$ORIGIN/external/tmp;/example/install/path/to/CMakePublic/lib

Notes

Using the (potentially non-exhaustive) list of target properties from https://www.kitware.com//cmake-target-properties/, I added a loop at the end of the CMakeLists.txt file in our CMaizePublicDepend test project, printing the values of all of the properties for the CMaizePublicDepend target and CMakePublic imported target. No property contained installation information, even though install() commands were guaranteed to have been applied to the targets by the end of the file (afaik). The following are the property values that were extracted:

CMakePublic

-- BUILD_WITH_INSTALL_RPATH: FALSE
-- IMPORTED: FALSE
-- INSTALL_RPATH: $ORIGIN;$ORIGIN/external/tmp;$ORIGIN;$ORIGIN/external/tmp
-- INSTALL_RPATH_USE_LINK_PATH: TRUE
-- SKIP_BUILD_RPATH: FALSE
-- SOURCES: /path/to/cmake_public/src/cmake_public/world.cpp

CMaizePublicDepend

-- BUILD_WITH_INSTALL_RPATH: FALSE
-- IMPORTED: FALSE
-- INSTALL_RPATH: $ORIGIN;$ORIGIN/external/tmp;$ORIGIN;$ORIGIN/external/tmp
-- INSTALL_RPATH_USE_LINK_PATH: TRUE
-- SKIP_BUILD_RPATH: FALSE
-- SOURCES: /path/to/cmaize_public_depend/src/cmaize_public_depend/cmaize_public_depend.cpp
-- SOVERSION: 1
-- VERSION: 1.0.0

On macOS Ventura, the same example installation path was added to CMakePublic and was successfully added to the libCMaizePublicDepend.1.0.0.dylib rpath. Output from otool -l libCMaizePublicDepend.1.0.0.dylib shows it added to the rpath:

Load command 16
          cmd LC_RPATH
      cmdsize 24
         path $ORIGIN (offset 12)
Load command 17
          cmd LC_RPATH
      cmdsize 40
         path $ORIGIN/external/tmp (offset 12)
Load command 18
          cmd LC_RPATH
      cmdsize 56
         path /example/install/path/to/CMakePublic/lib (offset 12)

TODOs

  • Figure out a way to populate the install_path property (probably push to an issue and later PR).
  • Test this on NWChemEx.
  • Test this on a Mac.
  • Ensure that all unit tests still work.
  • Ensure that all integration tests still work.

@zachcran zachcran self-assigned this Nov 3, 2023
@zachcran zachcran added the bump:minor Increments the minor version. label Nov 3, 2023
@ryanmrichard ryanmrichard marked this pull request as draft November 6, 2023 16:42
@ryanmrichard
Copy link
Collaborator

I made this a draft since it seems like it's still a work in progress.

@zachcran zachcran marked this pull request as ready for review November 17, 2023 20:03
@zachcran
Copy link
Contributor Author

@ryanmrichard I checked that this builds, the tests work, and the Python bindings test script works on my local system as well.

@ryanmrichard ryanmrichard merged commit 3cef972 into master Nov 17, 2023
6 checks passed
@ryanmrichard ryanmrichard deleted the deps_install_path branch November 17, 2023 20:59
Copy link

🚀 [bumpr] Bumped! New version:v0.2.0 Changes:v0.1.11...v0.2.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump:minor Increments the minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants