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(deps,ci): add protoc feature to build protoc from source #356

Merged
merged 4 commits into from
Jan 8, 2025

Conversation

wackywendell
Copy link
Contributor

@wackywendell wackywendell commented Jan 6, 2025

Fixes #355, by using the same setup for protoc as substrait-io/substrait-rs.

Testing

I checked locally:

  1. Download [email protected] from here, to get the same version as is used in the docs.rs workspace (ubuntu 22.04).
  2. Run PATH="/path/to/downloaded/protobuf/bin:$PATH" cargo doc
  • Shows me the error Error: Custom { kind: Other, error: "protoc failed: substrait/algebra.proto: This file contains proto3 optional fields, but --experimental_allow_proto3_optional was not set.\n" }, as seen in the docs.rs build
  1. Run PATH="/path/to/downloaded/protobuf/bin:$PATH" cargo doc --features protoc, to see it succeed

So this shows that it uses protobuf-src correctly. I haven't been able to confirm that the docs.rs metadata works correctly; I haven't been able to reproduce this inside a docs.rs docker container.

@wackywendell
Copy link
Contributor Author

I'm also going to see if I can add some CI for this feature once I open the PR, which should validate / ensure that the protoc feature continues to build hermetically. I'll move it from draft to 'Ready for Review' once that seems to work; in the meantime, having a draft PR means that the CI will run.

@wackywendell wackywendell marked this pull request as ready for review January 7, 2025 18:18
@wackywendell
Copy link
Contributor Author

OK, tests pass, this should be ready to review!

@mbrobbel mbrobbel changed the title Add protoc feature to build protoc from source build(deps,ci): add protoc feature to build protoc from source Jan 8, 2025
Copy link
Member

@mbrobbel mbrobbel left a comment

Choose a reason for hiding this comment

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

Thanks!

@mbrobbel mbrobbel merged commit aecff0e into substrait-io:main Jan 8, 2025
37 checks passed
@westonpace
Copy link
Member

This is very nice, thank you!

@wackywendell wackywendell deleted the protoc-feature branch January 8, 2025 15:46
@wackywendell
Copy link
Contributor Author

Thanks, @EpsilonPrime, @mbrobbel and @westonpace!

This was referenced Jan 8, 2025
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.

Docs failing to build
4 participants