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

Don't make users install protoc #3948

Closed
wants to merge 5 commits into from

Conversation

avantgardnerio
Copy link
Contributor

Which issue does this PR close?

Closes #3947.

Rationale for this change

Described in issue.

What changes are included in this PR?

  1. have build.rs download and extract protoc
  2. remove all references to installing protoc in the base OS

Are there any user-facing changes?

No 🤞

@tustvold
Copy link
Contributor

tustvold commented Oct 24, 2022

An alternative that might be less potentially problematic, as there are reasons prost stopped vending protoc binaries, might be to vend the generated descriptor set. I experimented with doing this for pbjson-types in influxdata/pbjson#79.

Another option might just be to vend the generated code, perhaps using something like https://docs.rs/protoc-gen-prost/latest/protoc_gen_prost/

Edit: I will have a play with the latter option today

@avantgardnerio
Copy link
Contributor Author

there are reasons prost stopped vending protoc

@tustvold skimming the reasons, it looks like they are down to user control. That made me realize I was overriding user choice. I added new code that does a no-op if the user has already set the PROTOC env var to their compiler of choice... does that resolve your concerns with this PR?

@tustvold
Copy link
Contributor

That's part of it, the other part is a lack of desire to maintain code that acts as a DIY package manager. The auto-generation of code for prost seems to just be a large source of issues, I'm inclined to think we should just check in the generated sources, like we do for thrift or flatbuffers, and move on 😅

@avantgardnerio
Copy link
Contributor Author

Putting this down as a historical PSA: there is more discussion in the alternative PR.

@avantgardnerio
Copy link
Contributor Author

Closing in favor of #3947

@avantgardnerio avantgardnerio deleted the bg_protoc branch October 26, 2022 20:27
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

Successfully merging this pull request may close these issues.

Don't make dependants install protoc
2 participants