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

Tweak Python semantics for Python 3 and OpenBSD. #10432

Closed
wants to merge 4 commits into from
Closed

Tweak Python semantics for Python 3 and OpenBSD. #10432

wants to merge 4 commits into from

Conversation

aldersondrive
Copy link
Contributor

@aldersondrive aldersondrive commented Dec 17, 2019

As discussed at #10432 (comment), Bazel is already switching to assume that the Python binary is called python3, not python.

This change is also useful for the OpenBSD port in #10250. (On OpenBSD, the python executable is called python3, and no python binary exists.)

@jin jin added the team-Rules-Server Issues for serverside rules included with Bazel label Dec 20, 2019
@benjaminp
Copy link
Collaborator

Maybe you can just unconditionally use env python3? It's been announced that Bazel assumes Python 3 now. It would be in the spirit of 29d4500.

@aldersondrive
Copy link
Contributor Author

aldersondrive commented Jan 7, 2020

Maybe you can just unconditionally use env python3? It's been announced that Bazel assumes Python 3 now. It would be in the spirit of 29d4500.

That would work for OpenBSD, but would also change behavior on all platforms. Notably, FreeBSD has no python3 binary, so that change would break Bazel on FreeBSD. (On FreeBSD the Python binary is called python3.6, python3.7, etc..)

On the other hand, if Bazel is going to assume the existence of a python3 binary anyway (per 29d4500), then what you describe is okay because the FreeBSD port needs to change in this respect anyway.

How would you like to proceed? I'm fine with whichever way you folks think best. I just hesitate to push a PR that changes behavior on all platforms and breaks one of them without confirmation from Bazel team that doing so is really the right way to go.

@aldersondrive
Copy link
Contributor Author

How would you like to proceed?

Update: I checked a clean FreeBSD system, and it does have a python3 in the PATH. Maybe there was something wrong with the other FreeBSD system I checked earlier.

In any case, it looks like Bazel already assumes the existence of python3, and apparently python3 does exist on FreeBSD after all, so I've gone ahead and made the change you suggested. PTAL.

@aldersondrive aldersondrive changed the title Tweak Python semantics for OpenBSD. Tweak Python semantics for Python3 and OpenBSD. Jan 8, 2020
@aldersondrive aldersondrive changed the title Tweak Python semantics for Python3 and OpenBSD. Tweak Python semantics for Python 3 and OpenBSD. Jan 8, 2020
@brandjon
Copy link
Member

brandjon commented Feb 3, 2020

The issue with putting python3 in the shebang is that 1) it's an incompatible change that 2) affects all py_binary targets. In contrast, 29d4500 only affects a test used in the development of Bazel itself. While we require Python 3 for Bazel development and for certain baked-into-bazel tools that users may depend on, we do not require it for arbitrary user py_binary targets.

For supporting this use case, I think we'd need an attribute on the Python toolchain to set the shebang to use in the stub script.

See #8685 for additional commentary along these lines. I think we can dedup this against that one.

@aldersondrive
Copy link
Contributor Author

Would you be okay with the first version of this PR?

It's not an incompatible change, because it affects behavior only on OpenBSD, where Bazel support is still under development.

#8685 sounds like a much larger and more complex change that likely won't be done anytime soon.

@brandjon
Copy link
Member

brandjon commented Feb 4, 2020

I'm hesitant to add additional ad hoc logic to the native rules. That said, given that

  1. this only affects a platform that isn't already fully supported,
  2. the new support is experimental, and
  3. we can migrate to Allow customizing Python stub script's shebang in the Python toolchain #8685 by adding a toolchain that mimics the proposed OS.getCurrent() check,

it may be ok. @katre, do you see any platform-related concerns with a migration path from this native check to a future built-into-bazel platform definition for OpenBSD? (And can you confirm our support status for OpenBSD?)

@katre
Copy link
Member

katre commented Feb 4, 2020

I am fine with this change, especially if we are moving towards having the python binary in the toolchain (so we can remove it). I'd like to see a TODO to remove the special casing in the future (possibly referencing #8685).

Yes, we want to support OpenBSD and if there aren't existing constraints for it, I'd support a PR to add them.

@brandjon
Copy link
Member

brandjon commented Feb 4, 2020

Ok, sounds good. @aldersondrive, can you please revert to the first version and add a TODO referencing #8685? (Standard format is // TODO(#8685): blah blah blah)

@brandjon
Copy link
Member

brandjon commented Feb 4, 2020

Should be merged tomorrow.

@bazel-io bazel-io closed this in 15d80dc Feb 5, 2020
luca-digrazia pushed a commit to luca-digrazia/DatasetCommitsDiffSearch that referenced this pull request Sep 4, 2022
    As discussed at bazelbuild/bazel#10432 (comment), Bazel is already switching to assume that the Python binary is called `python3`, not `python`.

    This change is also useful for the OpenBSD port in bazelbuild/bazel#10250. (On OpenBSD, the python executable is called `python3`, and no `python` binary exists.)

    Closes #10432.

    PiperOrigin-RevId: 293351316
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes team-Rules-Server Issues for serverside rules included with Bazel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants