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

Bazel 7 internally uses a too old version of rules_python #20315

Closed
cameron-martin opened this issue Nov 27, 2023 · 15 comments
Closed

Bazel 7 internally uses a too old version of rules_python #20315

cameron-martin opened this issue Nov 27, 2023 · 15 comments
Labels

Comments

@cameron-martin
Copy link
Contributor

cameron-martin commented Nov 27, 2023

Description of the bug:

If no version of rules_python is specified via bzlmod, bazel internally uses the oldest version, 0.4.0. However, this version is too old to support some features used by @bazel_tools, causing this to error.

Which category does this issue belong to?

No response

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

In a new directory:

touch WORKSPACE
echo 7.0.0rc4 > .bazelversion
bazel build @bazel_tools//src/main/protobuf:all

This should then output an error:

ERROR: while parsing '@bazel_tools//src/main/protobuf:all': error loading package '@@bazel_tools//src/main/protobuf': cannot load '@@rules_python~0.4.0//python:proto.bzl': no such file

Which operating system are you running Bazel on?

Ubuntu 22.04

What is the output of bazel info release?

release 7.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

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

No response

Have you found anything relevant by searching the web?

No response

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

No response

@cameron-martin
Copy link
Contributor Author

The internally-used version is specified here:

bazel_dep(name = "rules_python", version = "0.4.0")

The first version of that introduces py_proto_library is 0.17.0. See bazelbuild/rules_python@0d3c4f7.

@brentleyjones
Copy link
Contributor

@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 Nov 27, 2023
@keertk keertk added the team-Rules-Python Native rules for Python label Nov 27, 2023
@keertk
Copy link
Member

keertk commented Nov 27, 2023

@bazel-io fork 7.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 Nov 27, 2023
@keertk
Copy link
Member

keertk commented Nov 27, 2023

cc @meteorcloudy @rickeylev

@rickeylev
Copy link
Contributor

Huh. bazel_tools loads rules_python? TIL. Oh I see, it looks like these are protos for Bazel's protocols? Ok, yeah that makes sense.

In any case, yeah, this sounds like the version needs to be raised in MODULE.bazel

@meteorcloudy
Copy link
Member

@rickeylev Do you remember in which version hermetic python toolchain was introduced by default?

I tried to upgrade rules_python in MODULE.tools in #20042 (comment), but the default hermetic python toolchain broke running our integration tests without network.

I can try to upgprade to 0.17 and see if it works.

@meteorcloudy
Copy link
Member

And in the long term, I think users should not depend on anything in @bazel_tools//src/main/protobuf and we should completely delete this package from @bazel_tools and remove rules_python as a default dependency in MODULE.tools.

To workaround this issue, users can define a newer bazel version in their MODULE.bazel file (bazel_dep("rules_python", version="0.27.0")).

@cameron-martin
Copy link
Contributor Author

And in the long term, I think users should not depend on anything in @bazel_tools//src/main/protobuf and we should completely delete this package from @bazel_tools and remove rules_python as a default dependency in MODULE.tools.

What would the plan be for letting people access these protos?

To workaround this issue, users can define a newer bazel version in their MODULE.bazel file (bazel_dep("rules_python", version="0.27.0")).

The issue with that is it forces the repo to use bzlmod for rules_python, since this takes precedence over a WORKSPACE-defined rules_python. It's not the end of the world, but does make the migration more of a hassle.

@meteorcloudy
Copy link
Member

meteorcloudy commented Nov 28, 2023

What would the plan be for letting people access these protos?

I think they should be published as a separate Bazel module in the BCR. Then Bazel or any other projects can depend on it, but it won't be a default dependency for every Bazel project. @comius @lberki WDYT?

The issue with that is it forces the repo to use bzlmod for rules_python, since this takes precedence over a WORKSPACE-defined rules_python. It's not the end of the world, but does make the migration more of a hassle.

I see, but in generally, you should not have two different rules_python in your dependency graph. It may cause incompatibility somewhere later. But in this case, maybe single_version_override could override the module version without introducing the visibility on rules_python for the root module.

@meteorcloudy
Copy link
Member

As https://buildkite.com/bazel/google-bazel-presubmit/builds/74291 shows, upgrading the default rules_python version in bazel_tools requires some non trivial test clean up.

@cameron-martin @brentleyjones Are you fine with removing this issue as a release blocker since there is a workaround? So that we don't block Bazel 7 further and once we figure out a fix, we can backport it to 7.1.

@brentleyjones
Copy link
Contributor

Sounds good to me.

@cameron-martin
Copy link
Contributor Author

I see, but in generally, you should not have two different rules_python in your dependency graph. It may cause incompatibility somewhere later. But in this case, maybe single_version_override could override the module version without introducing the visibility on rules_python for the root module.

In our case we don't actually use any of the python-related rules from @bazel_tools, but yes I can see how this could cause issues if you do.

Using single_version_override here seems reasonable.

@lberki
Copy link
Contributor

lberki commented Nov 29, 2023

+1 for removing these protos from @bazel_tools. For a lot of them, there is no plausible reason why Bazel would carry them with itself (e.g. command_server.proto, invocation_policy.proto` and all others that don't describe an interface used within the build) and even those protos that do describe an intra-build interface, it's probably better to move them to a separate repo so that people who don't use them (which I expect is the vast majority) don't have to pay the price of having them there.

@rickeylev
Copy link
Contributor

rickeylev commented Nov 29, 2023

I have a pending change that updates to rules_python 0.22.0.

For posterity:

  • For [0.4.0 to 0.12.0] (inclusive), rules_python's MODULE.bazel would call register_toolchains("@bazel_tools//tools/python:autodetecting_toolchain")
  • There is no 0.13.0 and 0.14.0 was yanked.
  • For [0.15.0 to 0.22.0] (inclusive), no toolchain is registered by default
  • For 0.23.0 and later, a downloaded hermetic runtime is registered by default.
  • In 0.17.0, the py_proto_library rule was introduced. I'm not sure how functional it was in practice at this point, though.
  • meteorcloudy says he tried 0.17.0 and it didn't look good
  • 0.27.0 also had a lot of failures. Its also very recent, and would force it as the version used, so we'd rather not do that.
  • 0.23.0 might work better than 0.27.0, but CI had quite a few failures due to downloading more things, so this might take as much rework as 0.27.0 to land

Anyways, my point here is two fold:

  • In order for py_proto_library to be available, the implicit toolchain is no longer registered. Users (or I suppose bazel_tool's MODULE could?) will have to register a toolchain somehow (manually, or using rules_python 0.23.0 or later).
  • In order for us to progress past 0.22.0, we'll need to either update CI to pre-download the necessary repos, or have a place to define some toolchains for CI to use

github-merge-queue bot pushed a commit that referenced this issue Jan 17, 2024
Raise rules_python version to 0.22.0 so that py_proto_library is
available.

The py_proto_library rule was introduced in 0.17.0, but there's been a
variety of fixes since then, so using a newer version is more advisable.

Note: A difference between 0.4.0 and 0.22.0 is that 0.4.0 calls
`register_toolchains("@bazel_tools//tools/python:autodetecting_toolchain")`
to provide an extremely minimal, though incomplete, Python toolchain.
0.22.0 does not do that, and does not provide any Python toolchain by
default.

Fixes #20315
Commit
48eb023

PiperOrigin-RevId: 586758674
Change-Id: Ia479f4475d3bbbf048642470a3e33f272c2eba2e

---------

Co-authored-by: Richard Levasseur <rlevasseur@google.com>
tinder-maxwellelliott added a commit to Tinder/bazel-diff that referenced this issue Jan 29, 2024
@iancha1992
Copy link
Member

A fix for this issue has been included in Bazel 7.1.0 RC1. Please test out the release candidate and report any issues as soon as possible. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

10 participants