Skip to content
This repository has been archived by the owner on Jan 22, 2025. It is now read-only.

Bump prost, tonic to pick up protoc fix #26854

Merged
merged 3 commits into from
Aug 2, 2022
Merged

Bump prost, tonic to pick up protoc fix #26854

merged 3 commits into from
Aug 2, 2022

Conversation

CriesofCarrots
Copy link
Contributor

@CriesofCarrots CriesofCarrots commented Jul 29, 2022 •
edited
Loading

Problem

Protobufs build fails often in CI. Appears to be this issue: tokio-rs/prost#653

Summary of Changes

Bump prost (and related deps) to pick up fix.

3rd commit is still a question mark for me. This commit moves to compiling protoc from source, which definitely adds some time to fresh builds, but might make building less brittle. As of #25424, we recommend installing the protobuf compiler, but I'm not sure very many people have, since it doesn't seem to actually be necessary.

@CriesofCarrots CriesofCarrots changed the title Bump prost, tonic to pickup protoc fix Bump prost, tonic to pick up protoc fix Jul 29, 2022
@CriesofCarrots CriesofCarrots force-pushed the bump-prost branch 3 times, most recently from 8bf5583 to 5737be0 Compare July 29, 2022 22:34
@CriesofCarrots CriesofCarrots mentioned this pull request Jul 30, 2022
Closed
@riptl riptl mentioned this pull request Jul 31, 2022
Merged
t-nelson
t-nelson reviewed Aug 2, 2022
View reviewed changes
storage-bigtable/build-proto/src/main.rs Outdated Show resolved Hide resolved
storage-proto/build.rs Outdated Show resolved Hide resolved
@CriesofCarrots CriesofCarrots marked this pull request as ready for review August 2, 2022 17:31
@CriesofCarrots CriesofCarrots requested a review from t-nelson August 2, 2022 17:31
t-nelson
t-nelson approved these changes Aug 2, 2022
View reviewed changes
Copy link
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

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

thanks!

storage-bigtable/build-proto/src/main.rs
Comment on lines +3 to +5
if std::env::var(PROTOC_ENVAR).is_err() {
std::env::set_var(PROTOC_ENVAR, protobuf_src::protoc());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

could just unwrap_or(), but this works too

@CriesofCarrots CriesofCarrots merged commit b15be03 into solana-labs:master Aug 2, 2022
@riptl
Copy link
Contributor

riptl commented Aug 4, 2022

It seems like prost building from source causes issue with the Homebrew package manager: Homebrew/homebrew-core#106928 (comment)
But I don't think the Homebrew solana package is using any of the Protobuf-enabled packages atm, so probably ok

@jstarry
Copy link
Contributor

jstarry commented Aug 5, 2022

Is it possible that this change is causing issues here?

https://github.com/solana-labs/solana/runs/7696195623?check_suite_focus=true

error: failed to run custom build command for protobuf-src v1.0.5+3.19.3

@riptl
Copy link
Contributor

riptl commented Aug 5, 2022

Is it possible that this change is causing issues here?

@jstarry Yes, since protobuf-src needs a C compiler. Good luck with that on Windows 😂
If Visual Studio Command-Line Tools is stable enough, we can try keeping protobuf-src. Otherwise I would recommend setting the PROTOC_NO_VENDOR=1 env to use system protoc (maybe just getting the latest version from Chocolatey).

checking whether the C compiler works... no

@CriesofCarrots CriesofCarrots mentioned this pull request Aug 8, 2022
Merged
@CriesofCarrots CriesofCarrots mentioned this pull request Jan 10, 2023
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@t-nelson t-nelson t-nelson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@CriesofCarrots @riptl @jstarry @t-nelson