src-d / lookout-sdk

SDK for lookout analyzers
Apache License 2.0
4 stars 11 forks source link

Make protogen could install grpcio_tools outside environment #75

Open se7entyse7en opened 5 years ago

se7entyse7en commented 5 years ago

In my specific case I'm using a python env built using conda and not venv. When I ran make protogen it actually installed grpcio_tools=1.13.0 in my global env as pip3 was pointing to the global pip. Given that inside an environment pip points to the "correct" pip2 or pip3 depending on the python version of the environment I'd suggest to replace pip3 with pip.

If we're concerned about the python version I'd add a check that tests it before generating the python files.

smacker commented 5 years ago

On most of systems this change would break code generation. Default python on macos (latest Mojave):

$ python --version
Python 2.7.15
$ pip --version
pip 10.0.1 from /usr/local/lib/python2.7/site-packages/pip (python 2.7)

I also know that many linux distributives still point on python v2 & pip for python v2 by default. Even if python3 is also installed.

se7entyse7en commented 5 years ago

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" pip.

se7entyse7en commented 5 years ago

The simplest thing would be to skip the installation of grpcio_tools and assume that it is installed by raising an error if it is not present. The user is then free to use any envs or the system.

smacker commented 5 years ago

I like the idea of using grpcio_tools from a user's environment. But we need to enforce version of it, so we might need to check not only existence but version too.

se7entyse7en commented 5 years ago

But we need to enforce version of it

Sure! 👍

dpordomingo commented 5 years ago

I'm not a python dev, so let me know if I understood your proposal:

$ make protogen

won't install grpcio_tools to avoid messing user env; what seems good to me. grpcio_tools==1.13.0 is still a dependency that should be satisfied before running make protogen

imo it should be checked by protogen make rule, and fail gracefully if it is not available in the user env, providing a clear hint about how to satisfy it, ie.

$ 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.