-
Notifications
You must be signed in to change notification settings - Fork 55
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
Bugfix: CMake Install paths #1516
base: main
Are you sure you want to change the base?
Conversation
…installation dirs for bin, lib etc..
Thanks @Davknapp ! That fixes the issue of incorrect linking and two lib-directories I had encountered. |
file(RELATIVE_PATH relativeRpath | ||
${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_INSTALL_BINDIR} | ||
${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_INSTALL_LIBDIR} | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your solution may be completely right, but better safe than sorry:
According to CMake documentation, the RELATIVE_PATH
argument of file
works like this:
It seems to odd to me that we use two directories here instead; and wouldn't this mean relativePath
would contain the relative path from the bin
to the lib
folder, so basically .../lib
? 🤔
Or is this supposed to be some correction of the current state only?
@@ -36,7 +36,7 @@ function( add_t8_example ) | |||
set_target_properties( ${ADD_T8_EXAMPLE_NAME} PROPERTIES EXPORT_COMPILE_COMMANDS ON ) | |||
endif( T8CODE_EXPORT_COMPILE_COMMANDS ) | |||
|
|||
install( TARGETS ${ADD_T8_EXAMPLE_NAME} DESTINATION bin ) | |||
install( TARGETS ${ADD_T8_EXAMPLE_NAME} DESTINATION ${CMAKE_INSTALL_BINDIR} ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be install( TARGETS ${ADD_T8_EXAMPLE_NAME} RUNTIME )
, like in the tutorials below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested it on the Teamsserver and it worked fine - however, it also did so before, so maybe that's not a great test. As you can see, I do have some question about the relative path 🤔
Also, the PR description says it unifies the installation dirs with p4est and sc, but p4est does not seem to use |
Instead of hard-coding paths use GNUInstallDirs to automatically set installation dirs for bin, lib etc..
We hard coded some installation paths, while p4est and sc use GNUInstallDirs to automatically set these paths. This PR unifies the behaviour
All these boxes must be checked by the AUTHOR before requesting review:
Documentation:
,Bugfix:
,Feature:
,Improvement:
orOther:
.All these boxes must be checked by the REVIEWERS before merging the pull request:
As a reviewer please read through all the code lines and make sure that the code is fully understood, bug free, well-documented and well-structured.
General
Tests
If the Pull request introduces code that is not covered by the github action (for example coupling with a new library):
Scripts and Wiki
script/find_all_source_files.scp
to check the indentation of these files.License
doc/
(or already has one).