-
Notifications
You must be signed in to change notification settings - Fork 160
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
A bunch of CMake fixes #1178
Open
graebm
wants to merge
5
commits into
main
Choose a base branch
from
cmake-modules
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
A bunch of CMake fixes #1178
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…mpatible with latest versions too
This eliminates a CMake Deprecation Warning. The comment says we added this to "Enable options to get their values from normal variables" But reading the docs: https://cmake.org/cmake/help/latest/policy/CMP0077.html it seems like the NEW behavior is what lets options get their values from normal variables
…es like LIBRARY_DIRECTORY. Since every platform can do `include(GnuInstallDirs)`, it's fine to rely on the variables it defines, so let's just use them explicitly. - Explicitly include(GNUInstallDirs) in every CMakeLists.txt that uses the vars it defines (e.g. CMAKE_INSTALL_LIBDIR). I could get away omitting this include, since the AwsSharedLibsSetup.cmake helper script also pulls it in, but it seems like good practice. - Remove custom code that was setting FIND_LIBRARY_USE_LIB64_PATHS. In the PR that introduced this #226 the reviewer called out that this probably wasn't necessary. ChatGPT agrees. CMake is supposed to figure this out automatically. And if it's not working, then it's a weird cross-compile situation where the person building should be setting this to fix things. It shouldn't be up to every library on earth to do this hack. So, this PR started with me thinking that, if we're getting all the shared scripts via `find_package(aws-c-common)`, and the shared scripts are where FIND_LIBRARY_USE_LIB64_PATHS gets customized, but we need to customize FIND_LIBRARY_USE_LIB64_PATHS before we can do `find_package(aws-c-common)`, then ALL projects need to copy/paste the code that customizes FIND_LIBRARY_USE_LIB64_PATHS before doing `find_package(aws-c-common)`. So I set down that path. Then when writing the commit message I was like ... wait a minute this is ridiculous. So did some more research, and learned the FIND_LIBRARY_USE_LIB64_PATHS stuff was probably unnecessary.
graebm
changed the title
Install CMake modules in a more standard way.
A bunch of CMake fixes
Dec 23, 2024
waahm7
approved these changes
Dec 24, 2024
sfod
approved these changes
Dec 24, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #:
Description of changes:
cmake_minimum_required(VERSION 3.9)
cmake_minimum_required(VERSION 3.9...3.31)
CMAKE_INSTALL_LIBDIR
,CMAKE_INSTALL_BINDIR
,CMAKE_INSTALL_INCLUDEDIR
LIBRARY_DIRECTORY,RUNTIME_DIRECTORY)include(GNUInstallDirs)
on all platformsGNUInstallDirs
wasn't safe for Windows and Apple, but despite its name, it's fine. Professional CMake says:aws-c-common-config.cmake
lib/aws-c-common/cmake/...
lib/cmake/aws-c-common/...
AwsCFlags.cmake
lib/cmake/...
lib/cmake/aws-c-common/modules/...
aws-c-common-config.cmake
so that, when any dependency callsfind_package(aws-c-common)
, theCMAKE_MODULE_PATH
is modified to include the installed location of modules likeAwsCFlags.cmake
.FIND_LIBRARY_USE_LIB64_PATHS
, just let CMake handle itREASONING: Here's what Professional CMake: A Practical Guide 12th Edition has to say on this topic:
This wasn't causing issues, but I'm trying to cut down on the hacky boilerplate copy/pasted into dozens of other CMake files amongst our repos. In the PR that introduced this, it's commented with "this is the absolute dumbest thing in the world" and a reviewer mentions that it's not necessary in aws-sdk-cpp. My best guess as to why it seemed necessary is that we were trying to get cross-compile working somewhere and this fixed it. But it seems like the real fix is to address this in the cross-compilation toolchain file. It shouldn't be up to every CMake library on earth to include this hack. Removing now in 2024 doesn't seem to cause any issues, so 🪓
Thanks to @r-burns @madebr @glaubitz and others for helpful comments, and the Pull Requests it took us years to finally look at
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.