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.

Sign up for GitHub

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

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

Closed
wants to merge 10 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
032d9a6
ORC-1732:[C++] fix failure to detect Protobuf installed by Homebrew o…
luffy-zh Jun 27, 2024
18dac8c
format code
luffy-zh Jun 27, 2024
6f29a36
add macos ci job test
luffy-zh Jul 1, 2024
4abc8d1
fix build_and_test run failure
luffy-zh Jul 3, 2024
e6ad656
Update .github/workflows/build_and_test.yml
luffy-zh Jul 8, 2024
30108ae
Update .github/workflows/build_and_test.yml
luffy-zh Jul 8, 2024
8958559
Update cmake_modules/FindProtobuf.cmake
luffy-zh Jul 9, 2024
a9326e8
Update cmake_modules/FindProtobuf.cmake
luffy-zh Jul 9, 2024
7ddf8ce
Update cmake_modules/FindProtobuf.cmake
luffy-zh Jul 9, 2024
d479f05
add 12 to macOS version list
luffy-zh Jul 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions .github/workflows/build_and_test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,3 +197,25 @@ jobs:
with:
config: .github/.licenserc.yaml

macos-cpp-check:
name: "C++ Test on macOS"
strategy:
fail-fast: false
matrix:
version: [12, 14]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we skip 13?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no significant difference between 13 and 12 for C++ compilation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then, please add 13 too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

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

34 changes: 31 additions & 3 deletions cmake_modules/FindProtobuf.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,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)
Expand All @@ -55,15 +64,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
Expand Down
15 changes: 14 additions & 1 deletion tools/test/TestFileScan.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,9 +211,22 @@ void checkForError(const std::string& filename, const std::string& errorMsg) {
EXPECT_NE(std::string::npos, error.find(errorMsg)) << 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",
wgtmac marked this conversation as resolved.
Show resolved Hide resolved
"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"),
Expand Down
Loading