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

Python 2 tools in exec configuration don't work in Bazel 6.x #16935

Closed
eric-skydio opened this issue Dec 6, 2022 · 10 comments
Closed

Python 2 tools in exec configuration don't work in Bazel 6.x #16935

eric-skydio opened this issue Dec 6, 2022 · 10 comments
Labels

Comments

@eric-skydio
Copy link

Description of the bug:

When depending on a py_binary tool in exec configuration that uses python_version to specify "PY2", it somehow still tries to run in PY3 configuration and causes a build failure. This issue is not present in Bazel 5.3.0.

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

  1. Create empty files WORKSPACE and test.py

  2. Put the following content in BUILD

genrule(
    name = "gen",
    outs = ["out.txt"],
    cmd = "echo hi > $@",
    exec_tools = [":test"],
)

py_binary(
    name = "test",
    srcs = ["test.py"],
    python_version = "PY2",
    srcs_version = "PY2ONLY",
)
  1. Run bazel build //:gen
ERROR: /tmp/bazeltest/BUILD:24:10: Reporting failed target //:test located at /tmp/bazeltest/BUILD:24:10 [for tool] failed: //:test: This target is being built for Python 3 but (transitively) includes Python 2-only sources. You can get diagnostic information about which dependencies introduce this version requirement by running the `find_requirements` aspect. For more info see the documentation for the `srcs_version` attribute: https://bazel.build/reference/be/python#py_binary.srcs_version caused by //:test
Target //:gen failed to build

Note that bazel run //:test works just fine since it doesn't involve an exec configuration, and inserting an sh_binary rule in between the genrule and py_binary reveals that the issue is the failure of py_binary to apply an incoming edge transition (which it does apply correctly with Bazel 5.x).

Which operating system are you running Bazel on?

No response

What is the output of bazel info release?

release 6.0.0rc4

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

No response

What's the output of git remote get-url origin; git rev-parse master; git rev-parse HEAD ?

No response

Have you found anything relevant by searching the web?

No response

Any other information, logs, or outputs that you want to share?

I'm pretty surprised that this issue leaked through this close to 6.x releasing, so it's possible I'm missing something obvious or misunderstanding how this is supposed to work.

Tagging @fmeum and @gregestren in case this is caused by other configuration transition changes in the 6.x window.

@eric-skydio
Copy link
Author

This is definitely a blocking issue for us using Bazel 6.0, and I'm suspicious it represents something more fundamental broken in the configuration system. In case it's useful, I've attached screenshots of the cquery graph here. These are for the three-target version where the genrule exec-depends on a sh_binary which data-depends on a py_binary

With bazel 5.x, we see two transitions, one to exec configuration and a second to PY2
bazel5

With bazel 6.0.0rc4 we see only one transition

bazel6

@fmeum
Copy link
Collaborator

fmeum commented Dec 6, 2022

@comius for d988e8b, which a bisect points to.

@meteorcloudy Looks like a regression.

@eric-skydio
Copy link
Author

eric-skydio commented Dec 6, 2022

If it's useful, we originally saw this regression in our custom rules that use cfg="exec", my use of genrules in the example is just to make the reproducing example simpler.

@fmeum
Copy link
Collaborator

fmeum commented Dec 7, 2022

@bazel-io flag

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Dec 7, 2022
@meteorcloudy
Copy link
Member

@rickeylev @katre

@comius
Copy link
Contributor

comius commented Dec 7, 2022

This is caused by ordering of transitions, which happens in the following order:

  1. exec (set python version from flags)
  2. python (set python version from attributes)
  3. host (set python version from flags)

With Bazel 6.0.0 the host transition is gone, made identical to exec.

There are two options, how to handle python transition:

  1. Use config flags when in exec configuration and use target attributes when in target configuration (this is the current implementation)
  2. Always use the attributes on the target

I decided for #1, because I thought it breaks less people. I assumed that most people were only using host transition in Bazel 5.x. Native rules were mostly using host. And in this case the behaviour is the same.

My blind spot was exec_tools in genrule and potential custom rules with exec. When using exec this becomes a change in behaviour.

We can change the implementation to option #2. However, this will also change the behavior where people are still only using host configuration (you can use the same example, except s/exec_tools/tools/ in genrule).

If we stick with #1, you might be able to mitigate by setting config flags.
If we change to #2, people that rely on config flags will have to set rule attributes now.

There might be other good arguments for #2. Code would be simpler, more idiomatic and it gives users per target control. (But it will also potentially break somebody).

Maybe this issue is on its own a good argument to stop using PY2.

@fmeum
Copy link
Collaborator

fmeum commented Dec 7, 2022

We can change the implementation to option #2. However, this will also change the behavior where people are still only using host configuration (you can use the same example, except s/exec_tools/tools/ in genrule).

@comius Didn't you also make tools a synonym of exec_tools in d988e8b#diff-ad644c43228864143146c16221e85e5197a8e72712a1dfdb937363baf1c10d06R44?

@comius
Copy link
Contributor

comius commented Dec 7, 2022

@comius Didn't you also make tools a synonym of exec_tools in d988e8b#diff-ad644c43228864143146c16221e85e5197a8e72712a1dfdb937363baf1c10d06R44?

Yes, there's no host anymore. And exec_tools is slated for removal in favour of tools.

comius added a commit to comius/bazel that referenced this issue Dec 7, 2022
Fixes: bazelbuild#16935

RELNOTES[INC]: This changes the behavior of Python version in exec/host configuration. Mitigation is to set Python version on the targets.

PiperOrigin-RevId: 493545843
Change-Id: I3a4d787e7075d2b76835faf04d4c4e04c9de85b4
@meteorcloudy
Copy link
Member

@bazel-io fork 6.0.0

@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Dec 7, 2022
comius added a commit to comius/bazel that referenced this issue Dec 7, 2022
Fixes: bazelbuild#16935

RELNOTES[INC]: This changes the behavior of Python version in exec/host configuration. Mitigation is to set Python version on the targets.

PiperOrigin-RevId: 493545843
Change-Id: I3a4d787e7075d2b76835faf04d4c4e04c9de85b4
@rickeylev
Copy link
Contributor

py_binary tool that uses python_version to specify "PY2"

Lets continue any py2-specific discussion in #15684, but you really really need to move off Python 2

comius added a commit to comius/bazel that referenced this issue Dec 8, 2022
Fixes: bazelbuild#16935

RELNOTES[INC]: This changes the behavior of Python version in exec/host configuration. Mitigation is to set Python version on the targets.

PiperOrigin-RevId: 493545843
Change-Id: I3a4d787e7075d2b76835faf04d4c4e04c9de85b4
Wyverald pushed a commit that referenced this issue Dec 8, 2022
Fixes: #16935

RELNOTES[INC]: This changes the behavior of Python version in exec/host configuration. Mitigation is to set Python version on the targets.

PiperOrigin-RevId: 493804390
Change-Id: I3a4d787e7075d2b76835faf04d4c4e04c9de85b4
Wyverald added a commit that referenced this issue Dec 8, 2022
Fixes: #16935

RELNOTES[INC]: This changes the behavior of Python version in exec/host configuration. Mitigation is to set Python version on the targets.

PiperOrigin-RevId: 493804390
Change-Id: I3a4d787e7075d2b76835faf04d4c4e04c9de85b4

Co-authored-by: Googler <ilist@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants