bazelbuild / rules_python

Bazel Python Rules
https://rules-python.readthedocs.io
Apache License 2.0
521 stars 536 forks source link

Cannot use `runfiles.python` import path when running `bazel coverage` #2009

Open michaelboyd2 opened 3 months ago

michaelboyd2 commented 3 months ago

🐞 bug report

Affected Rule

compile_pip_requirements

Is this a regression?

Not sure.

Description

In some situations it is possible to get a conflict between the python module from the coverage package and the python package from the runfiles library. I would say this is due to the slightly risky use of "python" as a module/package name and the import from python.runfiles import runfiles in dependency_resolver.py.

πŸ”¬ Minimal Reproduction

cd examples/multi_python_versions
bazel coverage --incompatible_default_to_explicit_init_py //requirements:requirements_3_10_test

πŸ”₯ Exception or Error

michaelboyd multi_python_versions $ bazel coverage --incompatible_default_to_explicit_init_py //requirements:requirements_3_10_test
INFO: Using default value for --instrumentation_filter: "^//requirements[/:]".
INFO: Override the above default with --instrumentation_filter
INFO: Analyzed target //requirements:requirements_3_10_test (0 packages loaded, 0 targets configured).
FAIL: //requirements:requirements_3_10_test (see /private/var/tmp/_bazel_michaelboyd/52019027f6106b6e52388536c05b77eb/execroot/_main/bazel-out/darwin_arm64-fastbuild-ST-eaf12dacd369/testlogs/requirements/requirements_3_10_test/test.log)
INFO: From Testing //requirements:requirements_3_10_test:
==================== Test output for //requirements:requirements_3_10_test:
Traceback (most recent call last):
  File "/private/var/tmp/_bazel_michaelboyd/52019027f6106b6e52388536c05b77eb/execroot/_main/bazel-out/darwin_arm64-fastbuild-ST-eaf12dacd369/bin/requirements/requirements_3_10_test.runfiles/rules_python~/python/private/pypi/dependency_resolver/dependency_resolver.py", line 28, in <module>
    from python.runfiles import runfiles
ModuleNotFoundError: No module named 'python.runfiles'; 'python' is not a package

🌍 Your Environment

Operating System:

  
MacOS
  

Output of bazel version:

  
Bazelisk version: development
Build label: 7.2.0
Build target: @@//src/main/java/com/google/devtools/build/lib/bazel:BazelServer
Build time: Mon Jun 10 13:04:55 2024 (1718024695)
Build timestamp: 1718024695
Build timestamp as int: 1718024695  

Rules_python version:

  
Tested on main: ea49937782fb0b969c72625f3b397f4bab53d412
  
michaelboyd2 commented 3 months ago

I feel like this patch fixes the immediate issue:

--- python/pip_install/tools/dependency_resolver/dependency_resolver.py
+++ python/pip_install/tools/dependency_resolver/dependency_resolver.py
@@ -25,7 +25,7 @@ import click
 import piptools.writer as piptools_writer
 from piptools.scripts.compile import cli

-from python.runfiles import runfiles
+from rules_python.python.runfiles import runfiles

 # Replace the os.replace function with shutil.copy to work around os.replace not being able to
 # replace or move files across filesystems.

I could raise a PR with this change if it was thought to be a good idea.

Edit: Now think this may not work with bazlmod where the directory is rules_python~

michaelboyd2 commented 3 months ago

It is also possible to fix with legacy_create_init = True but I feel this shouldn't be needed? This issue implies that this will not be an option long term (although I imagine this may not change quickly).

aignas commented 1 month ago

I think this is the same issue that I saw below:

I found this when working on an unrelated change. This means that technically, a root module could break the non-root module if a toolchain with coverage is added and the non-root module has not verified that everything works with and without coverage, which itself would be annoying. This would only manifest in breakage when executing bazel coverage.