-
Notifications
You must be signed in to change notification settings - Fork 16k
cmake: Use linker version scripts #9545
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
cmake: Use linker version scripts #9545
Conversation
cmake/CMakeLists.txt
Outdated
| @@ -1,5 +1,5 @@ | |||
| # Minimum CMake required | |||
| cmake_minimum_required(VERSION 3.1.3) | |||
| cmake_minimum_required(VERSION 3.18) | |||
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.
In case anybody is curious, CheckLinkerFlag cmake module (used below) is documented in https://cmake.org/cmake/help/latest/module/CheckLinkerFlag.html that:
New in version 3.18.
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.
One of our CI builds is breaking because it's still using CMake 3.10.1. Is there any easy workaround that wouldn't require the CheckLinkerFlag module?
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 just checked and it doesn't appear to be easy to upgrade our CMake version, because we're using a Windows VM maintained by another team. I think we should probably just enable the behavior conditionally, only if CMAKE_VERSION is greater than or equal to 3.18 (we already do something like this 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.
Actually would you mind setting the minimum version to 3.10.1? Let's do that and see if anyone complains. Maybe we can try to gradually increase the minimum version over time so that we can start to rely on newer CMake features.
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 added fallback code using check_cxx_source_compiles function from CheckCXXSourceCompiles module.
Autotools build system already uses linker version scripts since commit 13d165d. Fixes: protocolbuffers#6113
b3fd1ca to
918b022
Compare
|
Thanks, @Arfrever! |
|
Is it really required to bump the cmake version to 3.10? FYI, gRPC still supports 3.5.1. |
|
Let me send out a pull request to lower it to 3.5.1. |
Autotools build system already uses linker version scripts since
commit 13d165d.
Fixes: #6113