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

Add protoc plugin handling to the prost-build Config and basic plugin #313

Closed
wants to merge 1 commit into from

Conversation

dfreese
Copy link
Contributor

@dfreese dfreese commented May 5, 2020

This refactors some of the private logic within prost-build's lib.rs so
that it is compatible with generating the logic necessary to run a
plugin for protoc or a build.rs file. In both cases, the output can be
changed based on how the Config is built. This adds the capability for
paramters passed in via protoc to a plugin to allow adjust this
configuration, but the mechanics of that are left as a TODO until the
design is solidified.

This based on #312 to take care of rustfmt issues and #313 for some CI issues.

prost-build/src/lib.rs Outdated Show resolved Hide resolved
prost-build/src/lib.rs Outdated Show resolved Hide resolved
prost-build/src/lib.rs Outdated Show resolved Hide resolved
This refactors some of the private logic within prost-build's lib.rs so
that it is compatible with generating the logic necessary to run a
plugin for protoc or a build.rs file.  In both cases, the output can be
changed based on how the Config is built.  This adds the capability for
paramters passed in via protoc to a plugin to allow adjust this
configuration, but the mechanics of that are left as a TODO until the
design is solidified.
@hrvolapeter
Copy link

Hi @dfreese what's the current status of this PR, is there maybe something I could help with?

@dfreese
Copy link
Contributor Author

dfreese commented Jul 13, 2020

@hrvolapeter I ended up developing a different approach that used prost-build directly, rather than needing this. It wasn't ideal, as it prost-build needs to write to a file.

From that experience, I don't think this is the best approach. Ideally, more information about the code generation from prost-build would be exposed, and this could be built on top of it. I have something locally that I can look at trying to wrap up and put up as a PR.

I'll start taking a look if we can open source what we ended up doing for a Bazel/Prost/Tonic integration.

@dfreese
Copy link
Contributor Author

dfreese commented Jul 13, 2020

@hrvolapeter the approach I was hoping to take is my generate branch here:
dfreese@ab69bf0

The TODO there is the main thing I thing I was hoping add before submitting it as a PR, otherwise I end up needing to re-implement a lot of the internal logic of prost-build. Let me know if you'd want to help with that approach, or want further clarification on why I was going that route.

@samschlegel
Copy link

I'll start taking a look if we can open source what we ended up doing for a Bazel/Prost/Tonic integration.

Definitely interested in this as I'm about to go down this path myself 😄

@epigramengineer
Copy link

@dfreese @samschlegel did either of you make (publicly available) progress on this? Attempting to do the same thing myself and hate reinventing the wheel.

@dfreese
Copy link
Contributor Author

dfreese commented Jan 13, 2021

@dfreese @samschlegel did either of you make (publicly available) progress on this? Attempting to do the same thing myself and hate reinventing the wheel.

Forgot to come back here, but I put up an initial attempt to export what we use here:
bazelbuild/rules_rust#479

@dfreese
Copy link
Contributor Author

dfreese commented Feb 26, 2021

Closing this for now. I don't think the plugin approach is worthwhile given the functionality to create more useful binaries directly in rust.

That being said, the extern_fields logic is difficult to get right unless you know exactly how prost goes about it's naming of things. If those could be exposed as public, that would be a huge help. I can take a look at if there's a clear way to do that.

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.

5 participants