-
Notifications
You must be signed in to change notification settings - Fork 11
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
Make protogen could install grpcio_tools outside environment #75
Comments
On most of systems this change would break code generation.
I also know that many linux distributives still point on python v2 & pip for python v2 by default. Even if python3 is also installed. |
Ok, and obviously we cannot just break code generation. I think that the first question is whether we want to support conda environments, otherwise, we just have to add to the documentation that we only support virtual envs. I haven't tested it, but I guess that it works as expected with an activated virtual env. In case we want to support conda envs, we should find a way to properly use the "right" |
The simplest thing would be to skip the installation of |
I like the idea of using |
Sure! 👍 |
I'm not a python dev, so let me know if I understood your proposal: $ make protogen won't install imo it should be checked by $ make protogen
Please install correct 'grpcio_tools' version 1.2.3. You can install it by typing:
pip3 install grpcio_tools==1.2.3
If you use non standard python environment the command might be different.
Makefile:25: *** "grpcio_tools==1.13.0 was not found". Stop. |
In my specific case I'm using a python env built using conda and not venv. When I ran
make protogen
it actually installedgrpcio_tools=1.13.0
in my global env aspip3
was pointing to the globalpip
. Given that inside an environmentpip
points to the "correct"pip2
orpip3
depending on the python version of the environment I'd suggest to replacepip3
withpip
.If we're concerned about the python version I'd add a check that tests it before generating the python files.
The text was updated successfully, but these errors were encountered: