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]: py_library instances from generated repositories are not found by py_test (bzlmod) #276

Closed
wade-arista opened this issue Feb 28, 2024 · 0 comments · Fixed by #299
Closed
Labels
bug Something isn't working untriaged Requires traige

Comments

@wade-arista
Copy link
Contributor

wade-arista commented Feb 28, 2024

What happened?

I'm using bzlmod in bazel 7.0.2.

I created a basic repository_rule that creates a rules_py py_library. I then created a simple test that specifies the external repository in its deps and tries to import this py_library (foo). It doesn't find the python package. If I access other rules_py py_library instances from my _main repo, instead, those are found as expected.

graph TD;
test(:test) --> baz(direct:baz)
test --> foo("@myrepo//:foo")
test --> bar("@myrepo//bar")
    subgraph "@myrepo"
    foo
    bar
    end
Loading

Version

Development (host) and target OS/architectures: linux x86_64 "Ubuntu 22.04.3 LTS"

Output of bazel --version:

Bazelisk version: v1.19.0
Build label: 7.0.2
Build target: @@//src/main/java/com/google/devtools/build/lib/bazel:BazelServer
Build time: Thu Jan 25 16:13:35 2024 (1706199215)
Build timestamp: 1706199215
Build timestamp as int: 1706199215

Version of the Aspect rules, or other relevant rules from your
WORKSPACE or MODULE.bazel file:

bazel_dep(name = "rules_python", version = "0.31.0")
bazel_dep(name = "aspect_rules_py", version = "0.5.0")

Language(s) and/or frameworks involved:
Python3

How to reproduce

I have a minimal repro here:

https://github.com/wade-arista/bazel-demo/tree/63fc4391a7c30be0587a642649b0c16514fab302/rules_py_reporule

test:

import baz
import foo
import bar

BUILD.bazel

load("@aspect_rules_py//py:defs.bzl", "py_test")

py_test(
    name = "test",
    srcs = ["test.py"],
    deps = [
        "//rules_py_reporule/direct/baz",
        "@myrepo//:foo",
        "@myrepo//bar",
    ],
)

py_test(
    name = "all_direct",
    srcs = ["test.py"],
    main = "test.py",
    deps = [
        "//rules_py_reporule/direct/baz",
        "//rules_py_reporule/imported:foo",
        "//rules_py_reporule/imported/bar",
    ],
)

for @myrepo:

importer = use_extension("//rules_py_reporule/rules:import.bzl", "importer")
use_repo(importer, "myrepo")

Any other information?

Using ctx.label.workspace_name when making the import path, when it is non-empty, fixes the issue.

--- a/py/private/py_library.bzl
+++ b/py/private/py_library.bzl
@@ -86,7 +86,7 @@ def _make_import_path(label, workspace, base, imp):
 def _make_imports_depset(ctx):
     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 ctx.attr.imports
     ] + [
         # Add the workspace name in the imports such that repo-relative imports work.

Here's the contents of the test's runfiles directory:

./bazel-out/k8-fastbuild/bin/rules_py_reporule/test.runfiles/test.venv
./bazel-out/k8-fastbuild/bin/rules_py_reporule/test.runfiles/_main~importer~myrepo
./bazel-out/k8-fastbuild/bin/rules_py_reporule/test.runfiles/bazel_tools
./bazel-out/k8-fastbuild/bin/rules_py_reporule/test.runfiles/rules_python~0.31.0~python~python_3_11_x86_64-unknown-linux-gnu
./bazel-out/k8-fastbuild/bin/rules_py_reporule/test.runfiles/_main

So in this case, ctx.label.workspace_name is _main~importer~myrepo for e.g. @myrepo//bar, but ctx.workspace_name is just _main, so we need to use the former when building the runfiles import paths.

@wade-arista wade-arista added the bug Something isn't working label Feb 28, 2024
@github-actions github-actions bot added the untriaged Requires traige label Feb 28, 2024
wade-arista added a commit to wade-arista/rules_py that referenced this issue Feb 28, 2024
Fix for aspect-build#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.
wade-arista added a commit to wade-arista/rules_py that referenced this issue Feb 28, 2024
Fix for aspect-build#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.
wade-arista added a commit to wade-arista/rules_py that referenced this issue Feb 28, 2024
Fix for aspect-build#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.
wade-arista added a commit to wade-arista/rules_py that referenced this issue Mar 5, 2024
Fix for aspect-build#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.
wade-arista added a commit to wade-arista/rules_py that referenced this issue Mar 5, 2024
Fix for aspect-build#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.
alexeagle pushed a commit that referenced this issue Mar 5, 2024
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.

---

### Test plan
- New test case added in `e2e/repository-rule-deps`.
- Manually tested against bazel 6.5 and 7.0.2.

Fixes: #276
@github-project-automation github-project-automation bot moved this to ✅ Done in Open Source Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working untriaged Requires traige
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

1 participant