-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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-3142] Apply futurize on gen_protos #4077
[BEAM-3142] Apply futurize on gen_protos #4077
Conversation
cc @aaltay any idea why this would fail here (or how to get the log file out of jenkins)? It hasn't even it the point of changes when it fails to generate. |
SUCCESS --none-- |
2 similar comments
SUCCESS --none-- |
SUCCESS --none-- |
Hey @holdenk, sorry I missed the question earlier. Do you still have the issue? |
No, its working now I did some poking in another PR to get the logs sorted out. |
sdks/python/gen_protos.py
Outdated
@@ -111,6 +111,15 @@ def generate_proto_files(force=False): | |||
'%s' % ret_code) | |||
|
|||
|
|||
if sys.version_info[0] >= 3: | |||
ret_code = subprocess.call( | |||
["futurize", "-0", "-wv", "--no-diff", out_dir]) |
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.
Could you expand the arguments to their long form: futurize --both-stages --write --verbose --no-diff
?
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.
sure
d967dc6
to
0a1737a
Compare
ping? would be nice to get this in. |
LGTM. Should we use futurize for Python 2 as well and remove the branch? |
Is this sufficient? Or should we be generating (and somehow selectively choosing) proto files generated targeting Py3? |
Futurizing for Python2 would require futurize to be installed by Python2 users so I'd say no. |
This PR has been open for a while, I will merge it. If the changes are not sufficient we can revisit. |
Python 3 removes relative import support, we can apply futurize on the generated protos to fix the imports. It's less than ideal, but it works.