From c285780d5dacd420fa2b6530d6309e55b541ad93 Mon Sep 17 00:00:00 2001 From: Wade Carpenter Date: Tue, 27 Feb 2024 17:18:08 -0800 Subject: [PATCH] fix: use correct runfiles path for venv import paths Fix for #276. In `_make_import_path`, `ctx.workspace_name` is always `_main` when evaluating the deps of targets in the main repo. This includes deps that are in external repos, e.g. importer = use_extension("//:import.bzl", "importer") use_repo(importer, "myrepo") with py_test(..., deps = ["@myrepo//foo"]) In this case, we actually want to use the label's `workspace_name` attr which will be the same as the runfiles path that we need for the venv import paths. Also added a new e2e test that creates this external repo and verifies the fix. Without the fix in place, the test does fail as described in the e2e/repository-rule-deps/README.md "Repro" section. --- .github/workflows/ci.yaml | 7 ++++ e2e/repository-rule-deps/.bazelrc | 7 ++++ e2e/repository-rule-deps/.bazelversion | 1 + e2e/repository-rule-deps/BUILD.bazel | 34 +++++++++++++++++ e2e/repository-rule-deps/MODULE.bazel | 37 ++++++++++++++++++ e2e/repository-rule-deps/README.md | 38 +++++++++++++++++++ e2e/repository-rule-deps/direct/BUILD.bazel | 1 + .../direct/directmod/BUILD.bazel | 8 ++++ .../direct/directmod/__init__.py | 1 + e2e/repository-rule-deps/imported/BUILD.bazel | 10 +++++ .../imported/flat/BUILD.bazel | 10 +++++ .../imported/flat/__init__.py | 1 + .../imported/subdir/__init__.py | 1 + e2e/repository-rule-deps/rules/BUILD.bazel | 4 ++ e2e/repository-rule-deps/rules/import.bzl | 30 +++++++++++++++ e2e/repository-rule-deps/test.py | 28 ++++++++++++++ e2e/repository-rule-deps/toplevel/BUILD.bazel | 7 ++++ e2e/repository-rule-deps/toplevel/__init__.py | 1 + py/private/py_library.bzl | 2 +- 19 files changed, 227 insertions(+), 1 deletion(-) create mode 100644 e2e/repository-rule-deps/.bazelrc create mode 100644 e2e/repository-rule-deps/.bazelversion create mode 100644 e2e/repository-rule-deps/BUILD.bazel create mode 100644 e2e/repository-rule-deps/MODULE.bazel create mode 100644 e2e/repository-rule-deps/README.md create mode 100644 e2e/repository-rule-deps/direct/BUILD.bazel create mode 100644 e2e/repository-rule-deps/direct/directmod/BUILD.bazel create mode 100644 e2e/repository-rule-deps/direct/directmod/__init__.py create mode 100644 e2e/repository-rule-deps/imported/BUILD.bazel create mode 100644 e2e/repository-rule-deps/imported/flat/BUILD.bazel create mode 100644 e2e/repository-rule-deps/imported/flat/__init__.py create mode 100644 e2e/repository-rule-deps/imported/subdir/__init__.py create mode 100644 e2e/repository-rule-deps/rules/BUILD.bazel create mode 100644 e2e/repository-rule-deps/rules/import.bzl create mode 100755 e2e/repository-rule-deps/test.py create mode 100644 e2e/repository-rule-deps/toplevel/BUILD.bazel create mode 100644 e2e/repository-rule-deps/toplevel/__init__.py diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 0e48ce9f..8f71adcc 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -30,6 +30,13 @@ jobs: {"folder": ".", "bzlmodEnabled": true} ] + repository-rule-deps: + uses: bazel-contrib/.github/.github/workflows/bazel.yaml@v6 + with: + folders: '["e2e/repository-rule-deps"]' + exclude: | + [{"bzlmodEnabled":false}] + verify-bcr-patches: runs-on: ubuntu-latest steps: diff --git a/e2e/repository-rule-deps/.bazelrc b/e2e/repository-rule-deps/.bazelrc new file mode 100644 index 00000000..70c1feb8 --- /dev/null +++ b/e2e/repository-rule-deps/.bazelrc @@ -0,0 +1,7 @@ +common --test_output=all + +# The test only works with bzlmod enabled. +common --enable_bzlmod + +# The imports=[".."] only works properly when this matches the python toolchain major version. +common --@aspect_rules_py//py:interpreter_version=3.11.6 diff --git a/e2e/repository-rule-deps/.bazelversion b/e2e/repository-rule-deps/.bazelversion new file mode 100644 index 00000000..a8907c02 --- /dev/null +++ b/e2e/repository-rule-deps/.bazelversion @@ -0,0 +1 @@ +7.0.2 diff --git a/e2e/repository-rule-deps/BUILD.bazel b/e2e/repository-rule-deps/BUILD.bazel new file mode 100644 index 00000000..6ed23e6f --- /dev/null +++ b/e2e/repository-rule-deps/BUILD.bazel @@ -0,0 +1,34 @@ +load("@aspect_rules_py//py:defs.bzl", "py_test") + +all_modules = [ + "directmod", + "flat", + "subdir", + "toplevel", +] + +always_deps = [ + "//direct/directmod", + "//toplevel", +] + +py_test( + name = "test", + srcs = ["test.py"], + args = all_modules, + deps = always_deps + [ + "@myrepo//:subdir", + "@myrepo//flat", + ], +) + +py_test( + name = "all_direct", + srcs = ["test.py"], + args = all_modules, + main = "test.py", + deps = always_deps + [ + "//imported:subdir", + "//imported/flat", + ], +) diff --git a/e2e/repository-rule-deps/MODULE.bazel b/e2e/repository-rule-deps/MODULE.bazel new file mode 100644 index 00000000..b5a3c1f2 --- /dev/null +++ b/e2e/repository-rule-deps/MODULE.bazel @@ -0,0 +1,37 @@ +"Bazel dependencies" + +bazel_dep(name = "aspect_rules_py", version = "0.0.0", dev_dependency = True) +local_path_override( + module_name = "aspect_rules_py", + path = "../..", +) + +# Because we use a prerelease of rules_py, we must compile the rust tools from source. +bazel_dep(name = "rules_rust", version = "0.40.0") + +rust = use_extension("@rules_rust//rust:extensions.bzl", "rust") +rust.toolchain( + edition = "2021", + versions = ["1.74.1"], +) +use_repo(rust, "rust_toolchains") + +register_toolchains("@rust_toolchains//:all") + +#---SNIP--- Below here is re-used in the snippet published on releases +# Minimum version needs: +# feat: add interpreter_version_info to py_runtime by @mattem in #1671 +bazel_dep(name = "rules_python", version = "0.29.0", dev_dependency = True) + +python = use_extension("@rules_python//python/extensions:python.bzl", "python") +python.toolchain( + configure_coverage_tool = True, + python_version = "3.11.6", +) + +# test case for py_library in an imported / generated repository +bazel_dep(name = "bazel_skylib", version = "1.5.0") + +importer = use_extension("//rules:import.bzl", "importer") +use_repo(importer, "myrepo") +# end test case diff --git a/e2e/repository-rule-deps/README.md b/e2e/repository-rule-deps/README.md new file mode 100644 index 00000000..f2bd69ec --- /dev/null +++ b/e2e/repository-rule-deps/README.md @@ -0,0 +1,38 @@ +# Test Case: py_library in generated / external repository + +See [#276](https://github.com/aspect-build/rules_py/issues/276). + +## Structure + +```mermaid +graph TD; + +test(:test) --> directmod(direct:directmod) +test --> subdir("@myrepo//:subdir") +test --> flat("@myrepo//bar") + subgraph "@myrepo" + subdir + flat + end +``` + +## Subdirs + +- [./BUILD.bazel](./BUILD.bazel) contains the demonstration `py_test` that makes use of the modules. +- [rules/](rules/) contains the "importer" rule used to create the external repository under test. It simply copies the files from the fixed path `./imported`. +- [direct/](direct/) counterexample showing that a `py_library` from within the `_main` repository is working as expected. +- [imported/](imported/) contains the contents of the external repository to be imported. +- [toplevel/](toplevel/) contains a simple python package defined at the same level as the test (no `imports` required). + +## Repro + +The `:test` test case imports two modules from `@myrepo` ("flat" using `imports=["."]` and "subdir" using `imports=[".."]`), and two from `_main`. The packages from `_main` are correctly seen by python while the other two are not. + +The second test case `:all_direct` skips the `repository_rule` to demonstrate that the `test.py` itself is correctly implemented and can actually pass when the bug is not present. + + +```console +$ bazel test ... +... +AssertionError: import errors: ["No module named 'flat'", "No module named 'subdir'"] +``` diff --git a/e2e/repository-rule-deps/direct/BUILD.bazel b/e2e/repository-rule-deps/direct/BUILD.bazel new file mode 100644 index 00000000..ba38a01c --- /dev/null +++ b/e2e/repository-rule-deps/direct/BUILD.bazel @@ -0,0 +1 @@ +## diff --git a/e2e/repository-rule-deps/direct/directmod/BUILD.bazel b/e2e/repository-rule-deps/direct/directmod/BUILD.bazel new file mode 100644 index 00000000..e7ed3e96 --- /dev/null +++ b/e2e/repository-rule-deps/direct/directmod/BUILD.bazel @@ -0,0 +1,8 @@ +load("@aspect_rules_py//py:defs.bzl", "py_library") + +py_library( + name = "directmod", + srcs = ["__init__.py"], + imports = [".."], + visibility = ["//visibility:public"], +) diff --git a/e2e/repository-rule-deps/direct/directmod/__init__.py b/e2e/repository-rule-deps/direct/directmod/__init__.py new file mode 100644 index 00000000..31aee9e6 --- /dev/null +++ b/e2e/repository-rule-deps/direct/directmod/__init__.py @@ -0,0 +1 @@ +name = "directmod" diff --git a/e2e/repository-rule-deps/imported/BUILD.bazel b/e2e/repository-rule-deps/imported/BUILD.bazel new file mode 100644 index 00000000..56e2169c --- /dev/null +++ b/e2e/repository-rule-deps/imported/BUILD.bazel @@ -0,0 +1,10 @@ +load("@aspect_rules_py//py:defs.bzl", "py_library") + +py_library( + name = "subdir", + srcs = [ + "subdir/__init__.py", + ], + imports = ["."], + visibility = ["//visibility:public"], +) diff --git a/e2e/repository-rule-deps/imported/flat/BUILD.bazel b/e2e/repository-rule-deps/imported/flat/BUILD.bazel new file mode 100644 index 00000000..0064629b --- /dev/null +++ b/e2e/repository-rule-deps/imported/flat/BUILD.bazel @@ -0,0 +1,10 @@ +load("@aspect_rules_py//py:defs.bzl", "py_library") + +py_library( + name = "flat", + srcs = [ + "__init__.py", + ], + imports = [".."], + visibility = ["//visibility:public"], +) diff --git a/e2e/repository-rule-deps/imported/flat/__init__.py b/e2e/repository-rule-deps/imported/flat/__init__.py new file mode 100644 index 00000000..cdb2df8a --- /dev/null +++ b/e2e/repository-rule-deps/imported/flat/__init__.py @@ -0,0 +1 @@ +name = "flat" diff --git a/e2e/repository-rule-deps/imported/subdir/__init__.py b/e2e/repository-rule-deps/imported/subdir/__init__.py new file mode 100644 index 00000000..07784d2e --- /dev/null +++ b/e2e/repository-rule-deps/imported/subdir/__init__.py @@ -0,0 +1 @@ +name = "subdir" diff --git a/e2e/repository-rule-deps/rules/BUILD.bazel b/e2e/repository-rule-deps/rules/BUILD.bazel new file mode 100644 index 00000000..6b0f0598 --- /dev/null +++ b/e2e/repository-rule-deps/rules/BUILD.bazel @@ -0,0 +1,4 @@ +exports_files( + srcs = ["import.bzl"], + visibility = ["//visibility:public"], +) diff --git a/e2e/repository-rule-deps/rules/import.bzl b/e2e/repository-rule-deps/rules/import.bzl new file mode 100644 index 00000000..785d84e3 --- /dev/null +++ b/e2e/repository-rule-deps/rules/import.bzl @@ -0,0 +1,30 @@ +load("@bazel_skylib//lib:paths.bzl", "paths") + +def _myrepo_impl(repository_ctx): + p = repository_ctx.workspace_root.get_child(repository_ctx.attr.path) + find_cmd = ["find", str(p), "-maxdepth", "1", "-mindepth", "1"] + r = repository_ctx.execute(find_cmd) + if r.return_code != 0: + fail(r.stdout + "\n" + r.stderr) + + files = r.stdout.splitlines() + for f in files: + repository_ctx.symlink(f, paths.basename(f)) + + r = repository_ctx.execute(find_cmd) + if r.return_code != 0: + fail(r.stdout + "\n" + r.stderr) + +myrepo = repository_rule( + implementation = _myrepo_impl, + attrs = { + "path": attr.string(mandatory = True), + }, +) + +def _importer_impl(module_ctx): + myrepo(name = "myrepo", path = "imported") + +importer = module_extension( + implementation = _importer_impl, +) diff --git a/e2e/repository-rule-deps/test.py b/e2e/repository-rule-deps/test.py new file mode 100755 index 00000000..40cfbfe6 --- /dev/null +++ b/e2e/repository-rule-deps/test.py @@ -0,0 +1,28 @@ +import importlib +import sys + + +def main(): + print("sys.path entries:") + for p in sys.path: + print(p) + errors = [] + mod_names = sys.argv[1:] + mods = {} + for name in mod_names: + try: + mods[name] = importlib.import_module(name) + except ImportError as e: + errors.append(e) + + assert not errors, f"import errors: {[str(e) for e in errors]}" + + for name in mod_names: + expected = name + actual = mods[name].name + print(f"{name}.name = ", mods[name].name) + assert expected == actual, f"expected: {expected!r}, actual: {actual!r}" + + +if __name__ == "__main__": + main() diff --git a/e2e/repository-rule-deps/toplevel/BUILD.bazel b/e2e/repository-rule-deps/toplevel/BUILD.bazel new file mode 100644 index 00000000..c60640a1 --- /dev/null +++ b/e2e/repository-rule-deps/toplevel/BUILD.bazel @@ -0,0 +1,7 @@ +load("@aspect_rules_py//py:defs.bzl", "py_library") + +py_library( + name = "toplevel", + srcs = ["__init__.py"], + visibility = ["//visibility:public"], +) diff --git a/e2e/repository-rule-deps/toplevel/__init__.py b/e2e/repository-rule-deps/toplevel/__init__.py new file mode 100644 index 00000000..989821aa --- /dev/null +++ b/e2e/repository-rule-deps/toplevel/__init__.py @@ -0,0 +1 @@ +name = "toplevel" diff --git a/py/private/py_library.bzl b/py/private/py_library.bzl index f965a258..26351254 100644 --- a/py/private/py_library.bzl +++ b/py/private/py_library.bzl @@ -130,7 +130,7 @@ def _make_import_path(label, workspace, base, imp): def _make_imports_depset(ctx, imports = [], extra_imports_depsets = []): base = paths.dirname(ctx.build_file_path) import_paths = [ - _make_import_path(ctx.label, ctx.workspace_name, base, im) + _make_import_path(ctx.label, ctx.label.workspace_name or ctx.workspace_name, base, im) for im in getattr(ctx.attr, "imports", imports) ] + [ # Add the workspace name in the imports such that repo-relative imports work.