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
[BEAM-3065] Avoids generating proto files for Windows if grpcio-tools is not installed. #4007
[BEAM-3065] Avoids generating proto files for Windows if grpcio-tools is not installed. #4007
Conversation
R: @robertwb |
sdks/python/gen_protos.py
Outdated
@@ -76,6 +77,14 @@ def generate_proto_files(force=False): | |||
try: | |||
from grpc_tools import protoc | |||
except ImportError: | |||
if platform.system() == 'Windows': | |||
# For Windows, grpcio-tools has to be installed manually. | |||
logging.warning( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Raise this as an exception. Otherwise the build will "succeed" and they'll get much harder to diagnose errors later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. PTAL.
sdks/python/gen_protos.py
Outdated
@@ -76,6 +77,14 @@ def generate_proto_files(force=False): | |||
try: | |||
from grpc_tools import protoc | |||
except ImportError: | |||
if platform.system() == 'Windows': | |||
# For Windows, grpcio-tools has to be installed manually. | |||
logging.warning( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
robertwb wrote:
Raise this as an exception. Otherwise the build will "succeed" and they'll get much harder to diagnose errors later.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, modulo one small change.
sdks/python/gen_protos.py
Outdated
'Cannot generate protos for Windows since grpcio-tools package is ' | ||
'not installed. Please install this package manually ' | ||
'using \'pip install grpcio-tools\'.') | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this redundant return.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
sdks/python/gen_protos.py
Outdated
'Cannot generate protos for Windows since grpcio-tools package is ' | ||
'not installed. Please install this package manually ' | ||
'using \'pip install grpcio-tools\'.') | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
robertwb wrote:
Remove this redundant return.
Done.
Run Python PreCommit |
2 similar comments
Run Python PreCommit |
Run Python PreCommit |
Follow this checklist to help us incorporate your contribution quickly and easily:
[BEAM-XXX] Fixes bug in ApproximateQuantiles
, where you replaceBEAM-XXX
with the appropriate JIRA issue.mvn clean verify
to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.