-
Notifications
You must be signed in to change notification settings - Fork 219
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
Replace dependency on protoc with grpcio-tools #107
Conversation
Nice. I didn't know about grcp-tools. If we start using betterproto models in the compiler (which we really should) then maybe we don't need to depend on protobuf at all! Unless the plugin actually uses grpc-tools I don't think it should be listed as a dependency. The user can install it if they want but shouldn't be forced to unnecessarily. |
Yes indeed! With this PR we only depend on it for testing, so I think it only needs to be in dev-dependencies. |
Yeah, I wasn't sure about that. Will drop the dependency from the compiler extra. Should the extra be called "plugin" instead of compiler? :) |
This change removes the dependency on platform provided protobuf tools in favour of `grpcio-tools` dependency. This makes both development and compiler use independent from platform dependencies.
will it still work with protoc as a plugin w/o the grpcio dep with this change? |
@aselus-hub no changes at all for users of |
If i understand correctly, the dependency is only there for development. So if your pipeline is just installing
|
Perhaps that makes a bit more sense yes. From the beginning I was confused what the From that respect, This also exposes a problem I hadn't realized before. The plugin is actually installed, even without the I'm not sure if its possible to move the script section into the extras, besides creating two different builds/packages. |
Maybe another PR? 2.0 might be a good version to make that change in.
At the moment no. Although, this is part of an improvement coming in a future poetry version. We can mitigate this issue by printing out some useful error message like "please install compiler extras using pip install betterproto[compiler]" when an import error occurs and calling exit. I can create another PR for this. |
Sure!
Actually we have that, oops. But I remember the message doesn't show nicely if you run protoc with the plugin. Something to retest, now i'm not sure. |
@aselus-hub do above answers take away your concerns? If yes, we can merge. |
yup no concerns from me thank you for the check @boukeversteegh |
This change removes the dependency on platform provided protobuf tools in favour of
grpcio-tools
dependency. This makes both development and compiler use independent from platform dependencies.This also could potentially allow for a
betterproto
cli tool. I have added the dependency to thecompiler
extra here, but this is not strictly required as currently this is required only for testing when generating test source.