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

Loading top-level targets in local_path_override modules in child directory breaks the build #22208

Open
mbland opened this issue May 1, 2024 · 9 comments
Labels
area-Bzlmod Bzlmod-specific PRs, issues, and feature requests P2 We'll consider working on this in future. (Assignee optional) team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. type: bug

Comments

@mbland
Copy link

mbland commented May 1, 2024

Description of the bug:

Using a Bazel module within a child directory (i.e., in the same repository, via local_path_override()) that calls load() on a top-level target breaks the build. The equivalent build using local_repository() in a WORKSPACE file succeeds.

In my mbland/bzlmod-local-module-bug example repo:

$ bazel build --nobuild --enable_bzlmod=false //...

INFO: Analyzed 0 targets (1 packages loaded, 0 targets configured).
INFO: Found 0 targets...
INFO: Elapsed time: 0.099s, Critical Path: 0.00s
INFO: 0 processes.
INFO: Build completed successfully, 0 total actions

$ bazel build --nobuild --enable_bzlmod=true //...

WARNING: Target pattern parsing failed.
ERROR: Skipping '//...': error loading package under directory '': error loading package 'third_party/vendored_module': cannot load '//:top_level_target.bzl': no such file
ERROR: error loading package under directory '': error loading package 'third_party/vendored_module': cannot load '//:top_level_target.bzl': no such file
INFO: Elapsed time: 0.054s
INFO: 0 processes.
ERROR: Build did NOT complete successfully

local_path_override() and the bzmlod mechanism seem to assume the module will not reside within the same repo. The WORKSPACE or MODULE.bazel repository boundary marker files don't appear to have an effect.

Please see the README.md in my example repo for more information.

Which category does this issue belong to?

Core

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

git clone https://github.com/mbland/bzlmod-local-module-bug.git
cd bzlmod-local-module-bug
bazel build --nobuild --enable_bzlmod=true //...

Which operating system are you running Bazel on?

macOS 14.4.1 (23E224)

What is the output of bazel info release?

release 8.0.0-pre.20240415.1

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

No response

What's the output of git remote get-url origin; git rev-parse HEAD ?

git@github.com:mbland/bzlmod-local-module-bug.git
31f1f966f4f2f1ef1c27cfe8ddf77a102fccdfa0

Is this a regression? If yes, please try to identify the Bazel commit where the bug was introduced.

No. This happens in the current 7.1.1 release, as well as 7.0.0, 6.5.0, and 6.0.0.

Have you found anything relevant by searching the web?

No. All local_path_override() examples and issues I found presume the local module is outside the current directory.

Any other information, logs, or outputs that you want to share?

No response

@github-actions github-actions bot added the team-Core Skyframe, bazel query, BEP, options parsing, bazelrc label May 1, 2024
@fmeum
Copy link
Collaborator

fmeum commented May 1, 2024

Is this fixed by adding third_party to .bazelignore? It looks like Bazel recurses into the submodule.

@mbland
Copy link
Author

mbland commented May 1, 2024

It does seem to fix my example repo; I'll have to try it in my actual project later to confirm.

But if .bazelignore wasn't necessary for the WORKSPACE version to work, should that advice become part of the bzlmod migration "Introduce local repositories" advice? (Specifically for local_path_override() instances within the same directory/repo.)

@fmeum
Copy link
Collaborator

fmeum commented May 1, 2024

We should probably just fix this and align the behavior with that of your WORKSPACE example.

@Wyverald Wyverald added team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. area-Bzlmod Bzlmod-specific PRs, issues, and feature requests P2 We'll consider working on this in future. (Assignee optional) and removed team-Core Skyframe, bazel query, BEP, options parsing, bazelrc untriaged labels May 1, 2024
@Wyverald
Copy link
Member

Wyverald commented May 1, 2024

I think this is because we have some special handling to check if there's a WORKSPACE local_repository defined for a directory in the main repo. No such logic exists for the Bzlmod equivalent, hence the failure.

This is related to #21515. (or maybe even just a dupe)

@mbland
Copy link
Author

mbland commented May 13, 2024

Can confirm the .bazelignore workaround works for my main project.

@Wyverald
Copy link
Member

Looking at this again due to #22754. I verified that in the WORKSPACE world, the package @@//third_party/vendored_module is simply considered not to exist, because there's a local_repository definition that points to that directory. This is consequently what causes the target pattern //... to not contain this package, because (well) it doesn't exist.

In the Bzlmod world, we don't have such a check. Implementing such a check can also be difficult, since what is local_repository now? Sure, local_path_override is currently backed by the native repo rule local_repository, but what about users of the new Starlarkified local_repository in bazel_tools? What about a homemade repo rule that refers to a path inside the main repo? What's even worse -- to find out if anyone has defined a local_repository, we technically need to run all module extensions unconditionally, which is kind of a non-starter.

cc @katre, who originally implemented the cross-repo check, for ideas.

@katre
Copy link
Member

katre commented Jun 18, 2024

When I implemented LocalRepositoryLookupFunction, the goal was "don't allow //... to recurse into things that are already part of other repositories". At the time, the only "other repository" that could exist inside the main repository was a local_repository with a relative path.

The implementation is actually kind of stupid: it doesn't check whether a directory is actually a repository, it just checks if there's a WORKSPACE file. It'd be equivalent to look for a REPO.bazel or MODULE.bazel file and declare that directory a sub-module. Or do local_path_override modules not have these marker files?

@mbland
Copy link
Author

mbland commented Jun 18, 2024

One other facet to the conversation: Just today I stumbled upon Vendor Mode in the documentation. I also stumbled on #19563 and #20366, which indicate it's not quite ready yet (at least for Bzlmod).

Would a VENDOR.bazel configuration eventually supplant local_repository() and local_path_override() for what I'm trying to do here? Or is it completely orthogonal?


Also, FWIW, I did get a report that using .bazelignore to hide some local repos imported by rules_jvm_external ended up breaking IDE autocompletion. Not raising that as a new issue, just documenting a side effect of the .bazelignore workaround for general awareness.

@Wyverald
Copy link
Member

The implementation is actually kind of stupid: it doesn't check whether a directory is actually a repository, it just checks if there's a WORKSPACE file. It'd be equivalent to look for a REPO.bazel or MODULE.bazel file and declare that directory a sub-module. Or do local_path_override modules not have these marker files?

Not sure if this was changed after your initial implementation, but LRLF does more than just check for existence of a WORKSAPCE file under a directory; it also checks whether a local_repository is defined to point to that directory (see #22208 (comment) again).

In the Bzlmod world we do indeed have REPO.bazel or MODULE.bazel etc, but I was wondering if just checking for the existence of these files would be sufficient or too much.

copybara-service bot pushed a commit that referenced this issue Jun 20, 2024
…le_workspace`

This still leaves the question of "what do we do instead?". See issues #22208 and #21515.

Fixes #22754.

Closes #22774.

PiperOrigin-RevId: 645148811
Change-Id: Ib9d07d2ecbc3a79e3341de6739de1c3349124d6b
bazel-io pushed a commit to bazel-io/bazel that referenced this issue Jun 20, 2024
…le_workspace`

This still leaves the question of "what do we do instead?". See issues bazelbuild#22208 and bazelbuild#21515.

Fixes bazelbuild#22754.

Closes bazelbuild#22774.

PiperOrigin-RevId: 645148811
Change-Id: Ib9d07d2ecbc3a79e3341de6739de1c3349124d6b
github-merge-queue bot pushed a commit that referenced this issue Jun 20, 2024
…--noenable_workspace` (#22837)

This still leaves the question of "what do we do instead?". See issues
#22208 and #21515.

Fixes #22754.

Closes #22774.

PiperOrigin-RevId: 645148811
Change-Id: Ib9d07d2ecbc3a79e3341de6739de1c3349124d6b

Commit
1246ff4

Co-authored-by: Xdng Yng <wyverald@gmail.com>
iancha1992 pushed a commit to iancha1992/bazel that referenced this issue Jun 21, 2024
…le_workspace`

This still leaves the question of "what do we do instead?". See issues bazelbuild#22208 and bazelbuild#21515.

Fixes bazelbuild#22754.

Closes bazelbuild#22774.

PiperOrigin-RevId: 645148811
Change-Id: Ib9d07d2ecbc3a79e3341de6739de1c3349124d6b
bazel-io pushed a commit to bazel-io/bazel that referenced this issue Jun 27, 2024
…le_workspace`

This still leaves the question of "what do we do instead?". See issues bazelbuild#22208 and bazelbuild#21515.

Fixes bazelbuild#22754.

Closes bazelbuild#22774.

PiperOrigin-RevId: 645148811
Change-Id: Ib9d07d2ecbc3a79e3341de6739de1c3349124d6b
github-merge-queue bot pushed a commit that referenced this issue Jun 28, 2024
…--noenable_workspace` (#22911)

This still leaves the question of "what do we do instead?". See issues
#22208 and #21515.

Fixes #22754.

Closes #22774.

PiperOrigin-RevId: 645148811
Change-Id: Ib9d07d2ecbc3a79e3341de6739de1c3349124d6b

Commit
1246ff4

Co-authored-by: Xdng Yng <wyverald@gmail.com>
fmeum pushed a commit to fmeum/bazel that referenced this issue Jul 18, 2024
…--noenable_workspace` (bazelbuild#22837)

This still leaves the question of "what do we do instead?". See issues
bazelbuild#22208 and bazelbuild#21515.

Fixes bazelbuild#22754.

Closes bazelbuild#22774.

PiperOrigin-RevId: 645148811
Change-Id: Ib9d07d2ecbc3a79e3341de6739de1c3349124d6b

Commit
bazelbuild@1246ff4

Co-authored-by: Xdng Yng <wyverald@gmail.com>
mbland added a commit to EngFlow/example that referenced this issue Aug 8, 2024
Requested by @laszlocsomor during review of the upcoming EngFlow blog
post: "Migrating to Bazel Modules (a.k.a. Bzlmod) - Repo Names and
Runfiles".

The .bazelignore file is required for now to prevent `bazel build //...`
from recursing into the modules under `runfiles/` pending resolution of:

- bazelbuild/bazel#22208
- bazelbuild/bazel#21515
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Bzlmod Bzlmod-specific PRs, issues, and feature requests P2 We'll consider working on this in future. (Assignee optional) team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. type: bug
Projects
None yet
Development

No branches or pull requests

7 participants