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

Bazel crashes using external aspect + --noenable_workspace #22691

Closed
ghost opened this issue Jun 11, 2024 · 15 comments
Closed

Bazel crashes using external aspect + --noenable_workspace #22691

ghost opened this issue Jun 11, 2024 · 15 comments
Labels
P2 We'll consider working on this in future. (Assignee optional) team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. type: bug

Comments

@ghost
Copy link

ghost commented Jun 11, 2024

Description of the bug:

With bzlmod and the --noenable_workspace option, Bazel crashes when we try to use an external aspect defined by the --aspects and --override_repository options.

$ bazel build \
   --enable_bzlmod \
   --noenable_workspace \
   --aspects=@@foo//:foo.bzl%foo_aspect \
   --override_repository=foo=%workspace%/foo \
   //...
Analyzing: target //:dummy (1 packages loaded, 0 targets configured)
FATAL: bazel crashed due to an internal error. Printing stack trace:
java.lang.IllegalStateException: Evaluation of SkyKey failed and no dependencies were requested: KeyForBuild{label=@@foo//:foo.bzl, isBuildPrelude=false} IncrementalInMemoryNodeEntry{key=KeyForBuild{label=@@foo//:foo.bzl, isBuildPrelude=false}, value=null, dirtyBuildingState=InitialBuildingState{dirtyState=REBUILDING, signaledDeps=5, externalDeps=0, dirtyDirectDepIndex=0}, version=com.google.devtools.build.skyframe.Version$MinimalVersion@5937f9c0, directDeps=GroupedDeps{size=5, elements=[Starlark @_builtins, CONTAINING_PACKAGE_LOOKUP:@@foo//, FILE:[<output_base>/external/foo]/[foo.bzl], PRECOMPUTED:starlark_semantics, Key{repoName=@@foo, rootModuleShouldSeeWorkspaceRepos=true}]}, reverseDeps=ReverseDeps{reverseDeps=[], singleReverseDep=false, dataToConsolidate=[BuildTopLevelAspectsDetailsKey{topLevelAspectsClasses=[@@foo//:foo.bzl%foo_aspect], topLevelAspectsParameters={}}]}}
	at com.google.common.base.Preconditions.checkState(Preconditions.java:835)
	at com.google.devtools.build.skyframe.AbstractParallelEvaluator$Evaluate.run(AbstractParallelEvaluator.java:690)
	at com.google.devtools.build.lib.concurrent.AbstractQueueVisitor$WrappedRunnable.run(AbstractQueueVisitor.java:414)
	at java.base/java.util.concurrent.ForkJoinTask$RunnableExecuteAction.exec(Unknown Source)
	at java.base/java.util.concurrent.ForkJoinTask.doExec(Unknown Source)
	at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(Unknown Source)
	at java.base/java.util.concurrent.ForkJoinPool.scan(Unknown Source)
	at java.base/java.util.concurrent.ForkJoinPool.runWorker(Unknown Source)
	at java.base/java.util.concurrent.ForkJoinWorkerThread.run(Unknown Source)
  • No problem with --enable_workspace option.
  • No problem when passing a missing bzl file in the --aspects option, e.g --aspects=@@foo//:xxx.bzl%foo_aspect.
  • Same problem when passing a non exported symbol in the --aspects option, e.g --aspects=@@foo//:foo.bzl%xxx.
  • Same problem with Bazel 7.1.2.

Which category does this issue belong to?

Core, External Dependency

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

.
├── .bazelversion
├── BUILD.bazel
├── foo
│   ├── BUILD.bazel
│   ├── foo.bzl
│   └── WORKSPACE
├── MODULE.bazel
├── WORKSPACE
└── WORKSPACE.bzlmod

.bazelversion

7.2.0

BUILD.bazel

genrule(
    name = "dummy",
    outs = ["dummy.txt"],
    cmd = "touch $@",
)

foo/foo.bzl

def _foo_aspect_impl(target, ctx):
    return []

foo_aspect = aspect(
    implementation = _foo_aspect_impl,
)

foo/WORKSPACE

workspace(name = "foo")

MODULE.bazel

module(name = "my_workspace")

WORKSPACE

workspace(name = "my_workspace")

$ bazel build \
  --enable_bzlmod \
  --noenable_workspace \
  --lockfile_mode=off \
  --aspects=@@foo//:foo.bzl%foo_aspect \
  --override_repository=foo=%workspace%/foo \
  -- //...
Starting local Bazel server and connecting to it...
Analyzing: target //:dummy (2 packages loaded, 0 targets configured)
FATAL: bazel crashed due to an internal error. Printing stack trace:
java.lang.IllegalStateException: Evaluation of SkyKey failed and no dependencies were requested: KeyForBuild{label=@@foo//:foo.bzl, isBuildPrelude=false} IncrementalInMemoryNodeEntry{key=KeyForBuild{label=@@foo//:foo.bzl, isBuildPrelude=false}, value=null, dirtyBuildingState=InitialBuildingState{dirtyState=REBUILDING, signaledDeps=5, externalDeps=0, dirtyDirectDepIndex=0}, version=com.google.devtools.build.skyframe.Version$MinimalVersion@5ba14436, directDeps=GroupedDeps{size=5, elements=[Starlark @_builtins, CONTAINING_PACKAGE_LOOKUP:@@foo//, FILE:[<output_base>/external/foo]/[foo.bzl], PRECOMPUTED:starlark_semantics, Key{repoName=@@foo, rootModuleShouldSeeWorkspaceRepos=true}]}, reverseDeps=ReverseDeps{reverseDeps=[], singleReverseDep=false, dataToConsolidate=[BuildTopLevelAspectsDetailsKey{topLevelAspectsClasses=[@@foo//:foo.bzl%foo_aspect], topLevelAspectsParameters={}}]}}
	at com.google.common.base.Preconditions.checkState(Preconditions.java:835)
	at com.google.devtools.build.skyframe.AbstractParallelEvaluator$Evaluate.run(AbstractParallelEvaluator.java:690)
	at com.google.devtools.build.lib.concurrent.AbstractQueueVisitor$WrappedRunnable.run(AbstractQueueVisitor.java:414)
	at java.base/java.util.concurrent.ForkJoinTask$RunnableExecuteAction.exec(Unknown Source)
	at java.base/java.util.concurrent.ForkJoinTask.doExec(Unknown Source)
	at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(Unknown Source)
	at java.base/java.util.concurrent.ForkJoinPool.scan(Unknown Source)
	at java.base/java.util.concurrent.ForkJoinPool.runWorker(Unknown Source)
	at java.base/java.util.concurrent.ForkJoinWorkerThread.run(Unknown Source)

Which operating system are you running Bazel on?

Linux / amd64

What is the output of bazel info release?

release 7.2.0

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 ?

No response

If this is a regression, please try to identify the Bazel commit where the bug was introduced with bazelisk --bisect.

No response

Have you found anything relevant by searching the web?

No response

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

No response

@github-actions github-actions bot added team-Core Skyframe, bazel query, BEP, options parsing, bazelrc team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. labels Jun 11, 2024
@Wyverald Wyverald removed the team-Core Skyframe, bazel query, BEP, options parsing, bazelrc label Jun 11, 2024
@Wyverald
Copy link
Member

Thank you for the crystal clear repro instructions! Looking now.

@Wyverald
Copy link
Member

The crux of the issue is that we have no way to figure out the repo mapping of @@foo, as its definition doesn't exist anywhere (other than the --override_repository flag, that is). With --enable_workspace, we happen to have a fallback in the WORKSPACE file (which gives an empty repo mapping for nonexistent repos); this fallback is not there with --noenable_workspace.

Now, it's not completely clear to me what the correct behavior should be. What should we use as the repo mapping of @@foo? If we give it an empty mapping, it essentially means that @@foo cannot use any labels with a repo part (e.g. load("@bazel_skylib//... would fail). As an alternative, it's arguable that we should just outright fail, since @@foo isn't defined anywhere. @lbcjbb, could you share a bit about your use case with the "external aspect"? Is it for IDE usage?

In any case, we definitely shouldn't crash. So this is still a legit bug.

@Wyverald Wyverald added P2 We'll consider working on this in future. (Assignee optional) and removed untriaged labels Jun 11, 2024
@ghost
Copy link
Author

ghost commented Jun 11, 2024

I have no direct use for it. However, the Bazel plugin for Intellij yes.
This plugin needs to execute a command similar to the one below:

bazel build \
  --tool_tag=ijwb:IDEA:ultimate \
  --keep_going \
  --build_event_binary_file=<tmpfile> \
  --nobuild_event_binary_file_path_conversion \
  --curses=no \
  --color=yes \
  --progress_in_terminal_title=no \
  --noexperimental_run_validations \
  --aspects=@@intellij_aspect//:intellij_info_bundled.bzl%intellij_info_aspect \
  --override_repository=intellij_aspect=$HOME/.local/share/JetBrains/IntelliJIdea2024.1/ijwb/aspect \
  --output_groups=intellij-resolve-go,intellij-resolve-java,intellij-resolve-py,intellij-info-generic,intellij-info-go,intellij-info-java,intellij-info-py \
  -- \
  //...

I mitigated the problem by configuring the option --enable_workspace option in the build_flags to override the --noenable_workspace option of my .bazelrc file.

@fmeum
Copy link
Collaborator

fmeum commented Jun 11, 2024

Looking at the aspect source, I think that IntelliJ should be using --override_module instead with Bzlmod. With Bazel 8, it would need to add a dependency on rules_java to access Java providers, even if the main module may not have such a dependency. Any synthetic repo mapping we could produce wouldn't work in this situation. But yes, it shouldn't crash.

@Wyverald
Copy link
Member

Problem is that --override_module doesn't add a dep. So we'd really need an --add_module or some such... which would then mess with version resolution.

We did discuss this with the JetBrains people before, especially as they might want to support e.g. providers in rules_kotlin without having to force the user into fetching rules_kotlin. I can't remember what conclusion we reached (if any).

cc @agluszak

@fmeum
Copy link
Collaborator

fmeum commented Sep 4, 2024

I think that the following could work:

  1. Allow --override_repository to add new repos and have them inherit their repo mapping from the main repo (and also add them to the main repo's mapping so that they can be referenced via flags).
  2. In a preparatory step, let the IDE parse the direct module deps and their apparent names out of the MODULE.bazel file (see https://github.com/bazelbuild/bazel-gazelle/blob/533d1ef739f05bd925ae0b01aa82eea663f572f0/internal/module/module.go#L33 for how Gazelle does this).
  3. Generate a repo with references to the providers in those direct deps only, using the apparent names.

@rogerhu
Copy link
Contributor

rogerhu commented Sep 24, 2024

So to be clear the current workaround is to do build --enable_workspace (or setting the bazel flags in the IntelliJ project) This incompatibility is the result of 5881c38 introduced last month.

@fmeum
Copy link
Collaborator

fmeum commented Sep 27, 2024

In this Slack conversation with @tpasternak we realized that bazel mod deps could be extended to provide the mapping from module names to apparent names in the root module.

@fmeum
Copy link
Collaborator

fmeum commented Sep 28, 2024

unused_deps is also affected by this problem.

copybara-service bot pushed a commit that referenced this issue Sep 30, 2024
Otherwise `"<root>"` ends up being escaped with Unicode escape sequences, which is unnecessarily complex.

Work towards #22691

Closes #23785.

PiperOrigin-RevId: 680633877
Change-Id: Ic3c90c33bbf1209efa90be78b432e2132f0a1f05
bazel-io pushed a commit to bazel-io/bazel that referenced this issue Sep 30, 2024
Otherwise `"<root>"` ends up being escaped with Unicode escape sequences, which is unnecessarily complex.

Work towards bazelbuild#22691

Closes bazelbuild#23785.

PiperOrigin-RevId: 680633877
Change-Id: Ic3c90c33bbf1209efa90be78b432e2132f0a1f05
github-merge-queue bot pushed a commit that referenced this issue Oct 1, 2024
Otherwise `"<root>"` ends up being escaped with Unicode escape
sequences, which is unnecessarily complex.

Work towards #22691

Closes #23785.

PiperOrigin-RevId: 680633877
Change-Id: Ic3c90c33bbf1209efa90be78b432e2132f0a1f05

Commit
e959d78

Co-authored-by: Fabian Meumertzheim <[email protected]>
tpasternak pushed a commit to tpasternak/bazel-intellij that referenced this issue Oct 7, 2024
tpasternak pushed a commit to bazelbuild/intellij that referenced this issue Oct 8, 2024
* Disable bazel deps call. It's unused yet, and could fail

bazelbuild/bazel#22691
copybara-service bot pushed a commit that referenced this issue Nov 5, 2024
This allows IDEs to query for the direct dependencies of the root module as well as how they can refer to them from the point of view of the root module.

Also always emit `name` and `version` so that consumers don't have to know how to parse module keys.

Work towards #22691

Closes #23787.

PiperOrigin-RevId: 693453084
Change-Id: Ie3fd5e89301d8e83d0eaa686188634923853f01a
bazel-io pushed a commit to bazel-io/bazel that referenced this issue Nov 6, 2024
This allows IDEs to query for the direct dependencies of the root module as well as how they can refer to them from the point of view of the root module.

Also always emit `name` and `version` so that consumers don't have to know how to parse module keys.

Work towards bazelbuild#22691

Closes bazelbuild#23787.

PiperOrigin-RevId: 693453084
Change-Id: Ie3fd5e89301d8e83d0eaa686188634923853f01a
github-merge-queue bot pushed a commit that referenced this issue Nov 7, 2024
…4234)

This allows IDEs to query for the direct dependencies of the root module
as well as how they can refer to them from the point of view of the root
module.

Also always emit `name` and `version` so that consumers don't have to
know how to parse module keys.

Work towards #22691

Closes #23787.

PiperOrigin-RevId: 693453084
Change-Id: Ie3fd5e89301d8e83d0eaa686188634923853f01a

Commit
1003d2c

Co-authored-by: Fabian Meumertzheim <[email protected]>
github-merge-queue bot pushed a commit that referenced this issue Nov 8, 2024
…4234)

This allows IDEs to query for the direct dependencies of the root module
as well as how they can refer to them from the point of view of the root
module.

Also always emit `name` and `version` so that consumers don't have to
know how to parse module keys.

Work towards #22691

Closes #23787.

PiperOrigin-RevId: 693453084
Change-Id: Ie3fd5e89301d8e83d0eaa686188634923853f01a

Commit
1003d2c

Co-authored-by: Fabian Meumertzheim <[email protected]>
@tpasternak
Copy link

Folks, please correct me if I'm wrong - we still need to wait for the --inject_repository flag PR to be merged?

@fmeum
Copy link
Collaborator

fmeum commented Nov 12, 2024

Folks, please correct me if I'm wrong - we still need to wait for the --inject_repository flag PR to be merged?

Yes. The other required PR, the one that adds apparent names to bazel mod deps JSON output, has already been merged into the Bazel 8 branch.

Wyverald pushed a commit that referenced this issue Nov 12, 2024
Suggest the new flag instead of crashing when `--override_repository` is applied to a non-existent repo with `--noenable_workspace`.

Fixes #22691

RELNOTES: The new `--inject_repository` flag can be used to add new repositories via the CLI with `--enable_bzlmod`. Such repositories behave as if they were declared by `local_repository` via `use_repo_rule` in the root module.

Closes #23791.

PiperOrigin-RevId: 695848897
Change-Id: I92ed25261c92d07f289815fcf6a65485ff43f373
github-merge-queue bot pushed a commit that referenced this issue Nov 12, 2024
…stent repo (#24301)

Suggest the new flag instead of crashing when `--override_repository` is
applied to a non-existent repo with `--noenable_workspace`.

Fixes #22691

RELNOTES: The new `--inject_repository` flag can be used to add new
repositories via the CLI with `--enable_bzlmod`. Such repositories
behave as if they were declared by `local_repository` via
`use_repo_rule` in the root module.

Closes #23791.

PiperOrigin-RevId: 695848897
Change-Id: I92ed25261c92d07f289815fcf6a65485ff43f373

Co-authored-by: Fabian Meumertzheim <[email protected]>
@tpasternak
Copy link

Thank you. It looks we are now unblocked and can move forward. However I noticed a few interesting things releated to inject repository:

  1. When running aspect from CLI, I have to use apparent names @. Canonical references don't work @@
  2. bazel mod graph and bazel mod deps don't show the injected repo (probably it's the same root cause as (1)

@fmeum
Copy link
Collaborator

fmeum commented Nov 13, 2024

  1. When running aspect from CLI, I have to use apparent names @. Canonical references don't work @@

This is expected: With Bzlmod, canonical names should always be managed by Bazel itself to ensure that they can't collide. That's why your injected repo @foo will have a canonical name such as @@+_repo_rules+foo that you shouldn't ever have to reference. Since WORKSPACE repos have canonical names equal to their apparent names, you should be able to exclusively use apparent names.

  1. bazel mod graph and bazel mod deps don't show the injected repo (probably it's the same root cause as (1)

I will look into this, but note that these commands generally show information about modules, not repos, by default.

@tpasternak
Copy link

Thank you!

@iancha1992
Copy link
Member

A fix for this issue has been included in Bazel 8.0.0 RC3. Please test out the release candidate and report any issues as soon as possible.
If you're using Bazelisk, you can point to the latest RC by setting USE_BAZEL_VERSION=8.0.0rc3. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

Successfully merging a pull request may close this issue.

7 participants