ORC-1732: [C++] Fix detecting Homebrew-installed Protobuf on MacOS by luffy-zh · Pull Request #1963 · apache/orc

luffy-zh · 2024-06-27T09:36:51Z

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

How was this patch tested?

Test it locally.

Was this patch authored or co-authored using generative AI tooling?

NO

…n macOS

tools/test/TestFileScan.cc

wgtmac · 2024-06-28T15:21:32Z

@kou Could you help review this? Thanks!

kou

How about adding a CI job on macOS?
https://github.com/apache/orc/compare/main...kou:orc:cpp-protobuf?expand=1#diff-48c0ee97c53013d18d6bbae44648f7fab9af2e0bf5b0dc1ca761e18ec5c478f2R101-R133 is a sample configuration.

kou · 2024-07-01T02:51:09Z

cmake_modules/FindProtobuf.cmake

+      message(FATAL_ERROR "Cannot determine Protobuf include directory.")
+    endif ()
+
+    if (Protobuf_LIBRARIES MATCHES "\\.a$")


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

Thanks for looking at that, '.a' has been replaced with CMAKE_STATIC_LIBRARY_SUFFIX

kou · 2024-07-01T02:52:53Z

cmake_modules/FindProtobuf.cmake

+    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.")


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.")

wgtmac · 2024-07-02T02:01:39Z

The new github action doesn't run. I found the error message below:

Invalid workflow file: .github/workflows/build_and_test.yml#L224
The workflow is not valid. .github/workflows/build_and_test.yml (Line: 224, Col: 7): 'run' is already defined .github/workflows/build_and_test.yml (Line: 225, Col: 7): 'run' is already defined

https://github.com/apache/orc/actions/runs/9746489872

.github/workflows/build_and_test.yml

Co-authored-by: Gang Wu <ustcwg@gmail.com>

wgtmac

+1

Thanks @luffy-zh for the fix and @kou for reviewing it!

kou

+1

cmake_modules/FindProtobuf.cmake

Co-authored-by: Sutou Kouhei <kou@cozmixng.org>

dongjoon-hyun · 2024-07-09T19:21:04Z

.github/workflows/build_and_test.yml

+    strategy:
+      fail-fast: false
+      matrix:
+        version: [12, 14]


Why do we skip 13?

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

Then, please add 13 too.

dongjoon-hyun

+1, LGTM.

### 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>

dongjoon-hyun · 2024-07-11T15:45:21Z

Thank you, @luffy-zh , @wgtmac , @kou .
Merged to main/2.0.

### Rationale for this change This PR aims to upgrade ORC to 2.0.3 to bring the following bug fixes. - https://orc.apache.org/news/2024/08/15/ORC-2.0.2/ - apache/orc#1963 - apache/orc#1997 - apache/orc#1981 - https://orc.apache.org/news/2024/11/14/ORC-2.0.3/ - apache/orc#2055 ### What changes are included in this PR? To use the latest bug fixed version. ### Are these changes tested? This should pass the CIs. ### Are there any user-facing changes? No. * GitHub Issue: #44744 Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

ORC-1732:[C++] fix failure to detect Protobuf installed by Homebrew o…

032d9a6

…n macOS

github-actions bot added BUILD CPP labels Jun 27, 2024

format code

18dac8c

wgtmac reviewed Jun 28, 2024

View reviewed changes

tools/test/TestFileScan.cc Show resolved Hide resolved

kou reviewed Jul 1, 2024

View reviewed changes

add macos ci job test

6f29a36

github-actions bot added the INFRA label Jul 1, 2024

fix build_and_test run failure

4abc8d1

wgtmac reviewed Jul 3, 2024

View reviewed changes

.github/workflows/build_and_test.yml Outdated Show resolved Hide resolved

wgtmac reviewed Jul 3, 2024

View reviewed changes

.github/workflows/build_and_test.yml Show resolved Hide resolved

wgtmac changed the title ~~ORC-1732:[C++] fix failure to detect Protobuf installed by Homebrew o…~~ ORC-1732:[C++] Fix detecting Homebrew-installed Protobuf on MacOS Jul 3, 2024

wgtmac changed the title ~~ORC-1732:[C++] Fix detecting Homebrew-installed Protobuf on MacOS~~ ORC-1732: [C++] Fix detecting Homebrew-installed Protobuf on MacOS Jul 4, 2024

luffy-zh and others added 2 commits July 8, 2024 10:33

Update .github/workflows/build_and_test.yml

e6ad656

Co-authored-by: Gang Wu <ustcwg@gmail.com>

Update .github/workflows/build_and_test.yml

30108ae

Co-authored-by: Gang Wu <ustcwg@gmail.com>

wgtmac approved these changes Jul 8, 2024

View reviewed changes

kou approved these changes Jul 8, 2024

View reviewed changes

cmake_modules/FindProtobuf.cmake Outdated Show resolved Hide resolved

cmake_modules/FindProtobuf.cmake Outdated Show resolved Hide resolved

cmake_modules/FindProtobuf.cmake Outdated Show resolved Hide resolved

luffy-zh and others added 3 commits July 9, 2024 12:07

Update cmake_modules/FindProtobuf.cmake

8958559

Co-authored-by: Sutou Kouhei <kou@cozmixng.org>

Update cmake_modules/FindProtobuf.cmake

a9326e8

Co-authored-by: Sutou Kouhei <kou@cozmixng.org>

Update cmake_modules/FindProtobuf.cmake

7ddf8ce

Co-authored-by: Sutou Kouhei <kou@cozmixng.org>

dongjoon-hyun reviewed Jul 9, 2024

View reviewed changes

add 12 to macOS version list

d479f05

dongjoon-hyun approved these changes Jul 11, 2024

View reviewed changes

dongjoon-hyun closed this in a9e0351 Jul 11, 2024

dongjoon-hyun added this to the 2.0.2 milestone Jul 11, 2024

kou mentioned this pull request Jul 31, 2024

GH-43416: [CI] Upgrade our vcpkg version on .env apache/arrow#43417

Closed

raulcd mentioned this pull request Aug 6, 2024

ORC-1751: [C++] fix syntax error in ThirdpartyToolchain #1994

Closed

This was referenced Nov 15, 2024

[C++] Upgrade ORC to 2.0.3 apache/arrow#44744

Closed

GH-44744: [C++] Upgrade ORC to 2.0.3 apache/arrow#44745

Merged

	message(FATAL_ERROR "Cannot determine Protobuf include directory.")
	message(FATAL_ERROR "Cannot determine Protobuf include directory from protobuf::libprotobuf and Protobuf_INCLUDE_DIRS.")

Conversation

luffy-zh commented Jun 27, 2024

What changes were proposed in this pull request?

Why are the changes needed?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

Uh oh!

Uh oh!

wgtmac commented Jun 28, 2024

Uh oh!

kou left a comment

Choose a reason for hiding this comment

Uh oh!

kou Jul 1, 2024

Choose a reason for hiding this comment

Uh oh!

luffy-zh Jul 3, 2024

Choose a reason for hiding this comment

Uh oh!

kou Jul 1, 2024

Choose a reason for hiding this comment

Uh oh!

wgtmac commented Jul 2, 2024

Uh oh!

Uh oh!

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Uh oh!

kou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dongjoon-hyun Jul 9, 2024

Choose a reason for hiding this comment

Uh oh!

luffy-zh Jul 10, 2024

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Jul 10, 2024

Choose a reason for hiding this comment

Uh oh!

luffy-zh Jul 11, 2024

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Jul 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants