Skip to content
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: bump min CMake version to 3.18 or remove usage of check_linker_flag() #3500

Closed
SpaceIm opened this issue Feb 11, 2023 · 5 comments · Fixed by #3510
Closed

CMake: bump min CMake version to 3.18 or remove usage of check_linker_flag() #3500

SpaceIm opened this issue Feb 11, 2023 · 5 comments · Fixed by #3510
Labels

Comments

@SpaceIm
Copy link

SpaceIm commented Feb 11, 2023

#3392 (part of zstd 1.5.4) checks linker flags in CMake build by relying on https://cmake.org/cmake/help/latest/module/CheckLinkerFlag.html, a function available since CMake 3.18. But cmake_minimum_required() of main CMakeLists is still 2.8.12.
Is it intentional to use such recent feature? If it is, cmake_minimum_required() should be updated accordingly. Otherwise it's possible to achieve the same checks by using check_source_compiles() and therefore avoid constraining CMakeLists of zstd to such recent CMake version.

@Cyan4973
Copy link
Contributor

We could update the minimum version of cmake if there is a good reason for it, but 3.18 is way too recent to become the new minimum (I think we could consider a new minimum as high as 3.5, but no more than that to ensure a good reach).

When a feature requires a version which is higher than the minimum, it should be guarded by version checks, and offer a fallback strategy.

#3392 was added by @terrelln , so we'll need his input.

@botanegg
Copy link

To anyone who is stuck with CI pipelines - you can upgrade your cmake from kitware apt - https://apt.kitware.com/
Look at kitware-archive.sh
This solved the problem for me

kou added a commit to kou/zstd that referenced this issue Feb 14, 2023
fix facebook#3500

CMake 3.18 or later was required by facebook#3392. Because it uses
`CheckLinkerFlag`. But requiring CMake 3.18 or later is a bit
aggressive. Because Ubuntu 20.04 LTS still uses CMake 3.16.3:
https://packages.ubuntu.com/search?keywords=cmake

This change disables `-z noexecstack` check with old CMake. This will
not break any existing users. Because users who need `-z nonexecstack`
must already use CMake 3.18 or later.
kou added a commit to kou/zstd that referenced this issue Feb 14, 2023
fix facebook#3500

CMake 3.18 or later was required by facebook#3392. Because it uses
`CheckLinkerFlag`. But requiring CMake 3.18 or later is a bit
aggressive. Because Ubuntu 20.04 LTS still uses CMake 3.16.3:
https://packages.ubuntu.com/search?keywords=cmake

This change disables `-z noexecstack` check with old CMake. This will
not break any existing users. Because users who need `-z noexecstack`
must already use CMake 3.18 or later.
@kou
Copy link
Contributor

kou commented Feb 14, 2023

How about just disabling the -z noexecstack with old CMake?
I want to use old CMake for apache/arrow.
See also: apache/arrow#34148

@terrelln
Copy link
Contributor

How about just disabling the -z noexecstack with old CMake?

It should be safe to default to not adding the linker flag if the cmake version is too old. This PR was mostly aiming at fixing our tests, and it should be safe to skip this flag when cmake doesn't support check_linker_flag(). I'll put up a PR to add that.

@kou
Copy link
Contributor

kou commented Feb 15, 2023

@terrelln Thanks for your comment! #3510 does it. Could you review the pull request?

kou added a commit to kou/zstd that referenced this issue Feb 15, 2023
fix facebook#3500

CMake 3.18 or later was required by facebook#3392. Because it uses
`CheckLinkerFlag`. But requiring CMake 3.18 or later is a bit
aggressive. Because Ubuntu 20.04 LTS still uses CMake 3.16.3:
https://packages.ubuntu.com/search?keywords=cmake

This change disables `-z noexecstack` check with old CMake. This will
not break any existing users. Because users who need `-z noexecstack`
must already use CMake 3.18 or later.
terrelln pushed a commit that referenced this issue Feb 16, 2023
fix #3500

CMake 3.18 or later was required by #3392. Because it uses
`CheckLinkerFlag`. But requiring CMake 3.18 or later is a bit
aggressive. Because Ubuntu 20.04 LTS still uses CMake 3.16.3:
https://packages.ubuntu.com/search?keywords=cmake

This change disables `-z noexecstack` check with old CMake. This will
not break any existing users. Because users who need `-z noexecstack`
must already use CMake 3.18 or later.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants