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.

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

[C++] CI failures on Windows & Substrait (appears to be abseil linking issues) #36598

Closed
westonpace opened this issue Jul 10, 2023 · 7 comments · Fixed by #36606
Closed

[C++] CI failures on Windows & Substrait (appears to be abseil linking issues) #36598

westonpace opened this issue Jul 10, 2023 · 7 comments · Fixed by #36606
Assignees
Milestone

Comments

@westonpace
Copy link
Member

Describe the bug, including details regarding any error messages, version, and platform.

I've seen this CI failure pop up recently on two unrelated PRs.

Example:

https://github.com/apache/arrow/actions/runs/5509303570/jobs/10041817398?pr=35440

 FAILED: release/libarrow_substrait.dll release/libarrow_substrait.dll.a 
cmd.exe /C "cd . && D:\a\_temp\msys64\mingw32\bin\c++.exe -Wno-noexcept-type -Wno-self-move  -fdiagnostics-color=always  -Wa,-mbig-obj -Wall -Wno-conversion -Wno-sign-conversion -Wunused-result -Wdate-time -fno-semantic-interposition -mxsave -msse4.2  -O3 -DNDEBUG -O2 -ftree-vectorize  -Wl,--version-script=D:/a/arrow/arrow/cpp/src/arrow/symbols.map -shared -o release\libarrow_substrait.dll -Wl,--out-implib,release\libarrow_substrait.dll.a -Wl,--major-image-version,1300,--minor-image-version,0 @CMakeFiles\arrow_substrait_shared.rsp  && cd ."
D:/a/_temp/msys64/mingw32/bin/../lib/gcc/i686-w64-mingw32/13.1.0/../../../../i686-w64-mingw32/bin/ld.exe: src/arrow/engine/CMakeFiles/arrow_substrait_shared.dir/substrait/serde.cc.obj:serde.cc:(.text+0xf30): undefined reference to `absl::lts_20230125::operator<<(std::ostream&, absl::lts_20230125::Status const&)'
D:/a/_temp/msys64/mingw32/bin/../lib/gcc/i686-w64-mingw32/13.1.0/../../../../i686-w64-mingw32/bin/ld.exe: src/arrow/engine/CMakeFiles/arrow_substrait_shared.dir/substrait/serde.cc.obj:serde.cc:(.text+0x101c): undefined reference to `absl::lts_20230125::Status::UnrefNonInlined(unsigned int)'
D:/a/_temp/msys64/mingw32/bin/../lib/gcc/i686-w64-mingw32/13.1.0/../../../../i686-w64-mingw32/bin/ld.exe: src/arrow/engine/CMakeFiles/arrow_substrait_shared.dir/substrait/serde.cc.obj:serde.cc:(.text+0x141d): undefined reference to `absl::lts_20230125::operator<<(std::ostream&, absl::lts_20230125::Status const&)'
D:/a/_temp/msys64/mingw32/bin/../lib/gcc/i686-w64-mingw32/13.1.0/../../../../i686-w64-mingw32/bin/ld.exe: src/arrow/engine/CMakeFiles/arrow_substrait_shared.dir/substrait/serde.cc.obj:serde.cc:(.text+0x14f1): undefined reference to `absl::lts_20230125::Status::UnrefNonInlined(unsigned int)'
D:/a/_temp/msys64/mingw32/bin/../lib/gcc/i686-w64-mingw32/13.1.0/../../../../i686-w64-mingw32/bin/ld.exe: src/arrow/engine/CMakeFiles/arrow_substrait_shared.dir/substrait/serde.cc.obj:serde.cc:(.text$_ZN4absl12lts_202301256StatusD1Ev[__ZN4absl12lts_202301256StatusD1Ev]+0x17): undefined reference to `absl::lts_20230125::Status::UnrefNonInlined(unsigned int)'
D:/a/_temp/msys64/mingw32/bin/../lib/gcc/i686-w64-mingw32/13.1.0/../../../../i686-w64-mingw32/bin/ld.exe: release/libsubstrait.a(algebra.pb.cc.obj):algebra.pb.cc:(.text$_ZN6google8protobuf13RepeatedFieldIiE14GrowNoAnnotateEii[__ZN6google8protobuf13RepeatedFieldIiE14GrowNoAnnotateEii]+0x8d): undefined reference to `__emutls_v._ZN6google8protobuf8internal15ThreadSafeArena13thread_cache_E'

It's very consistent. I am going to start investigating but wanted to create an issue in case someone was already looking into it.

Component(s)

C++

@westonpace
Copy link
Member Author

From what I can tell so far is that the mingw builds are installing a system-wide grpc, protobuf, and abseil. So, IIUC, those should all be compatible and nothing should be getting bundled?

@westonpace
Copy link
Member Author

Ok. I have confirmed locally that I get these exact errors when I try the versions of protobuf and abseil that pacman is installing:

libprotobuf               4.23.1               hd1fb520_0    conda-forge
libabseil                 20230125.3      cxx17_h59595ed_0    conda-forge

...which is unfortunate. Investigating further.

@westonpace
Copy link
Member Author

#35987 is probably related. It seems we want a newer version of protobuf.

@westonpace
Copy link
Member Author

@kou there is no protobuf 4.23.3 in mingw yet. Should we create an issue/PR at https://github.com/msys2/MINGW-packages ? I don't work with mingw much so I don't know the best approach.

@kou
Copy link
Member

kou commented Jul 11, 2023

Thanks for investing this!
I'll open a pull request to fix this.

kou added a commit to kou/arrow that referenced this issue Jul 11, 2023
* We need to use protobuf-config.cmake provided by Protobuf instead of
  FindProtobuf.cmake provided by CMake because FindProtobuf.cmake
  misses absl::status dependency.
* Accept Protobuf 23.4.
* Use PROTOBUF_USE_DLLS when we build substrait related files.
pitrou pushed a commit that referenced this issue Jul 12, 2023
### Rationale for this change

There are 2 problems:

* `FindProtobuf.cmake` provided by CMake is incomplete with Protobuf 23.4.
* Misses `-DPROTOBUF_USE_DLLS` for building Substrait related files.

### What changes are included in this PR?

* We need to use `protobuf-config.cmake` provided by Protobuf instead of `FindProtobuf.cmake` provided by CMake because `FindProtobuf.cmake` misses `absl::status` dependency.
* Accept Protobuf 23.4.
* Use `PROTOBUF_USE_DLLS` when we build Substrait related files.
* Use `Boost_INCLUDE_DIRS` instead of `Boost_INCLUDE_DIR` because `Boost_INCLUDE_DIR` isn't defined in `BoostConfig.cmake`.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

Yes.
* Closes: #36598

Authored-by: Sutou Kouhei <[email protected]>
Signed-off-by: Antoine Pitrou <[email protected]>
@pitrou pitrou added this to the 13.0.0 milestone Jul 12, 2023
@raulcd
Copy link
Member

raulcd commented Jul 13, 2023

@pitrou @kou should this be added to 13.0.0, I would think so.

raulcd pushed a commit that referenced this issue Jul 13, 2023
### Rationale for this change

There are 2 problems:

* `FindProtobuf.cmake` provided by CMake is incomplete with Protobuf 23.4.
* Misses `-DPROTOBUF_USE_DLLS` for building Substrait related files.

### What changes are included in this PR?

* We need to use `protobuf-config.cmake` provided by Protobuf instead of `FindProtobuf.cmake` provided by CMake because `FindProtobuf.cmake` misses `absl::status` dependency.
* Accept Protobuf 23.4.
* Use `PROTOBUF_USE_DLLS` when we build Substrait related files.
* Use `Boost_INCLUDE_DIRS` instead of `Boost_INCLUDE_DIR` because `Boost_INCLUDE_DIR` isn't defined in `BoostConfig.cmake`.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

Yes.
* Closes: #36598

Authored-by: Sutou Kouhei <[email protected]>
Signed-off-by: Antoine Pitrou <[email protected]>
@raulcd
Copy link
Member

raulcd commented Jul 13, 2023

I've decided to add it.

chelseajonesr pushed a commit to chelseajonesr/arrow that referenced this issue Jul 20, 2023
…ache#36606)

### Rationale for this change

There are 2 problems:

* `FindProtobuf.cmake` provided by CMake is incomplete with Protobuf 23.4.
* Misses `-DPROTOBUF_USE_DLLS` for building Substrait related files.

### What changes are included in this PR?

* We need to use `protobuf-config.cmake` provided by Protobuf instead of `FindProtobuf.cmake` provided by CMake because `FindProtobuf.cmake` misses `absl::status` dependency.
* Accept Protobuf 23.4.
* Use `PROTOBUF_USE_DLLS` when we build Substrait related files.
* Use `Boost_INCLUDE_DIRS` instead of `Boost_INCLUDE_DIR` because `Boost_INCLUDE_DIR` isn't defined in `BoostConfig.cmake`.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

Yes.
* Closes: apache#36598

Authored-by: Sutou Kouhei <[email protected]>
Signed-off-by: Antoine Pitrou <[email protected]>
R-JunmingChen pushed a commit to R-JunmingChen/arrow that referenced this issue Aug 20, 2023
…ache#36606)

### Rationale for this change

There are 2 problems:

* `FindProtobuf.cmake` provided by CMake is incomplete with Protobuf 23.4.
* Misses `-DPROTOBUF_USE_DLLS` for building Substrait related files.

### What changes are included in this PR?

* We need to use `protobuf-config.cmake` provided by Protobuf instead of `FindProtobuf.cmake` provided by CMake because `FindProtobuf.cmake` misses `absl::status` dependency.
* Accept Protobuf 23.4.
* Use `PROTOBUF_USE_DLLS` when we build Substrait related files.
* Use `Boost_INCLUDE_DIRS` instead of `Boost_INCLUDE_DIR` because `Boost_INCLUDE_DIR` isn't defined in `BoostConfig.cmake`.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

Yes.
* Closes: apache#36598

Authored-by: Sutou Kouhei <[email protected]>
Signed-off-by: Antoine Pitrou <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants