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

Replace dependency on protoc with grpcio-tools #107

Merged
merged 1 commit into from
Jul 10, 2020

Conversation

abn
Copy link
Collaborator

@abn abn commented Jul 8, 2020

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 the compiler extra here, but this is not strictly required as currently this is required only for testing when generating test source.

@abn abn marked this pull request as ready for review July 8, 2020 10:57
@nat-n
Copy link
Collaborator

nat-n commented Jul 8, 2020

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.

@boukeversteegh
Copy link
Collaborator

boukeversteegh commented Jul 8, 2020

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!

Yes indeed! With this PR we only depend on it for testing, so I think it only needs to be in dev-dependencies.

pyproject.toml Outdated Show resolved Hide resolved
@abn
Copy link
Collaborator Author

abn commented Jul 8, 2020

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.
@abn abn requested a review from boukeversteegh July 8, 2020 22:23
@aselus-hub
Copy link

will it still work with protoc as a plugin w/o the grpcio dep with this change?
those of us w/ multilanguage build pipelines are very negatively affected by dependancy to grpcio-tools

@abn
Copy link
Collaborator Author

abn commented Jul 8, 2020

@aselus-hub no changes at all for users of protoc and this change does not impact functionality. This is done so that we can generate test files without having to install protobuf compiler and library out of band from the package's toolchain.

@boukeversteegh
Copy link
Collaborator

boukeversteegh commented Jul 9, 2020

will it still work with protoc as a plugin w/o the grpcio dep with this change?
those of us w/ multilanguage build pipelines are very negatively affected by dependancy to grpcio-tools

If i understand correctly, the dependency is only there for development. So if your pipeline is just installing betterproto (including betterproto[compile]), and not with dev depenendies included, grpcio-tools will not be installed.

plugin.py does not reference grpcio-tools, so when you are running protoc --..... with the plugin, you'll have nothing to do with grpcio.-tools.

@boukeversteegh
Copy link
Collaborator

Yeah, I wasn't sure about that. Will drop the dependency from the compiler extra. Should the extra be called "plugin" instead of compiler? :)

Perhaps that makes a bit more sense yes. From the beginning I was confused what the [compiler] package was for. It took me some time to realize this library is needed for both generating and for referencing the python files, and that some people might want to reduce their runtime dependencies by excluding the plugin part.

From that respect, betterproto[protoc-plugin] would be even more clear. At least I would understand that immediately.

This also exposes a problem I hadn't realized before. The plugin is actually installed, even without the [compile] extras. This is because the script that protoc needs is defined in the project.toml (protoc-gen-python_betterproto), but not as part of the extras. If you then run the plugin, you'll get missing modules if you're lucky, or a exit code 1, if you're unlucky, with no failure message from protoc.

I'm not sure if its possible to move the script section into the extras, besides creating two different builds/packages.

@abn
Copy link
Collaborator Author

abn commented Jul 9, 2020

From that respect, betterproto[protoc-plugin] would be even more clear. At least I would understand that immediately.

Maybe another PR? 2.0 might be a good version to make that change in.

I'm not sure if its possible to move the script section into the extras, besides creating two different builds/packages.

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.

@boukeversteegh
Copy link
Collaborator

From that respect, betterproto[protoc-plugin] would be even more clear. At least I would understand that immediately.

Maybe another PR? 2.0 might be a good version to make that change in.

Sure!

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.

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.

@boukeversteegh
Copy link
Collaborator

@aselus-hub do above answers take away your concerns? If yes, we can merge.

@aselus-hub
Copy link

yup no concerns from me thank you for the check @boukeversteegh

@boukeversteegh boukeversteegh merged commit 0321160 into danielgtaylor:master Jul 10, 2020
@abn abn deleted the grpcio-tools branch July 10, 2020 11:17
@abn abn mentioned this pull request Nov 24, 2020
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.

4 participants