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

Build failure: CMake Error at third_party/abseil-cpp/CMake/AbseilHelpers.cmake:317 (target_link_libraries): #20

Closed
emarsden opened this issue Jul 12, 2024 · 10 comments

Comments

@emarsden
Copy link

The build process fails with the error shown above, see for example the following GitHub action failure

https://github.com/emarsden/dash-mpd-rs/actions/runs/9903706206/job/27359686497#step:9:720

This is due to a bug in abseil-cpp which has been fixed upstream (and I believe in the latest protobuf version 27.2). Would it be possible to update to the latest protobuf version?

Thanks!

@benesch
Copy link
Member

benesch commented Jul 15, 2024

A bit short on time at the moment, but happy to take a PR that bumps the version!

@Guara92
Copy link
Contributor

Guara92 commented Jul 16, 2024

I opened #21 trying to fix building errors in CI but the error is still there, @emarsden can you point to the fix?

@emarsden
Copy link
Author

As an aside, looking at the size of your patch, I'm thinking it might be better to checkout the right branch of the protobuf source code from the build.rs script, instead of keeping a copy here. I see that some other Rust crates do this, but don't know what is considered best practice.

I think your problem is that you haven't checked out the abseil-cpp source code, which needs to be done as a separate step.

The steps that work for me (on Linux, haven't tested elsewhere), from the rust-protobuf-native/protobuf-src directory:

rm -f protobuf
git clone --depth 1 --branch v27.2 https://github.com/protocolbuffers/protobuf
cd protobuf/third_party 
git clone https://github.com/abseil/abseil-cpp.git abseil-cpp

then the crate builds.

@emarsden
Copy link
Author

I forked the repository to check whether the "checkout src from build.rs instead of vendoring" concept is workable, visible here (not prepared as a proper PR, but I could do that if there is interest)

https://github.com/emarsden/rust-protobuf-native

This builds OK on the GitHub runners on Linux, Windows (using msys2) and MacOS. It requires git to be installed, which is not currently a dependency.

emarsden added a commit to emarsden/pssh-box-rs that referenced this issue Jul 20, 2024
Temporary workaround pending resolution of the following issue

   MaterializeInc/rust-protobuf-native#20
@Guara92
Copy link
Contributor

Guara92 commented Jul 20, 2024

As an aside, looking at the size of your patch, I'm thinking it might be better to checkout the right branch of the protobuf source code from the build.rs script, instead of keeping a copy here. I see that some other Rust crates do this, but don't know what is considered best practice.

I think your problem is that you haven't checked out the abseil-cpp source code, which needs to be done as a separate step.

The steps that work for me (on Linux, haven't tested elsewhere), from the rust-protobuf-native/protobuf-src directory:

rm -f protobuf
git clone --depth 1 --branch v27.2 https://github.com/protocolbuffers/protobuf
cd protobuf/third_party 
git clone https://github.com/abseil/abseil-cpp.git abseil-cpp

then the crate builds.

My problem is in CI of projects using rust-protobuf as a dependency with the same issue you had here.
BTW in my PR I checked out all third parties submodules and I have no problems on building rust-protobuf or a project using it as a dependency on my local setup (Fedora 40)

@Guara92
Copy link
Contributor

Guara92 commented Jul 20, 2024

I forked the repository to check whether the "checkout src from build.rs instead of vendoring" concept is workable, visible here (not prepared as a proper PR, but I could do that if there is interest)

https://github.com/emarsden/rust-protobuf-native

This builds OK on the GitHub runners on Linux, Windows (using msys2) and MacOS. It requires git to be installed, which is not currently a dependency.

Another solution could be having it as a git submodule, honestly I don't know which one would be better but yeah I would not say that having a copy of the original src here is the best solution

@emarsden
Copy link
Author

This problem only arises with cmake v3.30; cmake v3.29 works (and is perhaps what you have on Fedora 40).

I've updated my fork to check out source code using the git2-rs library, so that there is no build-time dependency on the git executable, and sent a pull request.

#22

@benesch
Copy link
Member

benesch commented Jul 22, 2024

It looks like the problem is actually with Abseil, not libprotobuf: open-telemetry/opentelemetry-cpp#2998 (comment)

I've reworked #21 to upgrade both libprotobuf and Abseil. Even though only the latter is necessary, but might as well bump to v27.2 of libprotobuf while we're here.

In terms of vendoring libprotobuf/abseil, I'd like to stick with the current approach. While it makes for large diffstats, I find this to be nonetheless my ideal balance of tradeoffs, as it pushes most of the pain onto the maintainer. By vendoring directly into the repository:

  • Downstream users don't need internet access when building, since if Cargo downloads the crate successfully all necessary files are included.
  • We're not reliant on upstream remaining available at the URLs encoded in this repository. libprotobuf and Abseil are pretty trustworthy, but it's still nice not to have an enduring dependency on their tarballs and/or Git repositories.
  • We're not relying on Cargo to check out Git submodules. Cargo has pretty good support for crates that include Git submodules, but historically this has been somewhat flaky, and breaks for some users who have Git configured in a way that Cargo doesn't expect.

@benesch
Copy link
Member

benesch commented Jul 22, 2024

By the way, there's a bin/update-protobuf script that handles the mechanics of downloading the latest versions of libprotobuf and Abseil and putting them in the right place. Not sure if y'all saw! 🙈 I pushed an update to MAINTAINERS.md in #21 that explains how to run bin/update-protobuf to hopefully help the next person who tackles a version upgrade.

@benesch
Copy link
Member

benesch commented Jul 22, 2024

Should be fixed by protobuf-src v2.1.0: https://crates.io/crates/protobuf-src/2.1.0+27.1.

Thanks for the report and the help getting this fixed!

@benesch benesch closed this as completed Jul 22, 2024
Michael-J-Ward added a commit to Michael-J-Ward/datafusion-python that referenced this issue Jul 25, 2024
The new version **should** remove the need for the blocked GH action.

Ref: MaterializeInc/rust-protobuf-native#20
Ref: apache#763
andygrove pushed a commit to apache/datafusion-python that referenced this issue Jul 25, 2024
* update deps to force new protobuf-src

The new version **should** remove the need for the blocked GH action.

Ref: MaterializeInc/rust-protobuf-native#20
Ref: #763

* remove googletest-installer github action

Fixes #763

* remove needless Clone of TimeUnit type which is Copy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants