ORC-1732: [C++] Fix detecting Homebrew-installed Protobuf on MacOS

### What changes were proposed in this pull request? Detect Protobuf installed by Homebrew on macOS. ### Why are the changes needed? Deal with the issue discussed [here](apache/arrow#42149) ### How was this patch tested? Test it locally. ### Was this patch authored or co-authored using generative AI tooling? NO Closes #1963 from luffy-zh/ORC-1732. Lead-authored-by: luffy-zh <zhnice@outlook.com> Co-authored-by: Hao Zou <34559830+luffy-zh@users.noreply.github.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit a9e0351) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
apache · Jul 11, 2024 · 810f466 · 810f466
1 parent 6d478cb
commit 810f466
Show file tree

Hide file tree

Showing 3 changed files with 67 additions and 4 deletions.
diff --git a/.github/workflows/build_and_test.yml b/.github/workflows/build_and_test.yml
@@ -194,3 +194,25 @@ jobs:
         with:
           config: .github/.licenserc.yaml
 
+  macos-cpp-check:
+    name: "C++ Test on macOS"
+    strategy:
+      fail-fast: false
+      matrix:
+        version: [12, 13, 14]
+    runs-on: macos-${{ matrix.version }}
+    steps:
+    - name: Checkout repository
+      uses: actions/checkout@v4
+    - name: Install dependencies
+      run: |
+        brew update
+        brew install protobuf
+    - name: Test
+      run: |
+        CMAKE_PREFIX_PATH=$(brew --prefix protobuf)
+        mkdir -p build
+        cd build
+        cmake .. -DBUILD_JAVA=OFF -DPROTOBUF_HOME=${CMAKE_PREFIX_PATH}
+        make package test-out
+
diff --git a/cmake_modules/FindProtobuf.cmake b/cmake_modules/FindProtobuf.cmake
@@ -32,8 +32,17 @@ endif()
 
 message (STATUS "PROTOBUF_HOME: ${PROTOBUF_HOME}")
 
+if (NOT DEFINED CMAKE_STATIC_LIBRARY_SUFFIX)
+  if (WIN32)
+    set (CMAKE_STATIC_LIBRARY_SUFFIX ".lib")
+  else ()
+    set (CMAKE_STATIC_LIBRARY_SUFFIX ".a")
+  endif ()
+endif ()
+
 find_package (Protobuf CONFIG)
 if (Protobuf_FOUND)
+  if (TARGET protobuf::libprotobuf)
     set (PROTOBUF_LIBRARY protobuf::libprotobuf)
     set (PROTOBUF_STATIC_LIB PROTOBUF_STATIC_LIB-NOTFOUND)
     set (PROTOC_LIBRARY protobuf::libprotoc)
@@ -42,15 +51,34 @@ if (Protobuf_FOUND)
 
     get_target_property (target_type protobuf::libprotobuf TYPE)
     if (target_type STREQUAL "STATIC_LIBRARY")
-        set(PROTOBUF_STATIC_LIB protobuf::libprotobuf)
+      set (PROTOBUF_STATIC_LIB protobuf::libprotobuf)
     endif ()
 
     get_target_property (target_type protobuf::libprotoc TYPE)
     if (target_type STREQUAL "STATIC_LIBRARY")
-        set (PROTOC_STATIC_LIB protobuf::libprotoc)
+      set (PROTOC_STATIC_LIB protobuf::libprotoc)
+    endif ()
+
+    get_target_property (PROTOBUF_INCLUDE_DIR protobuf::libprotobuf INTERFACE_INCLUDE_DIRECTORIES)
+    if (NOT PROTOBUF_INCLUDE_DIR)
+      set (PROTOBUF_INCLUDE_DIR ${Protobuf_INCLUDE_DIRS})
+      if (NOT PROTOBUF_INCLUDE_DIR)
+        message (FATAL_ERROR "Cannot determine Protobuf include directory from protobuf::libprotobuf and Protobuf_INCLUDE_DIRS.")
+      endif ()
+    endif ()
+  else ()
+    set (PROTOBUF_LIBRARY ${Protobuf_LIBRARIES})
+    set (PROTOBUF_INCLUDE_DIR ${Protobuf_INCLUDE_DIRS})
+    if (NOT PROTOBUF_INCLUDE_DIR)
+      message (FATAL_ERROR "Cannot determine Protobuf include directory.")
     endif ()
 
-    get_target_property (PROTOBUF_INCLUDE_DIR protobuf::libprotoc INTERFACE_INCLUDE_DIRECTORIES)
+    if (Protobuf_LIBRARIES MATCHES "\\${CMAKE_STATIC_LIBRARY_SUFFIX}$")
+      set (PROTOBUF_STATIC_LIB ${Protobuf_LIBRARIES})
+    else ()
+      set (PROTOBUF_STATIC_LIB PROTOBUF_STATIC_LIB-NOTFOUND)
+    endif ()
+  endif ()
 
 else()
     find_path (PROTOBUF_INCLUDE_DIR google/protobuf/io/zero_copy_stream.h HINTS

diff --git a/tools/test/TestFileScan.cc b/tools/test/TestFileScan.cc
@@ -211,9 +211,22 @@ void checkForError(const std::string& filename, const std::string& error_msg) {
   EXPECT_NE(std::string::npos, error.find(error_msg)) << error;
 }
 
+void checkForError(const std::string& filename, const std::string& errorMsg1,
+                   const std::string& errorMsg2) {
+  const std::string pgm = findProgram("tools/src/orc-scan");
+  std::string output;
+  std::string error;
+  EXPECT_EQ(1, runProgram({pgm, filename}, output, error));
+  EXPECT_EQ("", output);
+  if (error.find(errorMsg1) == std::string::npos && error.find(errorMsg2) == std::string::npos) {
+    FAIL() << error;
+  }
+}
+
 TEST(TestFileScan, testErrorHandling) {
   checkForError(findExample("corrupt/stripe_footer_bad_column_encodings.orc"),
-                "bad number of ColumnEncodings in StripeFooter: expected=6, actual=0");
+                "bad number of ColumnEncodings in StripeFooter: expected=6, actual=0",
+                "bad StripeFooter from zlib");
   checkForError(findExample("corrupt/negative_dict_entry_lengths.orc"),
                 "Negative dictionary entry length");
   checkForError(findExample("corrupt/missing_length_stream_in_string_dict.orc"),