Don't require CMake 3.18 or later#3510
Conversation
|
Hi @kou! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
kou
force-pushed
the
cmake-not-require-3.18
branch
from
327e533 to
e6d5b5d
Compare
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
1 similar comment
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Instead of just disable this flag it can be done by |
|
The shim implementation will increase maintenance cost a bit. The author of #3392 uses If the shim implementation is needed for this project, I can do it. |
|
I think it is reasonable to simply disable the extra ldflags when I think we should just add a comment to the function to clarify that the ldflags don't get added for old cmake versions. |
| endif () | ||
| if (_LD) | ||
| CHECK_LINKER_FLAG(C ${_flag} LD_FLAG_${varname}) | ||
| if (ZSTD_HAVE_CHECK_LINKER_FLAG) |
There was a problem hiding this comment.
Can you just add a comment to this function that says that it doesn't add ldflags for older cmake versions? In case that we need to add other ldflags that are non-optional later.
There was a problem hiding this comment.
It makes sense. I've add a comment:
# We never add a linker flag with CMake < 3.18. We will
# implement CHECK_LINKER_FLAG() like feature for CMake < 3.18
# or require CMake >= 3.18 when we need to add a required
# linker flag in future.Does this make sense?
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
force-pushed
the
cmake-not-require-3.18
branch
from
e6d5b5d to
71094bb
Compare
|
@terrelln Thanks! Do you have a plan to release a new version with this change soon? (If it's done, apache/arrow can use the latest Zstandard. But it's not a blocker. So apache/arrow can wait a new version.) |
4 participants
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=cmakeThis change disables
-z noexecstackcheck with old CMake. This will not break any existing users. Because users who need-z noexecstackmust already use CMake 3.18 or later.