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.

Sign up for GitHub

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

Convert to Python 3 #11201

Closed
wants to merge 1 commit into from
Closed

Convert to Python 3 #11201

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ public void createExecutable(
* logic will extract the zip's runfiles into a temporary directory.
*
* The stub script has a shebang pointing to a first-stage Python interpreter (as of this
* writing "#!/usr/bin/env python"). When a zip file is built on unix, this shebang is also
* writing "#!/usr/bin/env python3"). When a zip file is built on unix, this shebang is also
* prepended to the final zip artifact. On Windows shebangs are ignored, and the launcher
* runs the first stage with an interpreter whose path is passed in as LaunchInfo.
*/
Expand Down Expand Up @@ -240,7 +240,7 @@ public void createExecutable(
PathFragment shExecutable = ShToolchain.getPathOrError(ruleContext);
// TODO(#8685): Remove this special-case handling as part of making the proper shebang a
// property of the Python toolchain configuration.
String pythonExecutableName = OS.getCurrent() == OS.OPENBSD ? "python3" : "python";
String pythonExecutableName = "python3";
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this break anyone who is still using Python3?

cc: @brandjon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aiuto assuming you mean Python 2 then, yes. If a user has only Python 2 installed and not Python 3 then this would fail for them. However, I'm not sure how many users, even those who use Python 2 as default, don't have Python 3 installed at all. (Or couldn't easily install it) Debian ships it even in our oldoldstable (which is really obsolete).

Copy link
Member

Choose a reason for hiding this comment

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

FYI macOS before Catalina only had python2 installed by default, so there is no global /usr/bin/python3, some users might have that installed through homebrew at /usr/local/bin/python3, but it's not a requirement today and arguably relying on brewed python is an issue since users could have any sub-version there depending on when they installed it

Copy link
Member

Choose a reason for hiding this comment

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

We can't require that Python3 be installed on all target platforms where Python 2 targets run.

The proper solution is probably to review #11434 and make the autodetecting toolchain for Python 3 emit a python3 shebang. The only case I can think of where this would be a breaking change is where there's no python3 command in the environment, but there is a python command that resolve to a Python 3 interpreter. In which case I'm tempted to just say add an explicit toolchain to your build.

Copy link
Member

Choose a reason for hiding this comment

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

This code path only affects the disabled-by-default "launchable Python zips" in the deprecated native Python rules of Bazel. For example, if you build something like this:

philwo@philwo-macbookpro2:src/pytest $ cat BUILD 
py_binary(
    name = "test",
    srcs = ["test.py"],
)

philwo@philwo-macbookpro2:src/pytest $ cat test.py 
print("Hello world!")

philwo@philwo-macbookpro2:src/pytest $ bazel build --build_python_zip :test
Target //:test up-to-date:
  bazel-bin/test
  bazel-bin/test.zip

Then the bazel-bin/test would be a ZIP file with the shebang prepended, so you can just run it as an executable, but only if you have a python3 executable in your path. Without this change you can only run it if you have a python executable in your path.

The python executable on the path means "Python 2" on almost all distros (only Arch Linux was bold enough to migrate that to Python 3 :)). So currently we require everyone to install an outdated, EOL'd version of Python to run these binaries, even if their own code is Python 3, as it should be.

With the change, we no longer require Python 2.x installed, but Python 3. IMHO this is fine, especially considering https://python3statement.org/.

Copy link
Member

@philwo philwo May 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

In the meantime I think allowing configuring the shebang should suffice.

Yes, that sounds like a nice way. We could let it default to the current python one and introduce an incompatible flag that flips it to python3, WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Admittedly we wrote there that we'd still support building Python 2.x code. :/

Which was a very intentional policy point, not an oversight. Google isn't even free of Python 2 code, so we can't expect all our users to be. (Of course, we're talking about the ability to build and run Python 2 targets, which isn't the same thing as requiring Python 3 to also be present.)

What would dropping Python 2 support look like in your ideal vision? Would we no longer have a python_version attribute that chose between PY2 and PY3? To drop support of the whole concept of running PY2 code from the Python rules, I'd want to see virtually no users having any PY2 code in their builds. Given that only last year we changed the default and it broke people, I think we have some time before that happens. Perhaps enough that we'll first migrate the rules entirely to Starlark and it'll be a different calculus (e.g. if the version selection system gets more granularity).

We could let it default to the current python one and introduce an incompatible flag that flips it to python3, WDYT?

If people are not setting up an explicit Python toolchain in their build, then they're using the autodetecting toolchain, which will look for Python 2 or Python 3 depending on the configuration of the target. It's easy for these toolchains to specify custom shebangs that use python and python3 respectively. (Of course, the python command on Arch will yield a Python 3 interpreter, but the stub script doesn't care about the version so long as it's at least present.)

If people are setting up a custom toolchain, they can modify their toolchain definition to explicitly specify a shebang.

For the default value, of a toolchain that is not the autodetecting one, we can probably use an incompatible flag to nudge people along. But this would be for convenience; it doesn't block anything.

Copy link
Contributor Author

@olekw olekw Jul 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, just looked at #11434 and I can now understand the %shebang% functionality better. Is there a consensus here to change the default to Python 3 in that mechanism as well? I would personally recommend that but I'll hold off on adding that to this PR lacking said consensus.
@aiuto @philwo @brandjon @oquenchil

Copy link
Member

Choose a reason for hiding this comment

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

@olekw Yes, let's do that and change the default there to python3.

Copy link
Member

@philwo philwo Aug 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I'll import this and send it for internal review, then we can do a follow-up change on that line.

// NOTE: keep the following line intact to support nix builds
String pythonShebang = "#!/usr/bin/env " + pythonExecutableName;
ruleContext.registerAction(
Expand Down
4 changes: 2 additions & 2 deletions src/test/py/bazel/launcher_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,7 +355,7 @@ def testPyBinaryLauncher(self):
'helloworld(', ' name = "hello",', ' out = "hello.txt",', ')'
])
foo_py = self.ScratchFile('foo/foo.py', [
'#!/usr/bin/env python',
'#!/usr/bin/env python3',
'import sys',
'if len(sys.argv) == 2:',
' with open(sys.argv[1], "w") as f:',
Expand All @@ -364,7 +364,7 @@ def testPyBinaryLauncher(self):
' print("Hello World!")',
])
test_py = self.ScratchFile('foo/test.py', [
'#!/usr/bin/env python',
'#!/usr/bin/env python3',
'import unittest',
'class MyTest(unittest.TestCase):',
' def test_dummy(self):',
Expand Down
2 changes: 1 addition & 1 deletion third_party/py/mock/setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#! /usr/bin/env python
#! /usr/bin/env python3

# Copyright (C) 2007-2012 Michael Foord & the mock team
# E-mail: fuzzyman AT voidspace DOT org DOT uk
Expand Down