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

Update make_deb.py to work for Python2 or 3 #47

Merged
merged 6 commits into from
Jul 3, 2019
Merged

Conversation

aiuto
Copy link
Collaborator

@aiuto aiuto commented Jul 3, 2019

Generally being more careful about unicode handling.
Some very weird magic with pre-processing argv to undo undesired interpreter behavior.

This leaves the code in a state where someone could use either interpreter.
Next step it to figure out how to select the python version to match whatever toolchain we have.

@aiuto
Copy link
Collaborator Author

aiuto commented Jul 3, 2019

Mostly fixes #35
@qzmfranklin, @jwnimmer-tri Do you want to patch this in, change the target to PY3 and see if it works for you?

@aiuto aiuto requested a review from brandjon July 3, 2019 13:58
@brandjon
Copy link
Member

brandjon commented Jul 3, 2019

Next step it to figure out how to select the python version to match whatever toolchain we have.

bazelbuild/bazel#8713. I think the easiest thing is to make it python_version = "PY3" since that's preferred, but keep it compatible with PY2 (enforced via a python_version = "PY2" py_test). In the host config you don't control its real Python version anyway.

If a user doesn't have Python 3 installed but wants to run targets marked PY3 that are in reality compatible with both Python 2 and 3, they should use the non-strict toolchain (implemented in bazelbuild/bazel#8547).

Copy link
Member

@brandjon brandjon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not super familiar with this code but looks plausible.

pkg/make_deb.py Show resolved Hide resolved
This way we can *not* use it for args of the form '@filename'.
The filename portion will already be encoded in the proper format
for the open call.

And add a python2 test.
@aiuto
Copy link
Collaborator Author

aiuto commented Jul 3, 2019

Also changed the default version to py3 and added a py2 test.

pkg/BUILD Outdated Show resolved Hide resolved
pkg/make_deb.py Outdated Show resolved Hide resolved
pkg/make_deb.py Show resolved Hide resolved
@aiuto
Copy link
Collaborator Author

aiuto commented Jul 3, 2019

Mostly fixes #35
@qzmfranklin, @jwnimmer-tri Do you want to patch this in, change the target to PY3 and see if it works for you?

I added changed to PY3 as the default and added a py2 test, so this is less important. I think I will merge and we'll see how that goes.

@aiuto aiuto merged commit 96e34ab into bazelbuild:master Jul 3, 2019
@aiuto aiuto deleted the py3 branch July 3, 2019 21:17
@jwnimmer-tri
Copy link

Thanks for your work on this! Apologies for the slow reply, I have been on vacation.

I tested the latest master here 96e34ab against my tree, and I can confirm that the decode()-related problems I saw in bazelbuild/bazel/issues/7370 are resolved.

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.

3 participants