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

Removal of PyInfo from Bazel causes semantic change #2414

Closed
comius opened this issue Nov 15, 2024 · 4 comments · Fixed by #2415
Closed

Removal of PyInfo from Bazel causes semantic change #2414

comius opened this issue Nov 15, 2024 · 4 comments · Fixed by #2415

Comments

@comius
Copy link
Contributor

comius commented Nov 15, 2024

🐞 bug report

Description

I upgraded Bazel to use rules_python 0.39.0 -> all the tests pass.
I removed python providers from Bazel -> smoke tests started failing.

🔬 Minimal Reproduction

Checkout bazelbuild/bazel#24343
bazel build //src/test/shell/bazel:bazel_example_test --test_filter=test_native_python

🔥 Exception or Error

https://storage.googleapis.com/bazel-untrusted-buildkite-artifacts/01932fe0-131a-4503-8eac-2516742f6f27/src/test/shell/bazel/bazel_example_test/shard_1_of_3/test_attempts/attempt_1.log

Example in: https://github.com/bazelbuild/bazel/blob/master/examples/py_native/bin.py


-- Test log: -----------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/Bazel.runfiles_vczmqfyg/runfiles/_main/examples/py_native/bin.py", line 4, in 
    from fib import Fib
ModuleNotFoundError: No module named 'fib'

My thoughs

It looks like that part of information was in Builtin providers that now gets lost.

@rickeylev
Copy link
Collaborator

rickeylev commented Nov 15, 2024 via email

@rickeylev
Copy link
Collaborator

I was able to repro.

repro note: PR was updated to use a repo-root based import. Needs to be changed back to from fib import Fib to demonstrate the failure.

sys.path is missing the fibonacci. So yeah, something is causing the imports=[.] part to be lost.

@rickeylev
Copy link
Collaborator

Ok, I think I figured it out. There's an operator precdence bug in collect_imports() that causes all imports strings to be dropped, but only if BuiltinPyInfo is none. I'll work on a fix.

github-merge-queue bot pushed a commit that referenced this issue Nov 16, 2024
The collect_imports() function added import strings from BuiltinPyInfo
if it was non-None.
However, operator precedence caused the `if-else` ternary to ignore both
list comprehensions
(one for PyInfo and one for BuiltinPyInfo) if BuiltinPyInfo was None.

To fix, I rewrote the function as a regular for loop to eliminate the
ambiguous looking
ternary expression.

Fixes: #2414
@rickeylev
Copy link
Collaborator

According to the comments in bazelbuild/bazel#24180, releasing a fix for this is a Bazel 8 blocker, so I'll prepare a release

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.

2 participants