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

Convert to Python 3 #11201

Closed
wants to merge 1 commit into from
Closed

Convert to Python 3 #11201

wants to merge 1 commit into from

Conversation

olekw
Copy link
Contributor

@olekw olekw commented Apr 23, 2020

Python 2 has reached end-of-life.
This change was imported from Debian packaging.

@aiuto
Copy link
Contributor

aiuto commented Apr 23, 2020

I am dubious about updating third_party except to pull down fresh releases or cherrypicks from from upstream. Unless this is scriptable from an update, it is going to introduce a maintenance burden.

We would be better off eliminating the dependency on gflags. A very quick grep on the source tree shows no use of it. I wonder if this package is vestigial from before conversion to absl flags?

@jin jin added the team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website label May 19, 2020
@philwo philwo added P1 I'll work on this now. (Assignee required) type: feature request labels May 19, 2020
@philwo philwo self-requested a review May 19, 2020 17:08
@@ -237,7 +237,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

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

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

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.

@olekw
Copy link
Contributor Author

olekw commented May 19, 2020

I am dubious about updating third_party except to pull down fresh releases or cherrypicks from from upstream. Unless this is scriptable from an update, it is going to introduce a maintenance burden.

That's a fair point. Regarding scripting, with the exception of BazelPythonSemantics.java, all the other changes were made by using sed (obviously, double-checked afterwards!) I used the whole "#! /usr/bin/env python" string to make sure I didn't accidentally catch something extraneous. That would probably be fairly trivial to script if you wanted to go that route. I searched and (at the time at least) did not find any other references for python vs python3.

@aiuto
Copy link
Contributor

aiuto commented May 21, 2020

I think we should drop this PR and replace it with 3 independent ones.

  • Drop the dependency on third_party/py/gflags. That is a large part of this PR and it is a bigger win
  • Convert out nanopb use to py3
  • Convert mock to py3

Circle back on which python gets invoked in a few months, after we see more migration to rules_python.

aiuto added a commit to aiuto/bazel that referenced this pull request Jun 8, 2020
bazel-io pushed a commit that referenced this pull request Jun 9, 2020
It is unused.

See #11201

Closes #11564.

PiperOrigin-RevId: 315438708
@aiuto
Copy link
Contributor

aiuto commented Jun 9, 2020

Hi Olek: #11564 removed //third_party/gflags, so much of this can go away.

@werkt
Copy link
Contributor

werkt commented Jul 6, 2020

@aiuto related to #11564

https://github.com/bazelbuild/rules_docker/blob/faaa10a72fa9abde070e2a20d6046e9f9b849e9a/container/BUILD#L47 (current master) is still referencing this package - this breaks rules_docker for bazel master.

@philwo
Copy link
Member

philwo commented Sep 23, 2020

What's the status of this PR?

@olekw
Copy link
Contributor Author

olekw commented Sep 28, 2020

What's the status of this PR?

Hi @philwo. I'm happy to refactor this in light of the other changes. As I mentioned in the meeting last week, I've been heavily occupied with things much less fun than working on FOSS. But if the consensus is that you'd like to merge (an updated version of) this PR then I can certainly fix it up in the next couple days.

@lberki lberki added team-Rules-Python Native rules for Python and removed team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website labels Oct 21, 2020
@lberki
Copy link
Contributor

lberki commented Oct 21, 2020

@brandjon , I'm inclined to merge this change as it is. WDYT? (then it'll need to be rebased, though)

@meisterT meisterT mentioned this pull request Oct 31, 2020
@aiuto
Copy link
Contributor

aiuto commented Jan 6, 2021

Is this still a work in progress?
third_party/py/gflags is no longer part of Bazel, so half of this PR can be deleted.

@aiuto
Copy link
Contributor

aiuto commented Jun 24, 2021

Friendly ping. This is very stale now. Much of it refers to deleted code.
@olekw Are you going to refresh or close this?

@oquenchil
Copy link
Contributor

@olekw If you update this PR, we can merge.

@olekw
Copy link
Contributor Author

olekw commented Jul 19, 2021

Thanks for the pings, @aiuto and @oquenchil! Day job has been keeping me busy but I can definitely update this PR. Will try to do that in the next couple days.

Python 2 has reached end-of-life.
This change was adapted from Debian packaging.
@olekw
Copy link
Contributor Author

olekw commented Jul 23, 2021

Ok, went through line by line and this (much smaller) PR is updated for HEAD.

I did notice that there is now a %shebang% in python_stub_template.txt. From what I can tell, this new code appears to Do The Right Thing but I'll leave it to whoever implemented that functionality to verify. :)

Update: See requested change conversation

@oquenchil oquenchil requested a review from fweikert July 26, 2021 10:23
@oquenchil
Copy link
Contributor

@fweikert Hi Florian, IIRC you have experience with Py2 to Py3 migration. Could you please answer olekw's question and take over merging this PR?

@fweikert
Copy link
Member

Happy to merge this PR once the discussion is resolved. However, I'm, not familiar with the particular problem.

@bazel-io bazel-io closed this in a5d2973 Aug 11, 2021
bazel-io pushed a commit that referenced this pull request Aug 23, 2021
This reflects the change in recent Linux distributions that no longer ship a /usr/bin/python binary.

I'm not sure where this is actually used, now that if I understand correctly the shebang defaults to the Python binary found via auto-detection, but we should probably still fix this path (or remove this default fallback completely?).

Related to #11201. Closes #13851.

PiperOrigin-RevId: 392427248
@kersson kersson mentioned this pull request Mar 17, 2022
apattidb pushed a commit to databricks/bazel that referenced this pull request May 27, 2022
This reflects the change in recent Linux distributions that no longer ship a /usr/bin/python binary.

I'm not sure where this is actually used, now that if I understand correctly the shebang defaults to the Python binary found via auto-detection, but we should probably still fix this path (or remove this default fallback completely?).

Related to bazelbuild#11201. Closes bazelbuild#13851.

PiperOrigin-RevId: 392427248
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes P1 I'll work on this now. (Assignee required) team-Rules-Python Native rules for Python type: feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.