GH-34148: [C++][Thirdparty] make zstd require CMake 3.18

[mapleFU]

Rationale for this change

See #34148 . Zstd v1.5.4 require CMake 3.18

What changes are included in this PR?

CMake version check changed

Are these changes tested?

No

Are there any user-facing changes?

Yes, lower version cmake user cannot use zstd now

• Closes: [C++] Minimal build failed in crossbow because of zstd v1.5.4 requires CMake v3.18 #34148

[github-actions]

• Closes: [C++] Minimal build failed in crossbow because of zstd v1.5.4 requires CMake v3.18 #34148

[github-actions]

⚠️ GitHub issue #34148 has been automatically assigned in GitHub to PR creator.

[kou]

@github-actions crossbow submit example-cpp-minimal-build-static

[github-actions]

Revision: 13283be

Submitted crossbow builds: ursacomputing/crossbow @ actions-b63a17ccf5

Task	Status
example-cpp-minimal-build-static

[mapleFU]

CMake Error at cmake_modules/ThirdpartyToolchain.cmake:2509 (message):
  Building Zstandard using ExternalProject requires at least CMake 3.18
Call Stack (most recent call first):
  cmake_modules/ThirdpartyToolchain.cmake:205 (build_zstd)
  cmake_modules/ThirdpartyToolchain.cmake:278 (build_dependency)
  cmake_modules/ThirdpartyToolchain.cmake:2558 (resolve_dependency)
  CMakeLists.txt:504 (include)

lol...

[assignUser]

I am -1 on this. Can we not rather than stopping the build in case the cmake version is to low drop to the previous working zstd version (1.52.x or what it was)

[kou]

@assignUser OK. How about sending a patch like the following to upstream then?

diff --git a/build/cmake/CMakeModules/AddZstdCompilationFlags.cmake b/build/cmake/CMakeModules/AddZstdCompilationFlags.cmake
index 0265349f..c4be65ce 100644
--- a/build/cmake/CMakeModules/AddZstdCompilationFlags.cmake
+++ b/build/cmake/CMakeModules/AddZstdCompilationFlags.cmake
@@ -1,6 +1,15 @@
 include(CheckCXXCompilerFlag)
 include(CheckCCompilerFlag)
-include(CheckLinkerFlag)
+# VERSION_GREATER_EQUAL requires CMake 3.7 or later.
+# https://cmake.org/cmake/help/latest/command/if.html#version-greater-equal
+if (CMAKE_VERSION VERSION_LESS 3.18)
+    set(ZSTD_HAVE_CHECK_LINKER_FLAG false)
+else ()
+    set(ZSTD_HAVE_CHECK_LINKER_FLAG true)
+endif ()
+if (ZSTD_HAVE_CHECK_LINKER_FLAG)
+    include(CheckLinkerFlag)
+endif()
 
 function(EnableCompilerFlag _flag _C _CXX _LD)
     string(REGEX REPLACE "\\+" "PLUS" varname "${_flag}")
@@ -20,7 +29,11 @@ function(EnableCompilerFlag _flag _C _CXX _LD)
         endif ()
     endif ()
     if (_LD)
-        CHECK_LINKER_FLAG(C ${_flag} LD_FLAG_${varname})
+        if (ZSTD_HAVE_CHECK_LINKER_FLAG)
+            CHECK_LINKER_FLAG(C ${_flag} LD_FLAG_${varname})
+        else ()
+            set(LD_FLAG_${varname} false)
+        endif ()
         if (LD_FLAG_${varname})
             set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} ${_flag}" PARENT_SCOPE)
             set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} ${_flag}" PARENT_SCOPE)

[mapleFU]

All is ok for me, personally I use CMake 3.22, so this doesn't bother me...
Can we include some interface in CMake 3.16 rather than include in upstream?

[assignUser]

I will have time to add a patch at the end of the week :)

[kou]

Thanks!
But I opened a pull request facebook/zstd#3510 because some discussions were proceeded in facebook/zstd#3500 .

[assignUser]

That is good for the next release but we should fix this in arrow now. How about we just roll back to the previous version until your fix is released?

[assignUser]

Anyone that wants to use the 1.5.4 can use zstd_URL to override it.

[kou]

OK.
@mapleFU Could you revert #34114 ? We will bump zstd to1.5.5 that includes a fix for facebook/zstd#3500 later.

[mapleFU]

OK. @mapleFU Could you revert #34114 ? We will bump zstd to1.5.5 that includes a fix for facebook/zstd#3500 later.

Glad to do it. @kou but would it spend a long time? Seems that release time between 1.5.2 and 1.5.4 are several years

[kou]

Let's ask zstd maintainers when the next release be after facebook/zstd#3500 is fixed.

If the next release is unknown, we may implement conditional bundled zstd build. We use zstd 1.5.4 with newer CMake and use zstd 1.5.2 with older CMake.

[mapleFU]

I've revert it yet #34190 (comment) . And if you want me to pickup zstd in newer cmake, you can just tell me or edit it directly on my patch

[mapleFU]

Currently convert it to draft, maybe close it or change it introduce later zstd when upstream is ok

[kou]

OK. I close this.