ARROW-17104: [C++] Workaround protobuf ABI issues #13634

pitrou · 2022-07-18T11:38:06Z

The protocol buffers ABI unfortunately depends on whether NDEBUG is enabled or not.
When installing gRPC using a package manager (Homebrew, conda...), most often it is built with NDEBUG enabled.
Some hackery is then needed to avoid missing symbols on Arrow debug builds...

Should also fix ARROW-16520.

pitrou · 2022-07-18T11:38:51Z

@github-actions crossbow submit -g cpp

pitrou · 2022-07-18T11:42:56Z

@lidavidm @raulcd This at least works for me with the conda-forge libprotobuf package.

cpp/src/arrow/util/protobuf_abi_fixup_internal.h

github-actions · 2022-07-18T12:07:03Z

https://issues.apache.org/jira/browse/ARROW-17104

github-actions · 2022-07-18T12:07:04Z

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

github-actions · 2022-07-18T12:08:24Z

Revision: a6a445c

Submitted crossbow builds: ursacomputing/crossbow @ actions-f4bb15b092

Task	Status
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-debian-10-cpp-amd64
test-debian-10-cpp-i386
test-debian-11-cpp-amd64
test-debian-11-cpp-i386
test-fedora-35-cpp
test-ubuntu-18.04-cpp
test-ubuntu-18.04-cpp-release
test-ubuntu-18.04-cpp-static
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-14
test-ubuntu-20.04-cpp-17
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-22.04-cpp

raulcd · 2022-07-18T13:00:39Z

Do we require a higher protobuf version for substrait with this change? I think the failures on Ubuntu 18.04 are due to version 3.0.0 being used.
Found Protobuf: /usr/lib/x86_64-linux-gnu/libprotobuf.so;-pthread (found suitable version "3.0.0", minimum required is "3.0.0")
From what I can see metadata_lite.h was added on >=3.3.x
Not present on 3.2 branch:
https://github.com/protocolbuffers/protobuf/tree/3.2.x/src/google/protobuf
present on 3.3.:
https://github.com/protocolbuffers/protobuf/tree/3.3.x/src/google/protobuf

pitrou · 2022-07-18T13:02:42Z

@raulcd No, this should hopefully be fixed using a compile-time check.

lidavidm · 2022-07-18T15:05:36Z

cpp/src/arrow/compute/exec/exec_plan.h

+// FIXME there are too many inclusions here; besides, arrow/compute/exec/util.h
+// includes even more, including arrow/util/logging.h which is forbidden
+// in public headers.


Filed https://issues.apache.org/jira/browse/ARROW-17109

pitrou · 2022-07-18T16:22:32Z

Unfortunately the MacOS Python issue still occurs:
https://github.com/apache/arrow/runs/7391963825?check_suite_focus=true#step:5:98

@lidavidm @kou does one of you have a MacOS machine to debug this?

lidavidm · 2022-07-18T16:36:10Z

I usually SSH into the Ursa Mac machines to debug this sort of issue. I could take a look but it might be a few days.

pitrou · 2022-07-18T16:51:45Z

@paleolimbot Can you help me debug this R CI failure? I cannot even reproduce locally (build succeeds here with Docker):
https://github.com/apache/arrow/runs/7393839585?check_suite_focus=true#step:7:4753

paleolimbot · 2022-07-18T17:02:00Z

Seems like #include <arrow/engine/substrait/api.h> is not sufficient for our usage in the R package? Maybe #include <arrow/engine/substrait/serde.h> is needed?

pitrou · 2022-07-18T17:03:42Z

Well, arrow/engine/substrait/api.h includes arrow/engine/substrait/serde.h.

paleolimbot · 2022-07-18T17:20:25Z

I also cannot reproduce locally (MacOS, M1). I will try locally on Linux as well...I don't quite understand the failure mode...even if ARROW_HOME was picking up a different version of Arrow than that of the commit, are there any versions of engine/substrait/api.h that have ever excluded the basic functionality of Substrait?

pitrou · 2022-07-18T17:24:04Z

are there any versions of engine/substrait/api.h that have ever excluded the basic functionality of Substrait?

Apparently not. It was like that from the start.

paleolimbot · 2022-07-18T17:28:43Z

I can't reproduce locally on Linux either. One other thing to try is change all ValueOrStop() and StopIfNotOk() calls to arrow::ValueOrStop()/arrow::StopIfNotOk(). I don't know why it's not complaining about any of the other ValueOrStop()/StopIfNotOk() calls in the file, but that does seem to be part of the problem?

pitrou · 2022-07-18T17:32:07Z

I'm not sure. TBH I'm a bit surprised these calls work at all in other instances. Perhaps because of argument-dependent lookup?

In any case, I think the arrow::engine::DeserializePlans and arrow::engine::internal errors should be investigated first.

pitrou · 2022-07-18T17:36:04Z

I'm a trying a cache-less build...

pitrou · 2022-07-18T18:09:42Z

Ok, I'm relieved that the issue isn't due to caching at least.

nealrichardson · 2022-07-18T19:14:48Z

cpp/src/arrow/compute/exec/exec_plan.h

@@ -22,6 +22,10 @@
 #include <string>
 #include <vector>

+// FIXME there are too many inclusions here; besides, arrow/compute/exec/util.h


Suggested change

// FIXME there are too many inclusions here; besides, arrow/compute/exec/util.h

// TODO(ARROW-17109): there are too many inclusions here; besides, arrow/compute/exec/util.h

pitrou · 2022-07-19T14:54:35Z

Ok, my latest attempt failed again.

pitrou · 2022-07-19T17:23:33Z

Well, finally it seems the issue may be fixed in the next upstream release:
protocolbuffers/protobuf#10271

kou · 2022-07-20T03:44:44Z

I've fixed GLib and Python failures.

kou · 2022-07-20T03:45:01Z

@github-actions crossbow submit -g cpp -g python

github-actions · 2022-07-20T04:08:34Z

Revision: 8b0b941

Submitted crossbow builds: ursacomputing/crossbow @ actions-f8f2014cd2

Task	Status
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-conda-python-3.10
test-conda-python-3.7
test-conda-python-3.7-hdfs-2.9.2
test-conda-python-3.7-hdfs-3.2.1
test-conda-python-3.7-kartothek-latest
test-conda-python-3.7-kartothek-master
test-conda-python-3.7-pandas-0.24
test-conda-python-3.7-pandas-latest
test-conda-python-3.7-spark-v3.1.2
test-conda-python-3.8
test-conda-python-3.8-hypothesis
test-conda-python-3.8-pandas-latest
test-conda-python-3.8-pandas-nightly
test-conda-python-3.8-spark-v3.2.0
test-conda-python-3.9
test-conda-python-3.9-dask-latest
test-conda-python-3.9-dask-master
test-conda-python-3.9-pandas-master
test-conda-python-3.9-spark-master
test-debian-10-cpp-amd64
test-debian-10-cpp-i386
test-debian-11-cpp-amd64
test-debian-11-cpp-i386
test-debian-11-python-3
test-fedora-35-cpp
test-fedora-35-python-3
test-ubuntu-18.04-cpp
test-ubuntu-18.04-cpp-release
test-ubuntu-18.04-cpp-static
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-14
test-ubuntu-20.04-cpp-17
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-20.04-python-3
test-ubuntu-22.04-cpp

kou · 2022-07-20T06:54:11Z

https://app.travis-ci.com/github/apache/arrow/jobs/577173431#L2887

In file included from /arrow/cpp/src/arrow/flight/protocol_internal.h:22,
                 from /arrow/cpp/src/arrow/flight/serialization_internal.h:22,
                 from /arrow/cpp/src/arrow/flight/client.cc:39,
                 from src/arrow/flight/CMakeFiles/arrow_flight_objlib.dir/Unity/unity_0_cxx.cxx:4:
/arrow/cpp/src/arrow/util/protobuf_abi_fixup_internal.h:39:2: error: #error "Protobuf already included, fixup can't be applied!"
   39 | #error "Protobuf already included, fixup can't be applied!"
      |  ^~~~~

It seems that we need a wrapper for Flight.pb.cc like cpp/src/arrow/flight/transport/grpc/protocol_grpc_internal.cc.

pitrou · 2022-07-20T07:51:57Z

Now that this is fixed upstream, my inclination would be to wait for conda-forge and homebrew packages to be updated.
We can also add an error message when compiling in debug mode with the offending protobuf versions.

kou · 2022-07-21T05:09:43Z

I think that we need this because other packaging system may ship Protobuf that has this problem.
If you don't want to complete this, can I take over this?

pitrou · 2022-07-21T07:26:06Z

I think that we need this because other packaging system may ship Protobuf that has this problem.

Which packaging systems ship the offending protobuf version?
This is mainly for us and our CI. If you compile Arrow in release mode, there is no problem. Third-party users are unlikely to compile Arrow in debug mode...

If you don't want to complete this, can I take over this?

@kou You can. But the fix is extremely brittle, so I'm in favour of not doing it since it's fixed upstream.

kou · 2022-07-21T08:00:34Z

This is mainly for us and our CI. If you compile Arrow in release mode, there is no problem.

Ah, I see. OK. We can close this.

raulcd · 2022-07-21T13:46:46Z

Should we still merge 9e10f6c independently? I think is required to fix the:

Program glib-mkenums mkenums found: NO
c_glib/arrow-glib/meson.build:216:0: ERROR: Program 'glib-mkenums mkenums' not found or not executable

Which happens on master?

pitrou · 2022-07-21T17:32:25Z

Right, and the added cpp/src/arrow/public_api_test.cc test (together with the necessary source code fixes) would be nice to get in as well. So let's keep this PR open until they're addressed by another PR?

pitrou · 2022-08-24T17:03:13Z

I opened #13965 for the public API test improvement.

pitrou force-pushed the ARROW-17104-grpc-abi branch from fdafabe to 1d78dd6 Compare July 18, 2022 11:41

pitrou force-pushed the ARROW-17104-grpc-abi branch from 1d78dd6 to a6a445c Compare July 18, 2022 11:49

lidavidm approved these changes Jul 18, 2022

View reviewed changes

cpp/src/arrow/util/protobuf_abi_fixup_internal.h Outdated Show resolved Hide resolved

github-actions bot added Component: FlightRPC Component: C++ labels Jul 18, 2022

pitrou force-pushed the ARROW-17104-grpc-abi branch from 413b687 to 815b1a3 Compare July 18, 2022 14:51

lidavidm mentioned this pull request Jul 18, 2022

ARROW-16911: [C++] Add Equals method to Partitioning #13567

Merged

github-actions bot added the Component: R label Jul 18, 2022

lidavidm approved these changes Jul 18, 2022

View reviewed changes

pitrou force-pushed the ARROW-17104-grpc-abi branch from 5301ee1 to 9582bd8 Compare July 18, 2022 16:32

pitrou force-pushed the ARROW-17104-grpc-abi branch from 3b2a345 to 9582bd8 Compare July 18, 2022 18:09

nealrichardson reviewed Jul 18, 2022

View reviewed changes

pitrou added 8 commits July 19, 2022 15:46

Fix compilation on old protobufs

be014d5

Try to fix some other issues

d459f17

More fixes, add comments

e100bbe

Fix ambiguous namespace on unity builds

2f7b334

Try to fix mystery R bug

9cc1723

Try to include file explicitly (why not?)

1f7205d

Set OpenTelemetry build type to protobuf build type

cd455ff

Try to force the destructor to be codegen'ed

9f281de

pitrou force-pushed the ARROW-17104-grpc-abi branch from 4874f99 to 9f281de Compare July 19, 2022 14:01

pitrou changed the title ~~ARROW-17104: [C++] Workaround gRPC ABI issues~~ ARROW-17104: [C++] Workaround protobuf ABI issues Jul 19, 2022

kou added 2 commits July 20, 2022 10:16

[GLib] Add glib-utils for glib-mkenums

9e10f6c

[C++] Use OpenTelemetry C++ in Homebrew

2b46e89

github-actions bot added the Component: GLib label Jul 20, 2022

kou added 2 commits July 20, 2022 11:43

[C++] Define NDEBUG in bundled Substrait

b6baaba

[C++] Define NDEBUG in bundled ORC

8b0b941

pitrou closed this Aug 24, 2022

asfimport mentioned this pull request Sep 13, 2022

[CI][Python] Pyarrow cannot be imported on CI job AMD64 MacOS 10.15 Python 3 #32409

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-17104: [C++] Workaround protobuf ABI issues #13634

ARROW-17104: [C++] Workaround protobuf ABI issues #13634

pitrou commented Jul 18, 2022 •

edited

Loading

pitrou commented Jul 18, 2022

pitrou commented Jul 18, 2022

github-actions bot commented Jul 18, 2022

github-actions bot commented Jul 18, 2022

github-actions bot commented Jul 18, 2022

raulcd commented Jul 18, 2022

pitrou commented Jul 18, 2022

lidavidm Jul 18, 2022

pitrou commented Jul 18, 2022

lidavidm commented Jul 18, 2022

pitrou commented Jul 18, 2022

paleolimbot commented Jul 18, 2022

pitrou commented Jul 18, 2022

paleolimbot commented Jul 18, 2022

pitrou commented Jul 18, 2022 •

edited

Loading

paleolimbot commented Jul 18, 2022

pitrou commented Jul 18, 2022

pitrou commented Jul 18, 2022

pitrou commented Jul 18, 2022

nealrichardson Jul 18, 2022

pitrou commented Jul 19, 2022

pitrou commented Jul 19, 2022

kou commented Jul 20, 2022

kou commented Jul 20, 2022

github-actions bot commented Jul 20, 2022

kou commented Jul 20, 2022

pitrou commented Jul 20, 2022

kou commented Jul 21, 2022

pitrou commented Jul 21, 2022

kou commented Jul 21, 2022

raulcd commented Jul 21, 2022

pitrou commented Jul 21, 2022

pitrou commented Aug 24, 2022

	// FIXME there are too many inclusions here; besides, arrow/compute/exec/util.h
	// TODO(ARROW-17109): there are too many inclusions here; besides, arrow/compute/exec/util.h

ARROW-17104: [C++] Workaround protobuf ABI issues #13634

ARROW-17104: [C++] Workaround protobuf ABI issues #13634

Conversation

pitrou commented Jul 18, 2022 • edited Loading

pitrou commented Jul 18, 2022

pitrou commented Jul 18, 2022

github-actions bot commented Jul 18, 2022

github-actions bot commented Jul 18, 2022

github-actions bot commented Jul 18, 2022

raulcd commented Jul 18, 2022

pitrou commented Jul 18, 2022

lidavidm Jul 18, 2022

Choose a reason for hiding this comment

pitrou commented Jul 18, 2022

lidavidm commented Jul 18, 2022

pitrou commented Jul 18, 2022

paleolimbot commented Jul 18, 2022

pitrou commented Jul 18, 2022

paleolimbot commented Jul 18, 2022

pitrou commented Jul 18, 2022 • edited Loading

paleolimbot commented Jul 18, 2022

pitrou commented Jul 18, 2022

pitrou commented Jul 18, 2022

pitrou commented Jul 18, 2022

nealrichardson Jul 18, 2022

Choose a reason for hiding this comment

pitrou commented Jul 19, 2022

pitrou commented Jul 19, 2022

kou commented Jul 20, 2022

kou commented Jul 20, 2022

github-actions bot commented Jul 20, 2022

kou commented Jul 20, 2022

pitrou commented Jul 20, 2022

kou commented Jul 21, 2022

pitrou commented Jul 21, 2022

kou commented Jul 21, 2022

raulcd commented Jul 21, 2022

pitrou commented Jul 21, 2022

pitrou commented Aug 24, 2022

pitrou commented Jul 18, 2022 •

edited

Loading

pitrou commented Jul 18, 2022 •

edited

Loading