[GitHub] [arrow-nanoarrow] assignUser commented on a diff in pull request #169: chore: Add IPC extension and ARM64 to verification script

["assignUser (via GitHub)" <gi...@apache.org>]

assignUser commented on code in PR #169:
URL: https://github.com/apache/arrow-nanoarrow/pull/169#discussion_r1152607329


##########
extensions/nanoarrow_ipc/CMakeLists.txt:
##########
@@ -16,9 +16,13 @@
 # under the License.
 
 message(STATUS "Building using CMake version: ${CMAKE_VERSION}")
-cmake_minimum_required(VERSION 3.11)
+cmake_minimum_required(VERSION 3.14)
 include(FetchContent)
 
+if(NOT DEFINED CMAKE_C_STANDARD)
+  set(CMAKE_C_STANDARD 11)

Review Comment:
   Fun fact, this actually does not enforce the usage of C++11 but allows a compiler to decay to an earlier version.
   See https://cmake.org/cmake/help/latest/prop_tgt/CXX_STANDARD_REQUIRED.html



##########
extensions/nanoarrow_ipc/CMakeLists.txt:
##########
@@ -181,11 +185,24 @@ if (NANOARROW_IPC_BUILD_TESTS)
   message(STATUS "Arrow SO version: ${ARROW_FULL_SO_VERSION}")
 
   # Warning about timestamps of downloaded files
-  cmake_policy(SET CMP0135 NEW)
-  FetchContent_Declare(
-    googletest
-    URL https://github.com/google/googletest/archive/release-1.11.0.zip
-  )
+  if (${CMAKE_VERSION} VERSION_GREATER_EQUAL "3.23")
+      cmake_policy(SET CMP0135 NEW)
+  endif()
+
+  # Use an old version of googletest if we have to to support gcc 4.8
+  if(NOT CMAKE_CXX_COMPILER_ID STREQUAL "GNU" OR
+    CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL "5.0.0")
+    FetchContent_Declare(
+        googletest
+        URL https://github.com/google/googletest/archive/release-1.11.0.zip

Review Comment:
   also checksums maybe?



##########
extensions/nanoarrow_ipc/CMakeLists.txt:
##########
@@ -199,8 +216,20 @@ if (NANOARROW_IPC_BUILD_TESTS)
     target_link_libraries(nanoarrow_ipc PRIVATE ipc_coverage_config)
   endif()
 
-  target_link_libraries(nanoarrow_ipc_decoder_test nanoarrow_ipc nanoarrow arrow_shared gtest_main)
-  target_link_libraries(nanoarrow_ipc_reader_test nanoarrow_ipc nanoarrow gtest_main)
+  # On Windows, dynamically linking Arrow is difficult to get right,
+  # so use static linking by default.
+  if (MSVC)
+      set(NANOARROW_IPC_ARROW_TARGET arrow_static)
+  else()
+      set(NANOARROW_IPC_ARROW_TARGET arrow_shared)
+  endif()

Review Comment:
   This could also be done with a generator expression something like (untested) `set(NANOARROW_IPC_ARROW_TARGET $<$<${MSVC}>:arrow_static>$<$<NOT:$<${MSVC}>>:arrow_shared>)` but I am not really convinced that it is more readable :D



##########
extensions/nanoarrow_ipc/CMakeLists.txt:
##########
@@ -181,11 +185,24 @@ if (NANOARROW_IPC_BUILD_TESTS)
   message(STATUS "Arrow SO version: ${ARROW_FULL_SO_VERSION}")
 
   # Warning about timestamps of downloaded files
-  cmake_policy(SET CMP0135 NEW)
-  FetchContent_Declare(
-    googletest
-    URL https://github.com/google/googletest/archive/release-1.11.0.zip
-  )
+  if (${CMAKE_VERSION} VERSION_GREATER_EQUAL "3.23")
+      cmake_policy(SET CMP0135 NEW)
+  endif()
+
+  # Use an old version of googletest if we have to to support gcc 4.8
+  if(NOT CMAKE_CXX_COMPILER_ID STREQUAL "GNU" OR
+    CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL "5.0.0")
+    FetchContent_Declare(
+        googletest
+        URL https://github.com/google/googletest/archive/release-1.11.0.zip

Review Comment:
   I'd extract the version into a var and set that in the if to avoid duplicating the whole FC_declare call.



##########
.github/workflows/verify.yaml:
##########
@@ -166,5 +173,5 @@ jobs:
 
         run: |
           cd src
-          # docker compose pull verify
-          docker compose run verify
+          # Set GITHUB_WORKSPACE to set more log-friendly headers

Review Comment:
   I am not sure what this envvar is supposed to do/how. If you want to use a var to detect the execution within GHA I would recommend using `GITHUB_ACTIONS` as this is a var that is set by GHA for this purpose vs `GITHUB_WORKSPACE` which usually contains the [workspace path](https://docs.github.com/en/actions/learn-github-actions/variables),



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org