fix(PyRuntimeInfo): use builtin PyRuntimeInfo unless pystar is enabled. #1748
+55
−3
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 still not enabled for Bazel 7. 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.
|
Ah hm, looks like the pystar config isn't happy with this. I'll put this back into draft and figure out what's going on. |
|
Tests fix. PTAL. It uncovered another gap in compatibility: pystar py_binary wasn't returning the builtin PyRuntimeInfo. It does now, which should allow it to (1) work with a builtin py_runtime defined toolchain, and (2) allow it to work with code that is expecting the builtin PyRuntimeInfo (i.e. users that aren't loading from rules_python). |
aignas
approved these changes
Feb 10, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes the bug where the PyRuntimeInfo symbol rules_python exported wasn't matching the provider identity that
py_binaryactually provided.The basic problem was, when pystar is disabled:
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.
py_binaryandpy_testto also return the builtin PyRuntimeInfo. Thisshould make the transition from the builtin symbols to the rules_python symbols a bit
easier.
Fixes #1732