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

incompatible_python_disallow_native_rules #17773

Open
rickeylev opened this issue Mar 14, 2023 · 10 comments
Open

incompatible_python_disallow_native_rules #17773

rickeylev opened this issue Mar 14, 2023 · 10 comments
Assignees
Labels
incompatible-change Incompatible/breaking change team-Rules-Python Native rules for Python

Comments

@rickeylev
Copy link
Contributor

rickeylev commented Mar 14, 2023

As part of moving the Python rules out of Bazel itself, usage of the native.py_* will only be allowed when done through the @rules_python.

The --incompatible_python_disallow_native_rules flag will control this behavior. When true, direct usage of native.py_* will result in a build failure.

To make migration easier, an allow list can be specifed using --python_native_rules_allowlist. If not specified, all usages are disallowed. When specified, packages matching the paths in the allowlist will be permitted to use the native rules, while others will fail. This makes incrementally migrating easier.

The basic changes necessary are:

  1. Add rules_python to your WORKSPACE and/or MODULE.bazel, per the rules_python instructions
  2. Reference any symbols through @rules_python.

How to load the symbols depends on what versions of rules_python you need to support.

If you require version 0.20.0 or later, load the symbols from their specific files:

load("@rules_python//python:py_binary.bzl", "py_binary")

py_binary(...)

For versions before 0.20.0, load the symbols from :defs.bzl:

load("@rules_python//python:defs.bzl", "py_binary")
py_binary(...)

Googlers: visibility is required to use defs.bzl; see the py-add-loads-lsc document for instructions

(To clarify: defs.bzl is present in all versions, but deprecated as of 0.20.0)

TODO:

  • link to tool that rewrites BUILD and bzl files
@rickeylev rickeylev self-assigned this Mar 14, 2023
@rickeylev rickeylev added incompatible-change Incompatible/breaking change team-Rules-Python Native rules for Python labels Mar 14, 2023
copybara-service bot pushed a commit that referenced this issue Mar 15, 2023
The `--incompatible_python_disallow_native_rules` flag, when enabled,
will cause an error if a native Python rule is used without going through
the @rule_python rule set.

To aid incremental migration, an allowlist can be specified using
`--python_native_rules_allowlist`. When specified, packages in the allowlist
won't fail when directly using the native rules.

Work towards #17773

PiperOrigin-RevId: 516687379
Change-Id: I154b538a9c7956bc3b2fba9e619fe63207559598
@meteorcloudy
Copy link
Member

If you want this flag to be tested by https://buildkite.com/bazel/bazelisk-plus-incompatible-flags, please add migration-ready label.

fweikert pushed a commit to fweikert/bazel that referenced this issue May 25, 2023
The `--incompatible_python_disallow_native_rules` flag, when enabled,
will cause an error if a native Python rule is used without going through
the @rule_python rule set.

To aid incremental migration, an allowlist can be specified using
`--python_native_rules_allowlist`. When specified, packages in the allowlist
won't fail when directly using the native rules.

Work towards bazelbuild#17773

PiperOrigin-RevId: 516687379
Change-Id: I154b538a9c7956bc3b2fba9e619fe63207559598
@paco-sevilla
Copy link

@rickeylev: When I activate this flag (Bazel 7.0.0) using a toolchain registered with python_register_toolchains from rules_python 0.27.1, I get the following error:

ERROR: /home/user/.cache/bazel/_bazel_user/87eea8b7fa21ae9ed4b3a002d3abf2a8/external/python_3_11_x86_64-unknown-linux-gnu/BUILD.bazel:63:11: in py_runtime rule @@python_3_11_x86_64-unknown-linux-gnu//:py3_runtime:
Traceback (most recent call last):
        File "/virtual_builtins_bzl/common/python/py_runtime_rule.bzl", line 24, column 25, in _py_runtime_impl
        File "/virtual_builtins_bzl/common/python/common.bzl", line 489, column 28, in check_native_allowed
Error in Label: invalid label in Label(): invalid package name '__EXTERNAL_REPOS__/python_3_11_x86_64-unknown-linux-gnu/': package names may not end with '/' (perhaps you meant ":"?)

I'm commenting here (and not in rules_python), because I think there's a bug in how this check is implemented:
The Label created in lines 489-492 of src/main/starlark/builtins_bzl/common/python/common.bzl should probably only be Label("@//__EXTERNAL_REPOS__/{workspace}".format(workspace=ctx.label.workspace_name) when the package is empty.

@rickeylev
Copy link
Contributor Author

Doh. Thanks for the report. Yeah, I think you're right. Should be an easy fix.

Basic repro steps: run rules_python's pip_parse example tests with --incompatible_python_disallow_native_rules set.

# git clone rules_python
cd examples/pip_parse
USE_BAZEL_VERSION=latest bazel test ... --incompatible_python_disallow_native_rules

rickeylev added a commit to rickeylev/bazel that referenced this issue Dec 12, 2023
…mpty package

This basically makes it usable with the downloaded runtimes rules_python
makes available. The issue was the code to construct the label was
leaving a trailing "/" when the the target being checked at the root
of the workspace. To fix, just omit the trailing slash when
the package name is empty to prevent the trailing slash.

Work towards bazelbuild#17773
rickeylev added a commit to rickeylev/bazel that referenced this issue Dec 13, 2023
…mpty package

This basically makes it usable with the downloaded runtimes rules_python
makes available. The issue was the code to construct the label was
leaving a trailing "/" when the the target being checked at the root
of the workspace. To fix, just omit the trailing slash when
the package name is empty to prevent the trailing slash.

Work towards bazelbuild#17773
rickeylev added a commit to rickeylev/bazel that referenced this issue Dec 13, 2023
…mpty package

This basically makes it usable with the downloaded runtimes rules_python
makes available. The issue was the code to construct the label was
leaving a trailing "/" when the the target being checked at the root
of the workspace. To fix, just omit the trailing slash when
the package name is empty to prevent the trailing slash.

Work towards bazelbuild#17773
rickeylev added a commit to rickeylev/bazel that referenced this issue Dec 13, 2023
…mpty package

This basically makes it usable with the downloaded runtimes rules_python
makes available. The issue was the code to construct the label was
leaving a trailing "/" when the the target being checked at the root
of the workspace. To fix, just omit the trailing slash when
the package name is empty to prevent the trailing slash.

Work towards bazelbuild#17773
rickeylev added a commit to rickeylev/bazel that referenced this issue Dec 13, 2023
…mpty package

This basically makes it usable with the downloaded runtimes rules_python
makes available. The issue was the code to construct the label was
leaving a trailing "/" when the the target being checked at the root
of the workspace. To fix, just omit the trailing slash when
the package name is empty to prevent the trailing slash.

Work towards bazelbuild#17773
copybara-service bot pushed a commit that referenced this issue Dec 14, 2023
…evel external repo targets

This basically makes it usable with the downloaded runtimes rules_python makes
available, which reference their runtimes as e.g. `@python_3_11//:python`.

The issue was the code to construct the label was leaving a trailing "/" when
the the target being checked at the root of the workspace. To fix, just omit
the trailing slash when the package name is empty to prevent the trailing
slash.

Work towards #17773

Closes #20516

PiperOrigin-RevId: 591053422
Change-Id: I7790df2db10278844ae2b36cfe671de03164972f
@rickeylev
Copy link
Contributor Author

Ok, this should be fixed at head now. It's worth putting that into a 7.x patch release

@rickeylev
Copy link
Contributor Author

@bazel-io flag

I'd like to have 1533cd1 backported into the next patch release.

(I think i did that right, not sure)

@rickeylev
Copy link
Contributor Author

@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 15, 2023
@iancha1992
Copy link
Member

@bazel-io fork 7.1.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 15, 2023
bazel-io pushed a commit to bazel-io/bazel that referenced this issue Jan 17, 2024
…evel external repo targets

This basically makes it usable with the downloaded runtimes rules_python makes
available, which reference their runtimes as e.g. `@python_3_11//:python`.

The issue was the code to construct the label was leaving a trailing "/" when
the the target being checked at the root of the workspace. To fix, just omit
the trailing slash when the package name is empty to prevent the trailing
slash.

Work towards bazelbuild#17773

Closes bazelbuild#20516

PiperOrigin-RevId: 591053422
Change-Id: I7790df2db10278844ae2b36cfe671de03164972f
github-merge-queue bot pushed a commit that referenced this issue Jan 17, 2024
…or top-level external repo targets (#20923)

This basically makes it usable with the downloaded runtimes rules_python
makes
available, which reference their runtimes as e.g.
`@python_3_11//:python`.

The issue was the code to construct the label was leaving a trailing "/"
when
the the target being checked at the root of the workspace. To fix, just
omit
the trailing slash when the package name is empty to prevent the
trailing
slash.

Work towards #17773

Closes #20516

Commit
1533cd1

PiperOrigin-RevId: 591053422
Change-Id: I7790df2db10278844ae2b36cfe671de03164972f

Co-authored-by: Richard Levasseur <rlevasseur@google.com>
@snakethatlovesstaticlibs
Copy link

snakethatlovesstaticlibs commented Jul 19, 2024

On bazel version 7.2.1 with rules_python 0.34.0 I'm seeing:

ERROR: /home/phantom/.cache/bazel/_bazel_phantom/f7e0b8bbe249fe113936b602b991b87d/external/bazel_tools/tools/python/BUILD:110:31: in py_runtime rule @@bazel_tools//tools/python:_autodetecting_py3_runtime:
Traceback (most recent call last):
	File "/virtual_builtins_bzl/common/python/py_runtime_rule.bzl", line 24, column 25, in _py_runtime_impl
	File "/virtual_builtins_bzl/common/python/common.bzl", line 520, column 13, in check_native_allowed
Error in fail: @@bazel_tools//tools/python:_autodetecting_py3_runtime not allowed to use native.py_runtime
Generated by: define_autodetecting_toolchain(name=autodetecting_toolchain) in tools/python/BUILD:110:31
Allowlist: no allowlist specified; all disallowed; specify one with --python_native_rules_allowlist
Migrate to using @rules_python, see https://github.com/bazelbuild/bazel/issues/17773
FIXCMD: add_python_loads --target=@@bazel_tools//tools/python:_autodetecting_py3_runtime --rule=py_runtime --generator_name=autodetecting_toolchain --location=tools/python/BUILD:110:31

when specifying common --incompatible_python_disallow_native_rules. Is this a know issue? I tried specifying --python_native_rules_allowlist=@@bazel_tools//tools/python:_autodetecting_py3_runtime

but then that caused a self edge:

ERROR: /home/phantom/.cache/bazel/_bazel_phantom/f7e0b8bbe249fe113936b602b991b87d/external/bazel_tools/tools/python/BUILD:110:31: in py_runtime rule @@bazel_tools//tools/python:_autodetecting_py3_runtime: cycle in dependency graph:
    //bazel:soar-tars (f0b4e44b5238b7ebbab8d156cd77b520bcaf9839f7a6ed7120d63ed1733253cc)
    //bazel:soar-tars (46af5107e5a89655aebe2bfe5749d1cdf130c6dbb862824e032d17a215d3e42f)
    //illusions:ui_tar (46af5107e5a89655aebe2bfe5749d1cdf130c6dbb862824e032d17a215d3e42f)
    @@rules_pkg//pkg/private/tar:build_tar (8022fbbb7d81b8b46abdb3283eeab62024b8de464a6b17a1c1283ba35dde434f)
    @@rules_pkg//pkg/private:manifest (8022fbbb7d81b8b46abdb3283eeab62024b8de464a6b17a1c1283ba35dde434f)
.-> @@bazel_tools//tools/python:_autodetecting_py3_runtime (8022fbbb7d81b8b46abdb3283eeab62024b8de464a6b17a1c1283ba35dde434f) [self-edge]

Edit: it looks like I can solve this by doing register_toolchains on a @bazel_tools//tools/python:toolchain_type toolchain type. I was expecting

python_register_toolchains(
    name = "python_3_9",
    python_version = "3.9",
)

to do that for me, but it did not

@rickeylev
Copy link
Contributor Author

The flag --python_native_rules_allowlist must point to a package_group target, which has a list of paths that are allowed. To permit that bazel tools path, I think you'd need something like //__EXTERNAL__REPOS__/bazel_tools/... in the package group.

python_register_toolchains() should have registered a toolchain with a higher priority than the bazel_tools autodetecting one. If it isn't, then please file a bug with rules_python with repro steps.

@kilian-funk
Copy link

There is a typo in @rickeylev 's comment above. The string you need in the package group is //__EXTERNAL_REPOS__/bazel_tools/... with only one underscore between EXTERNAL and REPOS. At least that is what worked for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
incompatible-change Incompatible/breaking change team-Rules-Python Native rules for Python
Projects
None yet
Development

No branches or pull requests

7 participants