-
Notifications
You must be signed in to change notification settings - Fork 536
Exclude cpptrace and its deps from install to avoid copying lib/include #3199
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
base: development
Are you sure you want to change the base?
Conversation
| include(cmake/InstallRules.cmake) | ||
| endif() | ||
| # Disabled: cpptrace is statically linked and not needed at runtime | ||
| # if(NOT CMAKE_SKIP_INSTALL_RULES) |
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.
Can we just CMAKE_SKIP_INSTALL_RULES=TRUE inside <root>/dep/CMakeLists.txt:24, so we don't modify the cpptrace tree.
It's hard to keep track of those changes if we upgrade the version of cpptrace.
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.
That's what I've tried with the first commit, but it broke the install process.
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.
Then let us try this in dep/CMakeLists.txt
set(PREV_SKIP_INSTALL_RULES ${CMAKE_SKIP_INSTALL_RULES})
set(CMAKE_SKIP_INSTALL_RULES TRUE)
add_subdirectory(cpptrace)
set(CMAKE_SKIP_INSTALL_RULES ${PREV_SKIP_INSTALL_RULES})( And also please add a comment explaining why we are doing that :P )
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.
Setting CMAKE_SKIP_INSTALL_RULES = TRUE prevents CMake from generating cpptrace/cmake_install.cmake. But it seems the parent install script dep/cmake_install.cmake (auto-generated) always tries to include install scripts from all subdirectories
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.
So what are we settling on? Is the current solution the best approach?
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.
@schell244 can you please document your current change in dep/cpptrace/README.md under "## Manual Changes"
|
Hmm, this doesn't seem to be working as intended for me; but maybe I'm misunderstanding something. I am currently already manually removing In preparation for this PR, I added some logging to confirm my manual cleanup would no longer be necessary when it gets merged. But when I build a Docker image from your PR branch, I am using a custom |
|
Oh ok, thanks for pointing that out. The |
5e533af to
c972274
Compare
c972274 to
44d2a9e
Compare
Ah, sorry, that would make sense. I somehow didn't consider checking what is actually inside these directories, otherwise I would've probably realized myself that what's left in there is not targeted by your original changes here. 😅 Have a nice vacation! |
🍰 Pullrequest
Skip install rules for cpptrace and its dependencies to avoid copying unnecessary
libandincludefolders.FetchContent_MakeAvailable so its install rules are not executed
Proof
Issues
How2Test
Todo / Checklist