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 2 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
26 changes: 23 additions & 3 deletions cmake_modules/FindProtobuf.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ message (STATUS "PROTOBUF_HOME: ${PROTOBUF_HOME}")

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 +56,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::libprotoc INTERFACE_INCLUDE_DIRECTORIES)
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.")
Copy link
Member

Choose a reason for hiding this comment

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

How about showing detection sources too for easy to debug?

Suggested change
message(FATAL_ERROR "Cannot determine Protobuf include directory.")
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.")
luffy-zh marked this conversation as resolved.
Show resolved Hide resolved
endif ()

if (Protobuf_LIBRARIES MATCHES "\\.a$")
Copy link
Member

Choose a reason for hiding this comment

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

We may be able to use https://cmake.org/cmake/help/latest/variable/CMAKE_STATIC_LIBRARY_SUFFIX.html instead of .a.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for looking at that, '.a' has been replaced with 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