Perform builtins injection for WORKSPACE-loaded bzl files. #18022

benjaminp · 2023-04-10T23:09:06Z

#17713

comius · 2023-04-11T10:06:52Z

cc @Wyverald @meteorcloudy @brandjon

src/main/java/com/google/devtools/build/lib/packages/BazelStarlarkEnvironment.java

src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java

comius · 2023-04-11T11:39:04Z

src/main/java/com/google/devtools/build/lib/skyframe/RepositoryMappingFunction.java

@@ -135,6 +135,10 @@ public SkyValue compute(SkyKey skyKey, Environment env)
        return computeForModuleExtensionRepo(
            repositoryName, moduleExtensionId.get(), extensionEvalValue, bazelDepGraphValue);
      }
+
+      if (RepositoryName.BAZEL_TOOLS.equals(repositoryName)) {


What is the justification for this check here?

The complexity of the code here is going slightly past my review capabilities and my first thought is this is just making the tests to pass and the code even more complex.

Similar complexity to this one is happening in BzlloadFunction.getRepositoryMapping, which I imagine is calling the location here.

Is there a way to simplify it?

I also don't understand this check. @bazel_tools should have its repo mapping calculated just like any other Bazel module, not the VALUE_FOR_ROOT_MODULE_WITHOUT_REPOS sentinel value.

Similar complexity to this one is happening in BzlloadFunction.getRepositoryMapping, which I imagine is calling the location here.

Indeed, BzlLoadFunction#getRepositoryMapping calls to RepositoryMappingFunction, but the former is more complex because repo mapping computation for .bzl files has extra considerations due to some "partial" loading. RepositoryMappingFunction deals with the general case of "all WORKSPACE/Bzlmod/injection has finished and we're just loading a normal BUILD file (and similar environments) now".

This check avoids a fundamental cycle when bzlmod is enabled: workspace loading -> @_builtins loading -> @_builtins repository mapping -> @bazel_tools repository mapping -> //external package -> workspace loading.

The effect of this check, I believe, is you can't declare a repo_mapping for @bazel_tools in WORKSPACE. That's fine, I think? I'm certainly open to any other ideas on how to resolve this cycle.

The conditions that lead to this line are (I took the liberty to enumerate them using and IDE, because I find it hard to follow {,} on github):

enableBzlMod

not @_builtins

not (main repo && rootModuleShouldSeeWorkspaceRepos)

bazelDepGraphValue.getCanonicalRepoNameLookup().get(repositoryName) == null

maybeGetModuleExtensionForRepo(repositoryName) == null

The conditions seem mostly fine, providing we can't use bazel_tools for either a module name or module extension within bzlmod. Is this true?
In case it can be used, you're back forming a cycle as soon as the new bazel_tools uses builtins. Or even worse, you don't allow this new repo to be used from anywhere (even when it should be locally available).

What I'm also concerned is, this doesn't seem to handle the same cycle when not enableBzlMod.

Also, because the cycle is started from @_builtins repository mapping request. I think this would be better put into BzlloadFunction.getRepositoryMapping, so that it covers only @_builtins -> @bazel_tools edge.

In that function it might even be clearer which cases need cycle-breaking.

(I'm not an expert in SkyFrame, I've written what I think and not everything might be completely accurate)

I'd also suggest adding a test for the minimal reproduction in the bug, having bzlmod enabled and disabled.

Hang on, I still don't understand. the branch on line 119 should include @bazel_tools, as bazel_tools is a builtin module ("builtin" as in "shipped with Bazel", not as in @_builtins). So this check on line 139 should never occur.

(In other words, from what Ivo wrote, the fourth bullet point should not hold.)

Alternatively, the question is -- how come this cycle wasn't a problem before? How does injection builtins for WORKSPACE-loaded .bzl files affect the repo mapping calculation of @bazel_tools when Bzlmod is enabled?

Sorry, there's more context about the extremely specific situations this cycle occurs in.

Hang on, I still don't understand. the branch on line 119 should include @bazel_tools, as bazel_tools is a builtin module ("builtin" as in "shipped with Bazel", not as in @_builtins). So this check on line 139 should never occur.

That is mostly true. In practice, bazel cmd --enable_bzlmod avoids the cycle I describe by loading the repository mapping from src/MODULE.tools. The cycle only happens in some Java tests that don't set up a MODULE.bazel for @bazel_tools.

Come to think of it, maybe I should simply improve the tests to be more realistic.

This looks good to me now. @Wyverald any concerns?

src/main/java/com/google/devtools/build/lib/packages/BazelStarlarkEnvironment.java

src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java

comius · 2023-04-12T09:18:46Z

src/main/java/com/google/devtools/build/lib/skyframe/RepositoryMappingFunction.java

@@ -135,6 +135,10 @@ public SkyValue compute(SkyKey skyKey, Environment env)
        return computeForModuleExtensionRepo(
            repositoryName, moduleExtensionId.get(), extensionEvalValue, bazelDepGraphValue);
      }
+
+      if (RepositoryName.BAZEL_TOOLS.equals(repositoryName)) {


The conditions that lead to this line are (I took the liberty to enumerate them using and IDE, because I find it hard to follow {,} on github):

enableBzlMod

not @_builtins

not (main repo && rootModuleShouldSeeWorkspaceRepos)

bazelDepGraphValue.getCanonicalRepoNameLookup().get(repositoryName) == null

maybeGetModuleExtensionForRepo(repositoryName) == null

The conditions seem mostly fine, providing we can't use bazel_tools for either a module name or module extension within bzlmod. Is this true?
In case it can be used, you're back forming a cycle as soon as the new bazel_tools uses builtins. Or even worse, you don't allow this new repo to be used from anywhere (even when it should be locally available).

What I'm also concerned is, this doesn't seem to handle the same cycle when not enableBzlMod.

Also, because the cycle is started from @_builtins repository mapping request. I think this would be better put into BzlloadFunction.getRepositoryMapping, so that it covers only @_builtins -> @bazel_tools edge.

In that function it might even be clearer which cases need cycle-breaking.

(I'm not an expert in SkyFrame, I've written what I think and not everything might be completely accurate)

bazelbuild#17713

sgowroji · 2023-04-17T02:54:41Z

Hi @comius, Since I can see that this PR has been approved, please let me know whether I should proceed with importing it. Thanks!

comius · 2023-04-17T11:55:01Z

Hi @comius, Since I can see that this PR has been approved, please let me know whether I should proceed with importing it. Thanks!

Please wait for @Wyverald's approval as well. Thanks.

bazelbuild#17713 Closes bazelbuild#18022. PiperOrigin-RevId: 525350732 Change-Id: I869653fcd6d4a82ccca49f14e66245c182062731

#17713 Closes #18022. PiperOrigin-RevId: 525350732 Change-Id: I869653fcd6d4a82ccca49f14e66245c182062731 Co-authored-by: Benjamin Peterson <[email protected]>

github-actions bot added the awaiting-review PR is awaiting review from an assigned reviewer label Apr 10, 2023

benjaminp force-pushed the workspace-bzl-injection branch 7 times, most recently from c95daf9 to 14212da Compare April 11, 2023 03:12

sgowroji added the team-Loading-API BUILD file and macro processing: labels, package(), visibility, glob label Apr 11, 2023

comius self-requested a review April 11, 2023 10:06

comius self-assigned this Apr 11, 2023

comius reviewed Apr 11, 2023

View reviewed changes

benjaminp force-pushed the workspace-bzl-injection branch from 14212da to c66e7ac Compare April 11, 2023 13:50

comius requested changes Apr 12, 2023

View reviewed changes

Perform builtins injection for WORKSPACE-loaded bzl files.

8ad943f

bazelbuild#17713

benjaminp force-pushed the workspace-bzl-injection branch from c66e7ac to 8ad943f Compare April 12, 2023 13:35

comius approved these changes Apr 14, 2023

View reviewed changes

Wyverald approved these changes Apr 17, 2023

View reviewed changes

Wyverald added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Apr 17, 2023

fmeum mentioned this pull request Apr 18, 2023

Python providers loaded during workspace fail due to stub provider #18131

Closed

copybara-service bot closed this in b1341be Apr 19, 2023

Pavank1992 removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Apr 19, 2023

benjaminp deleted the workspace-bzl-injection branch April 19, 2023 13:19

fweikert pushed a commit to fweikert/bazel that referenced this pull request May 25, 2023

Perform builtins injection for WORKSPACE-loaded bzl files.

f566f87

bazelbuild#17713 Closes bazelbuild#18022. PiperOrigin-RevId: 525350732 Change-Id: I869653fcd6d4a82ccca49f14e66245c182062731

benjaminp mentioned this pull request Jun 7, 2023

Builtins exports.bzl file is called after the WORKSPACE #17713

Closed

iancha1992 pushed a commit to iancha1992/bazel that referenced this pull request Jun 29, 2023

Perform builtins injection for WORKSPACE-loaded bzl files.

b10dee3

bazelbuild#17713 Closes bazelbuild#18022. PiperOrigin-RevId: 525350732 Change-Id: I869653fcd6d4a82ccca49f14e66245c182062731

iancha1992 mentioned this pull request Jun 29, 2023

[6.3.0] Perform builtins injection for WORKSPACE-loaded bzl files. #18819

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Perform builtins injection for WORKSPACE-loaded bzl files. #18022

Perform builtins injection for WORKSPACE-loaded bzl files. #18022

benjaminp commented Apr 10, 2023

comius commented Apr 11, 2023

comius Apr 11, 2023

Wyverald Apr 11, 2023

benjaminp Apr 11, 2023

comius Apr 12, 2023

comius Apr 12, 2023

Wyverald Apr 12, 2023

benjaminp Apr 12, 2023

comius Apr 14, 2023

comius Apr 12, 2023

sgowroji commented Apr 17, 2023

comius commented Apr 17, 2023

Perform builtins injection for WORKSPACE-loaded bzl files. #18022

Perform builtins injection for WORKSPACE-loaded bzl files. #18022

Conversation

benjaminp commented Apr 10, 2023

comius commented Apr 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgowroji commented Apr 17, 2023

comius commented Apr 17, 2023