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

Builtin python rules have toolchain error at HEAD #20458

Closed
brentleyjones opened this issue Dec 6, 2023 · 17 comments
Closed

Builtin python rules have toolchain error at HEAD #20458

brentleyjones opened this issue Dec 6, 2023 · 17 comments
Labels

Comments

@brentleyjones
Copy link
Contributor

Description of the bug:

Since 48eb023, using py_test without using rules_python, results in a toolchain error:

ERROR: /Users/brentley/Developer/rules_xcodeproj/tools/params_processors/BUILD:55:8: While resolving toolchains for target //tools/params_processors:swift_debug_settings_processor_tests (ee6eec1): No matching toolchains found for types @@bazel_tools//tools/python:toolchain_type.

Which category does this issue belong to?

Python Rules

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

git checkout https://github.com/MobileNativeFoundation/rules_xcodeproj.git
cd rules_xcodeproj
USE_BAZEL_VERSION=last_green bazelisk build --nobuild //tools/params_processors:swift_debug_settings_processor_tests

Which operating system are you running Bazel on?

macOS 14.1.1

What is the output of bazel info release?

No response

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

Is this a regression? If yes, please try to identify the Bazel commit where the bug was introduced.

48eb023

Have you found anything relevant by searching the web?

#20315

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

No response

@Wyverald
Copy link
Member

Wyverald commented Dec 6, 2023

cc @rickeylev

@rickeylev
Copy link
Contributor

On the user side, you can workaround it in the meantime by...

  • Add register_toolchains("@bazel_tools//tools/python:autodetecting_toolchain") to your MODULE.bazel
  • Specifying the above with --extra_toolchains.
  • Add a dep on rules_python >= 0.23.0. Note this will register a toolchain by default, but it will be a hermetic one, not the python-from-path ("autodetecting") one.

I'll try adding that register call in Bazel's (or bazel_tools?) MODULE.bazel, but I'm not sure that'll ultimately work out. The issue is it would fix this case, but wouldn't that toolchain have precedence over anything rules_python registers, because it would higher in the module dependency graph? I'm not sure where Bazel/bazel_tools priority lands if it registers a toolchain. In the workspace model, it somehow made sure it was always lowest priority.

@Wyverald
Copy link
Member

Wyverald commented Dec 6, 2023

I'll try adding that register call in Bazel's (or bazel_tools?) MODULE.bazel, but I'm not sure that'll ultimately work out. The issue is it would fix this case, but wouldn't that toolchain have precedence over anything rules_python registers, because it would higher in the module dependency graph? I'm not sure where Bazel/bazel_tools priority lands if it registers a toolchain. In the workspace model, it somehow made sure it was always lowest priority.

It goes by BFS in the module dependency graph, and every module in the graph has a (low-priority) implicit dependency on bazel_tools. So if the root module doesn't depend on rules_python, then bazel_tools has higher priority than rules_python; if the root module does depend on rules_python, then bazel_tools has lower priority.

@brentleyjones
Copy link
Contributor Author

What are the steps for non-bzlmod, if any? Thinking of what rules_xcodeproj has to do for its WORKSPACE users...

@Wyverald
Copy link
Member

Wyverald commented Dec 6, 2023

They could add the toolchain registration to the end of their WORKSPACE file. Or if rules_xcodeproj has a WORKSPACE macro setup that can make sure something happens after the default rules_python setup stuff, that should also be fine.

@brentleyjones
Copy link
Contributor Author

Thinking more on this, I think this needs to be fixed, or the python rules shouldn't be exposed from Bazel anymore. If I have to add a dep on rules_python to use the python rules, then we should just make people use those rules, right?

@fmeum
Copy link
Collaborator

fmeum commented Dec 12, 2023

I am with Brentley here. The only reasonable long-term solution seems to be to update the rules_python dep in MODULE.tools to get the hermetic Python toolchain registered automatically, which is exactly what users will get anyway when they manually depend on the latest version.

@rickeylev
Copy link
Contributor

or the python rules shouldn't be exposed from Bazel anymore. If I have to add a dep on rules_python to use the python rules, then we should just make people use those rules, right?

Yep, that's the plan! I'm planning to remove them as part of Bazel 8. So yes, if you have Python targets, you'll need to depend on rules_python one way or another.

In the meantime, I'll try updating the MODULE file to register @bazel_tools//tools/python:autodetecting_toolchain to preserve the previous behavior.

@rickeylev
Copy link
Contributor

What are the steps for non-bzlmod, if any?

Sorry, I forgot about this question. I don't think workspace builds have to do anything (for now). Under the hood, as part of the Bazel startup code (where Java code registers the Python rules as usable thing), an embedded WORKSPACE file is executed to register the autodetecting toolchain target. While writing a test for this, I had to go out of my way to make sure workspace was empty and ignored (using workspace.bzlmod). I'm not certain, but it seems like if workspace is triggered in any way, then that startup code registers the toolchain.

I say "for now", because I was working on a change to also stop Bazel itself from automatically registering @bazel_tools//tools/python:autodetecting_toolchain for workspace builds. It got caught up in a lot of test failures and there's more pre-cleanup I need to do before that can land. But, the take away is similar to what I said before: if you want to reliably have Python rules, go through rules_python and don't rely on the builtin bazel ones. The --incompatible_python_disallow_native_rules flag can help (but, due to #17773 that flag is effectively broken, so you'll have to wait for a patch release to fix that).

rickeylev added a commit to rickeylev/bazel that referenced this issue Dec 12, 2023
A regression from upgrading to rules_python 0.22.0 is that a Python
toolchain is no longer registered by default (the prior 0.4.0 version
registered the "autodetecting" toolchain). Subsequent versions of
rules_python (0.23.0 or later) register a toolchain by default again,
but it's a downloaded runtime, which makes upgrading more involved.

Instead, make the bazel_tools module register the "autodetecting"
toolchain to restore the rules_python 0.4.0 behavior. This should
suffice bazel_tools is upgraded to 0.23.0 or later.

Fixes bazelbuild#20458
rickeylev added a commit to rickeylev/bazel that referenced this issue Dec 12, 2023
A regression from upgrading to rules_python 0.22.0 is that a Python
toolchain is no longer registered by default (the prior 0.4.0 version
registered the "autodetecting" toolchain). Subsequent versions of
rules_python (0.23.0 or later) register a toolchain by default again,
but it's a downloaded runtime, which makes upgrading more involved.

Instead, make the bazel_tools module register the "autodetecting"
toolchain to restore the rules_python 0.4.0 behavior. This should
suffice bazel_tools is upgraded to 0.23.0 or later.

Fixes bazelbuild#20458
@keith
Copy link
Member

keith commented Dec 13, 2023

Now that HEAD is on bazel 8 could the work to remove python from bazel itself start now? In which case this probably wouldn't be necessary to fix

@rickeylev
Copy link
Contributor

Almost!

  1. The underlying code has to stay around until January or so for Google's internal builds. The Bazel symbols could be made inaccessible independently of that, though.
  2. rules_python needs to enable its Starlark implementation by default still. This is so projects using last_green and rolling releases still work.

@rickeylev
Copy link
Contributor

After some discussion, I'm not sure there's anything we can do in Bazel itself.

Wyverald pointed out that toolchains registered by bazel_tools will have a higher precedence than ones registered by rules_python. This is because in the BFS ordering, bazel_tools would come first. The net effect would be later versions of rules_python that register a hermetic runtime end up being ignored, which wouldn't be good.

The only other (expedient) option I can think of is to do a 0.22.1 release of rules_python, with the only change being it registers the toolchain.

katre added a commit to katre/rules_java that referenced this issue Dec 18, 2023
This version registers the python toolchains needed by rules_pkg and
others.

Fixes bazelbuild#161.
Related to bazelbuild/bazel#20458
@fmeum
Copy link
Collaborator

fmeum commented Dec 19, 2023

This is even more problematic than I thought: java_toolchain has a dependency on a Python toolchain through @bazel_tools//tools/jdk:proguard_whitelister.py, which means that building Java is broken with Bazel at HEAD unless a Python toolchain is registered. bazel-skylib itself is affected by this: bazelbuild/bazel-skylib#456.

This means that rules_java needs to add a dependency on a recent enough rules_python version.

@fmeum
Copy link
Collaborator

fmeum commented Dec 19, 2023

@ahumesky Is there a plan to get rid of proguard_whitelister.py, for example by replacing it with a prebuilt Java tool (e.g. a native image), or is it here to stay?

copybara-service bot pushed a commit to bazelbuild/rules_java that referenced this issue Dec 19, 2023
Copybara Import from #162

BEGIN_PUBLIC
Force rules_python to use version 0.24.0. (#162)

This version registers the python toolchains needed by rules_pkg and others.

Fixes #161.
Related to bazelbuild/bazel#20458

Closes #162
END_PUBLIC

COPYBARA_INTEGRATE_REVIEW=#162 from katre:fix-ci 5f0ce85
PiperOrigin-RevId: 592238927
Change-Id: If20c2d23aef44eb9f6c54600b21d14162a5ecea1
@cushon
Copy link
Contributor

cushon commented Dec 19, 2023

@ahumesky Is there a plan to get rid of proguard_whitelister.py, for example by replacing it with a prebuilt Java tool (e.g. a native image), or is it here to stay?

I think that rewriting it in Java would be a good thing to do, but there isn't an active plan to do that.

There was some internal discussion a couple years ago about rewriting it in Java, but that never ended up happening. We started using a prebuilt python binary for proguard_whitelister.py to avoid having the Java toolchain depend on python, and I'm not sure the Bazel python rules support that.

rickeylev added a commit to rickeylev/rules_python that referenced this issue Dec 19, 2023
@bazel_tools depends on rules_python, but the version is currently
specifies (0.22.0) doesn't register a toolchain by default. This means
the Bazel builtin Python rules aren't usable because no toolchain
is registered for them.

Registering the toolchain directly in @bazel_tools inverts the problem:
a toolchain is now always registered, but will usually always have
precedence over any that rules_python registers (because @bazel_tools
typically comes earlier in the module graph ordering).

Upgrading bazel_tools past to 0.23.0 or higher (which register a Python
toolchain automatically) is possible, but results in many failures in
Bazel CI that are hard to figure out.

To fix, we make rules_python register the autodetecting toolchain.
Later rules_python versions register a different toolchain, but this
at least gives Bazel something it can set as the minimum version that
preserves existing behavior.

Work towards bazelbuild/bazel#20458
@rickeylev
Copy link
Contributor

OK, 0.22.1 pending in BCR: bazelbuild/bazel-central-registry#1214

I'm not sure the Bazel python rules support [prebuilt python binaries]

If it's prebuilt, there aren't any dependencies or rules needed. It's just a file.

@rickeylev
Copy link
Contributor

Ok, 0.22.1 is now available in BCR. If you need the toolchain auto registered again, but can't upgrade rules_python to 0.23.0 or higher, then use 0.22.1.

The update to src/MODULE.bazel to use 0.22.1 is out for review.

meteorcloudy pushed a commit to meteorcloudy/bazel that referenced this issue Feb 15, 2024
… toolchain by default

A regression from upgrading to rules_python 0.22.0 is that a Python toolchain
is no longer registered by default (the prior 0.4.0 version registered the
"autodetecting" toolchain). Subsequent versions of rules_python (0.23.0 or
later) register a toolchain by default again, but it's a downloaded runtime,
which makes upgrading more involved.

To fix, upgrade to 0.22.1, which registers the autodetecting toolchain.
This restores the rules_python 0.4.0 behavior and should suffice until
bazel_tools is upgraded to 0.23.0 or later.

Fixes bazelbuild#20458

Closes bazelbuild#20514

PiperOrigin-RevId: 592578175
Change-Id: I707a4c96829063536f81cc10e144652102da1e7e
meteorcloudy pushed a commit to meteorcloudy/bazel that referenced this issue Feb 15, 2024
… toolchain by default

A regression from upgrading to rules_python 0.22.0 is that a Python toolchain
is no longer registered by default (the prior 0.4.0 version registered the
"autodetecting" toolchain). Subsequent versions of rules_python (0.23.0 or
later) register a toolchain by default again, but it's a downloaded runtime,
which makes upgrading more involved.

To fix, upgrade to 0.22.1, which registers the autodetecting toolchain.
This restores the rules_python 0.4.0 behavior and should suffice until
bazel_tools is upgraded to 0.23.0 or later.

Fixes bazelbuild#20458

Closes bazelbuild#20514

PiperOrigin-RevId: 592578175
Change-Id: I707a4c96829063536f81cc10e144652102da1e7e
github-merge-queue bot pushed a commit that referenced this issue Feb 15, 2024
… toolchain by default (#21369)

A regression from upgrading to rules_python 0.22.0 is that a Python
toolchain is no longer registered by default (the prior 0.4.0 version
registered the "autodetecting" toolchain). Subsequent versions of
rules_python (0.23.0 or later) register a toolchain by default again,
but it's a downloaded runtime, which makes upgrading more involved.

To fix, upgrade to 0.22.1, which registers the autodetecting toolchain.
This restores the rules_python 0.4.0 behavior and should suffice until
bazel_tools is upgraded to 0.23.0 or later.

Fixes #20458

Closes #20514

PiperOrigin-RevId: 592578175
Change-Id: I707a4c96829063536f81cc10e144652102da1e7e

Fixes #21348

Co-authored-by: Richard Levasseur <rlevasseur@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.

9 participants