-
Notifications
You must be signed in to change notification settings - Fork 15.5k
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
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