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

fix: use correct runfiles path for venv import paths #299

Merged
merged 2 commits into from
Mar 5, 2024

Conversation

wade-arista
Copy link
Contributor

@wade-arista wade-arista commented 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

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.
@CLAassistant
Copy link

CLAassistant commented Mar 5, 2024

CLA assistant check
All committers have signed the CLA.

@wade-arista
Copy link
Contributor Author

wade-arista commented Mar 5, 2024

I would like to see that the new e2e/repository-rule-deps is properly executed via the new "repository-rule-deps" check before this merges as well. I don't expect that I have permissions to run this, though.

Also, once bzlmod is enabled for . in the CI jobs, the tests can be moved out of e2e and executed directly.

Copy link
Member

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, nicely documented!

.github/workflows/ci.yaml Outdated Show resolved Hide resolved
As Alex pointed out:

> this doesn't need a new job, you can just add to the `folders` array
> of the `test` job above
@alexeagle alexeagle enabled auto-merge (squash) March 5, 2024 21:36
@alexeagle alexeagle merged commit 862a434 into aspect-build:main Mar 5, 2024
12 checks passed
@scasagrande
Copy link
Contributor

thanks @wade-arista , I was in the process of fixing this same issue, but you did a much better job than I had time for 😄

@wade-arista
Copy link
Contributor Author

wade-arista commented Mar 7, 2024

thanks @wade-arista , I was in the process of fixing this same issue, but you did a much better job than I had time for 😄

sure thing! Sorry, I had meant to post a link to discourse on #276 where we were discussing a PR, but then I forgot.

Hopefully this is the same thing fix you had in mind?

@scasagrande
Copy link
Contributor

scasagrande commented Mar 7, 2024

Yeah I believe this covers my case but I will have to double check just to make sure.

I did notice a further issue in my testing. After patching importing a py_libary from another workspace, I saw that the dependencies of this library not being picked up correctly. I'll take a look if that is still the case and raise a separate issue if so.

mattem pushed a commit that referenced this pull request Mar 19, 2024
---

### Type of change

- Bug fix (change which fixes an issue)

### Test plan

- Covered by existing test cases

### Details

As a follow-up to #299 ,
this PR enables the use of external dependencies that use workspace root
relative imports.

This just expands current functionality to also cover the case where the
dependency is from an external workspace.
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 this pull request may close these issues.

[Bug]: py_library instances from generated repositories are not found by py_test (bzlmod)
4 participants