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

bug: PyRuntimeInfo provided by py_binary doesn't match PyRuntimeInfo symbol from rules_python #1732

Closed
axbycc-mark opened this issue Jan 30, 2024 · 17 comments · Fixed by #1748
Assignees

Comments

@axbycc-mark
Copy link

axbycc-mark commented Jan 30, 2024

🐞 bug report

Affected Rule

The issue is caused by the rule:

py_binary rule

Is this a regression?

Yes. Well behaved code loading everything from rules_python (as recommended and desired) stopped working.

A workaround is possible if you control the rule that consumes the target that should provide PyRuntimeInfo: accept both the rules_python PyRuntimeInfo symbol and builtin PyRuntimeInfo provider. When accessing it in rule code, prefer the rules_python one over the bulitin one. When forwarding, forward both if you need compatibility. The gist of the code would look like this:

load("@rules_python//python:py_runtime_info.bzl", RulesPythonPyRuntimeInfo = "PyRuntimeInfo")

BuiltinPyRuntimeInfo = PyRuntimeInfo

foo_binary = rule(..., attrs = {
  # Note its a 2-element list of 1-element lists.
  "bin": attr.label(provides = [[RulesPythonPyRuntimeInfo], [BuiltinPyRuntimeInfo]])
})

def foo_binary_impl(ctx):
  bin = ctx.attr.bin
  if RulesPythonPyRuntimeInfo in bin:
    rp_info = bin[RulesPythonPyRuntimeInfo]
    providers.append(rp_info)
  else:
    rp_info = None
  if BuiltinPyRuntimeInfo in bin:
    builtin_info = bin[BuiltinPyRuntimeInfo]
    providers.append(builtin_info)
  else:
    builtin_info = None
  return providers

Description

Using py_binary rule triggers fail() called with error string "target {} does not have rules_python PyRuntimeInfo or builtin PyRuntimeInfo)" on Windows 10

🔬 Minimal Reproduction

Load PyRuntimeInfo and py_binary from rules_python, then try to read the loaded PyRuntimeInfo from the py_binary target.

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

foo(name="foo", bin=":bin")
py_binary(name="bin", srcs=["bin.py"])

# defs.bzl

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

foo = rule(attrs = {
  "bin": attr.label(provides=[PyRuntimeInfo])
}, ...
)

🔥 Exception or Error

An error about PyRuntimeInfo not being in the target being depended upon.

When it occurs through provided =..., it will fail the build pretty early with a Bazel-generated error

If you have rule code doing target[PyRuntimeInfo], it'll be an error pointing to the line in your rule code looking up that provider.

If the version-aware rules are used, it might result in an error like the below:


bazel run @hedron_compile_commands//:refresh_all
ERROR: C:/users/marki/_bazel_marki/alqgzwqn/external/hedron_compile_commands/BUILD:7:25: in _transition_py_binary rule @@hedron_compile_commands//:refresh_all:
Traceback (most recent call last):
    File "C:/users/marki/_bazel_marki/alqgzwqn/external/rules_python/python/config_settings/transition.bzl", line 78, column 13, in _transition_py_impl
            fail("target {} does not have rules_python PyRuntimeInfo or builtin PyRuntimeInfo".format(target))
Error in fail: target  does not have rules_python PyRuntimeInfo or builtin PyRuntimeInfo
ERROR: C:/users/marki/_bazel_marki/alqgzwqn/external/hedron_compile_commands/BUILD:7:25: Analysis of target '@@hedron_compile_commands//:refresh_all' failed
ERROR: Analysis of target '@@hedron_compile_commands//:refresh_all' failed; build aborted

🌍 Your Environment

Output of bazel version:

  
bazel 7.0.2
  

Rules_python version:
0.29.0 for sure. Likely 0.28.x, too, but I didn't check.

Anything else relevant?
Related issue in Hedron's compile-commands-extractor project hedronvision/bazel-compile-commands-extractor#163

@axbycc-mark
Copy link
Author

cc @cpsauer

@rickeylev
Copy link
Contributor

I'm somewhat skeptical that this is an issue in rules_python, or at the least is a new issue in a recent version. The logic producing that particular failure message was introduced in Dec 2023 in #1604, however, the prior logic was equivalent (it also required the PyRuntimeInfo provider because it did target[PyRuntimeInfo]), and has been around since Nov 2022 in #846.

The error message is also odd: the target name is blank. I don't know how that would happen, but is certainly very strange.

@cpsauer
Copy link

cpsauer commented Jan 31, 2024

Hey, thanks for replying back so fast Richard.

Context here is that I'd recently moved a (fairly popular) bazel tool we wrote over to rules_python to get hermetic python and pip_parse, but there have been some issues like this one that mean we might have to revert. I'm moderately confident that it's a rules_python (though of course, possibly upstream, bazel core) issue, since this works fine on other platforms (macOS and linux). @axbycc-mark has confirmed the issue with both rules_python 0.28 and 0.29, but that all works fine before our switch to rules_python.

Good spot on the missing target name. Is it possible rules_python breaks on Windows if py_binary is called from a macro, as it is here, causing the target to not get picked up somehow? Fairly simple wrapper-macro here otherwise. Would love your help scoping what might be wrong here.

I presume we're pretty sure that py_binary works more generally on Windows? @axbycc-mark, to narrow things down, I'll push up a commit adding a non-macro py_binary to test. If you could grab the latest commit of the tool (backlink incoming) and then see if bazel build @hedron_compile_commands//:nvcc_clang_diff succeeds or fails the same way, that'd be awesome.

Thanks,
Chris
(ex-Googler)

cpsauer added a commit to hedronvision/bazel-compile-commands-extractor that referenced this issue Jan 31, 2024
@rickeylev
Copy link
Contributor

Can you try a 0.27.0 or earlier? The 0.28.0 release contains some changes to py_runtime and PyRuntimeInfo, however they should be transparent.

re: empty target name: When I say weird, I mean it's an impossible code path, as in, the setting of that attribute involves a non-empty string literal. See https://github.com/bazelbuild/rules_python/blob/main/python/config_settings/transition.bzl#L250

So, worst case, it would have the value "_".

The only thing I can think of is a strange interaction when mixing the bazel-bundled py_runtime rule and/or PyRuntimeInfo objects and the rule_python ones. In case it wasn't clear, there are "two" PyRuntimeInfo objects -- the ones defined by rules_python, and the ones defined by Bazel itself. However, the particular code that is failing is compatibility code that is looking for either of those PyRuntimeInfo objects.

Hm, maybe a n interaction between workspace and bzlmod? In Bazel 7, bzlmod is enabled by default, but stuff in WORKSPACE can bleed over into bzlmod. Do things work if you set --noenable_bzlmod ? Alternatively, create an empty file named WORKSPACE.bzlmod (along side WORKSPACE); this will prevent WORKSPACE from bleeding over into bzlmod. You can also try building with Bazel 6.4, where bzlmod is disabled by default.

Also, in case it wasn't clear, loading the defs from the versioned repos means they are being bound to that specific python version. i.e. load("@python_3_11//:defs.bzl", "py_binary") means it's going to use a py_binary that forces itself to find a 3.11 compatible interpreter. Using these is more common for build tools (or other special situations) when you don't want the python version to change out from under you. Otherwise, load the version-unaware rules, load("@rules_python//python:py_binary.bzl").

@cpsauer
Copy link

cpsauer commented Feb 1, 2024

@axbycc-mark has the advantage on testing, so I'll defer to him, but it certainly seems like it's hitting that case somehow! Definitely seems worth running the test with --noenable_bzlmod

Richard, do you guys have the ability to test on windows and investigate?

Re the python version: Yep, got it & intentional. This is a build/dev tool seeking reproducible hermetic python for its implementation, rather than exposing to users.

cpsauer added a commit to hedronvision/bazel-compile-commands-extractor that referenced this issue Feb 1, 2024
Mostly reverts 0e5b1aa

Tracking restoration at #168

Please see
- #163
- bazelbuild/rules_python#1732
- #165
- (rules_python issue to come)
- #166
- bazelbuild/rules_python#1169
@axbycc-mark
Copy link
Author

Here are my results with the --noenable_bzlmod command from the reproduction repo.

bazel run --noenable_bzlmod @hedron_compile_commands//:refresh_all
INFO: Analyzed target @@hedron_compile_commands//:refresh_all (50 packages loaded, 4222 targets configured).
INFO: Found 1 target...
Target @@hedron_compile_commands//:refresh_all up-to-date:
  bazel-bin/external/hedron_compile_commands/_refresh_all.zip
  bazel-bin/external/hedron_compile_commands/_refresh_all.exe
  bazel-bin/external/hedron_compile_commands/refresh_all.py
  bazel-bin/external/hedron_compile_commands/refresh_all.zip
INFO: Elapsed time: 34.757s, Critical Path: 9.24s
INFO: 16 processes: 12 internal, 4 local.
INFO: Build completed successfully, 16 total actions
INFO: Running command line: bazel-bin/external/hedron_compile_commands/refresh_all.exe
Traceback (most recent call last):
  File "C:\Users\marki\AppData\Local\Temp\Bazel.runfiles_qg7mw25d\runfiles\hedron_test\..\hedron_compile_commands\refresh_all.py", line 1382, in <module>
    _ensure_external_workspaces_link_exists()
  File "C:\Users\marki\AppData\Local\Temp\Bazel.runfiles_qg7mw25d\runfiles\hedron_test\..\hedron_compile_commands\refresh_all.py", line 1291, in _ensure_external_workspaces_link_exists
    current_dest = current_dest.removeprefix('\\\\?\\')
                   ^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'WindowsPath' object has no attribute 'removeprefix'

@cpsauer
Copy link

cpsauer commented Feb 1, 2024

^ Oh, interesting! Looks like it's a windows-only rules_python bzlmod interaction issue, then!
Great hypothesis @rickeylev, and thanks for testing @axbycc-mark.
@rickeylev, any idea what might be going on?

[That other error is part of the tool, not rules_python, already fixed in a subsequent commit; rules_python is working as expected with --noenable_bzlmod.]

@cpsauer
Copy link

cpsauer commented Feb 1, 2024

@axbycc-mark, would you be down to still point to hedronvision/bazel-compile-commands-extractor@40a51d6 and try bazel build @hedron_compile_commands//:nvcc_clang_diff with and without --noenable_bzlmod?

@rickeylev
Copy link
Contributor

That looks like an error in the refresh_all.py code. removeprefix is a method of python str objects, but it sounds like it has a WindowsPath object from pathlib. It's a fairly easy mistake to make. A function accepting a string path often doesn't notice because it just passes the value onto a lower-level API that understands Path objects.

@cpsauer
Copy link

cpsauer commented Feb 2, 2024

^yep, as above! Looks like you were right that the original issue is a windows-specific rules_python bazelmod interaction?

@axbycc-mark
Copy link
Author

@cpsauer

bzlmod

bazel build @hedron_compile_commands//:nvcc_clang_diff
Starting local Bazel server and connecting to it...
DEBUG: Rule 'hedron_compile_commands' indicated that a canonical reproducible form can be obtained by modifying arguments integrity = "sha256-Ijvk9dFQwzhEUTJNPIfQ3DC8EM63maTm4f5vvWBKK9o="
DEBUG: Repository hedron_compile_commands instantiated at:
  D:/hedron_test/WORKSPACE:9:13: in <toplevel>
Repository rule http_archive defined at:
  C:/users/marki/_bazel_marki/alqgzwqn/external/bazel_tools/tools/build_defs/repo/http.bzl:381:31: in <toplevel>
ERROR: C:/users/marki/_bazel_marki/alqgzwqn/external/hedron_compile_commands/BUILD:41:10: in _transition_py_binary rule @@hedron_compile_commands//:nvcc_clang_diff:
Traceback (most recent call last):
        File "C:/users/marki/_bazel_marki/alqgzwqn/external/rules_python/python/config_settings/transition.bzl", line 78, column 13, in _transition_py_impl
                fail("target {} does not have rules_python PyRuntimeInfo or builtin PyRuntimeInfo".format(target))
Error in fail: target <target @@hedron_compile_commands//:_nvcc_clang_diff> does not have rules_python PyRuntimeInfo or builtin PyRuntimeInfo
ERROR: C:/users/marki/_bazel_marki/alqgzwqn/external/hedron_compile_commands/BUILD:41:10: Analysis of target '@@hedron_compile_commands//:nvcc_clang_diff' failed
ERROR: Analysis of target '@@hedron_compile_commands//:nvcc_clang_diff' failed; build aborted
INFO: Elapsed time: 20.824s, Critical Path: 0.06s
INFO: 1 process: 1 internal.
ERROR: Build did NOT complete successfully

no bzlmod

bazel build --noenable_bzlmod @hedron_compile_commands//:nvcc_clang_diff
DEBUG: Rule 'hedron_compile_commands' indicated that a canonical reproducible form can be obtained by modifying arguments integrity = "sha256-Ijvk9dFQwzhEUTJNPIfQ3DC8EM63maTm4f5vvWBKK9o="
DEBUG: Repository hedron_compile_commands instantiated at:
  D:/hedron_test/WORKSPACE:9:13: in <toplevel>
Repository rule http_archive defined at:
  C:/users/marki/_bazel_marki/alqgzwqn/external/bazel_tools/tools/build_defs/repo/http.bzl:381:31: in <toplevel>
INFO: Analyzed target @@hedron_compile_commands//:nvcc_clang_diff (47 packages loaded, 4198 targets configured).
INFO: Found 1 target...
Target @@hedron_compile_commands//:nvcc_clang_diff up-to-date:
  bazel-bin/external/hedron_compile_commands/_nvcc_clang_diff.zip
  bazel-bin/external/hedron_compile_commands/_nvcc_clang_diff.exe
  bazel-bin/external/hedron_compile_commands/nvcc_clang_diff.zip
INFO: Elapsed time: 24.503s, Critical Path: 6.76s
INFO: 12 processes: 10 internal, 2 local.
INFO: Build completed successfully, 12 total actions

@cpsauer
Copy link

cpsauer commented Feb 2, 2024

Thanks for testing! That helps narrow things down: Looks like this has nothing to do with macros or other fancy uses of py_binary. Instead, all is consistent with Richard's hypothesis that rules_python is clashing with itself when brought in via the workspace in Bazel 7 on Windows when bzlmod defaults to being on. Perhaps because a rules_python version is brought in by default as a Bazel tool?

For now, ofc, please hop back to the latest commit of the tool where all should work (we've temporarily reverted our rules_python usage until this and some other issues are fixed). Thanks for all your help tracking this down!

@aignas
Copy link
Collaborator

aignas commented Feb 3, 2024

@cpsauer, if you want to things not clash, you should check out WORKSPACE.bzlmod usage.

@cpsauer
Copy link

cpsauer commented Feb 9, 2024

^ indeed, but folks are going to use the tool and rules_python from the WORKSPACE for a good while--isn't that a case worth supporting?

@rickeylev
Copy link
Contributor

I'm very sure the bug this is talking about is the same one @mattem was able to show a simpler repro for last week, so I'll repurpose this to that. I don't fully understand how these hadron rules are triggering it, but the cause would be the same.

The basic issue is the switch to using the rules_python PyRuntimeInfo object overlooked that some users do want to get the PyRuntimeInfo object out of e.g. py_binary. This created the situation where the builtin py_binary rules provide the builtin PyRuntimeInfo object, but the rules_python PyRuntimeInfo binding was pointing to the rules_python PyRuntimeInfo definition. Because providers operate by identity, the two aren't considered the same.

I'll switch the PyRuntimeInfo symbol back to the builtin one for now. Instead it'll be guarded by pystar being enabled

I have a fix with tests I'm about ready to send for review. It'll go into the next release, planned for once this fix is merged.

@rickeylev rickeylev changed the title Windows 10: rules_python complaining about it's own py_binary rule bug: PyRuntimeInfo provided by py_binary doesn't match PyRuntimeInfo symbol from rules_python Feb 9, 2024
@rickeylev rickeylev self-assigned this Feb 9, 2024
@cpsauer
Copy link

cpsauer commented Feb 10, 2024

Thanks so much for tackling, @rickeylev!

@cpsauer
Copy link

cpsauer commented Feb 10, 2024

@axbycc-mark, could you give that PR's commit a quick test from your workspace, just to make sure that fixes things? Just spin up a quick py_binary and make sure all works (but didn't on the prior release).

Thanks so much all 💛

github-merge-queue bot pushed a commit that referenced this issue Feb 10, 2024
…d. (#1748)

This fixes the bug where the PyRuntimeInfo symbol rules_python exported
wasn't matching the provider identity that `py_binary` actually
provided.

The basic problem was, when pystar is disabled:
 * PyRuntimeInfo is the rules_python defined provider
* py_binary is `native.py_binary`, which provides only the builtin
PyRuntimeInfo provider.

Thus, when users loaded the rules_python PyRuntimeInfo symbol, it was
referring to a provider that the underlying py_binary didn't actually
provide. Pystar is always disabled on Bazel 6.4,
and enabling it for Bazel 7 will happen eminently.

This typically showed up when users had a custom rule with an attribute
definition that used the rules_python PyRuntimeInfo.

To fix, only use the rules_python define PyRuntimeInfo if pystar is
enabled. This ensures the providers the underlying rules are providing
match the symbols that rules_python is exported.

* Also fixes `py_binary` and `py_test` to also return the builtin
PyRuntimeInfo. This
should make the transition from the builtin symbols to the rules_python
symbols a bit
  easier.

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

Successfully merging a pull request may close this issue.

4 participants