Skip to content

Commit 48a0fc5

Browse files
WyveraldWyverald
committed
Automated rollback of commit 7d09b4a.
*** Reason for rollback *** Now that unknown commit has been submitted, we can roll this forward with a small fix. *** Original change description *** Automated rollback of commit 13112c0. *** Reason for rollback *** Causes [failures](https://buildkite.com/bazel/google-bazel-presubmit/builds/52620) in Presubmit of unknown commit, itself a Rollback of 78d0131 *** Original change description *** Bzlmod: When evaluating extensions in the main repo, do not load WORKSPACE (#13316) See new comment in BzlLoadFunction.java for details. bazelbuild/bazel-central-registry#47 (comment) also has a bit more context. *** PiperOrigin-RevId: 417820112
1 parent 43bcf80 commit 48a0fc5

File tree

5 files changed

+123
-38
lines changed

5 files changed

+123
-38
lines changed

‎src/main/java/com/google/devtools/build/lib/skyframe/BUILD‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2211,6 +2211,7 @@ java_library(
22112211
"//src/main/java/com/google/devtools/build/lib/concurrent",
22122212
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec",
22132213
"//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
2214+
"//third_party:auto_value",
22142215
"//third_party:guava",
22152216
],
22162217
)

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

Lines changed: 26 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
3030
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
3131
import com.google.devtools.build.lib.cmdline.RepositoryMapping;
32+
import com.google.devtools.build.lib.cmdline.RepositoryName;
3233
import com.google.devtools.build.lib.concurrent.BlazeInterners;
3334
import com.google.devtools.build.lib.events.Event;
3435
import com.google.devtools.build.lib.events.EventHandler;
@@ -813,6 +814,7 @@ private static RepositoryMapping getRepositoryMapping(BzlLoadValue.Key key, Envi
813814
}
814815

815816
Label enclosingFileLabel = key.getLabel();
817+
RepositoryName repoName = enclosingFileLabel.getRepository();
816818

817819
if (key instanceof BzlLoadValue.KeyForWorkspace) {
818820
// Still during workspace file evaluation
@@ -828,30 +830,41 @@ private static RepositoryMapping getRepositoryMapping(BzlLoadValue.Key key, Envi
828830
// Note: we know for sure that the requested WorkspaceFileValue is fully computed so we do
829831
// not need to check if it is null
830832
return RepositoryMapping.createAllowingFallback(
831-
workspaceFileValue
832-
.getRepositoryMapping()
833-
.getOrDefault(enclosingFileLabel.getRepository(), ImmutableMap.of()));
833+
workspaceFileValue.getRepositoryMapping().getOrDefault(repoName, ImmutableMap.of()));
834834
// NOTE(wyv): this means that, in the WORKSPACE file, we can't load from a repo generated by
835835
// bzlmod. If that's a problem, we should "fall back" to the bzlmod case below.
836836
}
837837
}
838838

839-
if (key instanceof BzlLoadValue.KeyForBzlmod
840-
&& enclosingFileLabel.getRepository().strippedName().equals("bazel_tools")) {
841-
// Special case: we're only here to get the @bazel_tools repo (for example, for http_archive).
842-
// This repo shouldn't have visibility into anything else (during repo generation), so we just
843-
// return an empty repo mapping.
844-
// TODO(wyv): disallow fallback.
845-
return RepositoryMapping.ALWAYS_FALLBACK;
839+
if (key instanceof BzlLoadValue.KeyForBzlmod) {
840+
if (repoName.equals(RepositoryName.BAZEL_TOOLS)) {
841+
// Special case: we're only here to get the @bazel_tools repo (for example, for
842+
// http_archive). This repo shouldn't have visibility into anything else (during repo
843+
// generation), so we just return an empty repo mapping.
844+
// TODO(wyv): disallow fallback.
845+
return RepositoryMapping.ALWAYS_FALLBACK;
846+
}
847+
if (repoName.isMain()) {
848+
// Special case: when we try to run an extension in the main repo, we need to grab the repo
849+
// mapping for the main repo, which normally would include all WORKSPACE repos. This is
850+
// problematic if the reason we're running an extension at all is that we're trying to do a
851+
// `load` in WORKSPACE. So we specifically say that, to run an extension in the main repo,
852+
// we ask for a repo mapping *without* WORKSPACE repos.
853+
RepositoryMappingValue repositoryMappingValue =
854+
(RepositoryMappingValue)
855+
env.getValue(RepositoryMappingValue.KEY_FOR_ROOT_MODULE_WITHOUT_WORKSPACE_REPOS);
856+
if (repositoryMappingValue == null) {
857+
return null;
858+
}
859+
return repositoryMappingValue.getRepositoryMapping();
860+
}
846861
}
847862

848863
// This is either a .bzl loaded from BUILD files, or a .bzl loaded for bzlmod (in which case the
849864
// .bzl file *has* to be from a Bazel module anyway). So we can just use the full repo mapping
850865
// from RepositoryMappingFunction.
851-
PackageIdentifier packageIdentifier = enclosingFileLabel.getPackageIdentifier();
852866
RepositoryMappingValue repositoryMappingValue =
853-
(RepositoryMappingValue)
854-
env.getValue(RepositoryMappingValue.key(packageIdentifier.getRepository()));
867+
(RepositoryMappingValue) env.getValue(RepositoryMappingValue.key(repoName));
855868
if (repositoryMappingValue == null) {
856869
return null;
857870
}

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

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ public class RepositoryMappingFunction implements SkyFunction {
4646
@Override
4747
public SkyValue compute(SkyKey skyKey, Environment env)
4848
throws SkyFunctionException, InterruptedException {
49-
RepositoryName repositoryName = (RepositoryName) skyKey.argument();
49+
RepositoryName repositoryName = ((RepositoryMappingValue.Key) skyKey).repoName();
5050

5151
BazelModuleResolutionValue bazelModuleResolutionValue = null;
5252
if (Preconditions.checkNotNull(RepositoryDelegatorFunction.ENABLE_BZLMOD.get(env))) {
@@ -56,9 +56,10 @@ public SkyValue compute(SkyKey skyKey, Environment env)
5656
return null;
5757
}
5858

59-
// The root module should be able to see repos defined in WORKSPACE. Therefore, we find all
60-
// workspace repos and add them as extra visible repos in root module's repo mappings.
61-
if (repositoryName.isMain()) {
59+
if (repositoryName.isMain()
60+
&& ((RepositoryMappingValue.Key) skyKey).rootModuleShouldSeeWorkspaceRepos()) {
61+
// The root module should be able to see repos defined in WORKSPACE. Therefore, we find all
62+
// workspace repos and add them as extra visible repos in root module's repo mappings.
6263
SkyKey externalPackageKey = PackageValue.key(LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER);
6364
PackageValue externalPackageValue = (PackageValue) env.getValue(externalPackageKey);
6465
if (env.valuesMissing()) {

‎src/main/java/com/google/devtools/build/lib/skyframe/RepositoryMappingValue.java‎

Lines changed: 20 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -14,14 +14,15 @@
1414

1515
package com.google.devtools.build.lib.skyframe;
1616

17+
import com.google.auto.value.AutoValue;
1718
import com.google.common.base.Preconditions;
1819
import com.google.common.collect.Interner;
1920
import com.google.devtools.build.lib.cmdline.RepositoryMapping;
2021
import com.google.devtools.build.lib.cmdline.RepositoryName;
2122
import com.google.devtools.build.lib.concurrent.BlazeInterners;
2223
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
23-
import com.google.devtools.build.skyframe.AbstractSkyKey;
2424
import com.google.devtools.build.skyframe.SkyFunctionName;
25+
import com.google.devtools.build.skyframe.SkyKey;
2526
import com.google.devtools.build.skyframe.SkyValue;
2627
import java.util.Objects;
2728

@@ -48,6 +49,8 @@
4849
* packages. If the mappings are changed then the external packages need to be reloaded.
4950
*/
5051
public class RepositoryMappingValue implements SkyValue {
52+
public static final Key KEY_FOR_ROOT_MODULE_WITHOUT_WORKSPACE_REPOS =
53+
Key.create(RepositoryName.MAIN, /*rootModuleShouldSeeWorkspaceRepos=*/ false);
5154

5255
private final RepositoryMapping repositoryMapping;
5356

@@ -63,7 +66,8 @@ public RepositoryMapping getRepositoryMapping() {
6366

6467
/** Returns the {@link Key} for {@link RepositoryMappingValue}s. */
6568
public static Key key(RepositoryName repositoryName) {
66-
return RepositoryMappingValue.Key.create(repositoryName);
69+
return RepositoryMappingValue.Key.create(
70+
repositoryName, /*rootModuleShouldSeeWorkspaceRepos=*/ true);
6771
}
6872

6973
/** Returns a {@link RepositoryMappingValue} for a workspace with the given name. */
@@ -90,21 +94,25 @@ public String toString() {
9094
return repositoryMapping.toString();
9195
}
9296

93-
/** {@link com.google.devtools.build.skyframe.SkyKey} for {@link RepositoryMappingValue}. */
94-
@AutoCodec.VisibleForSerialization
95-
@AutoCodec
96-
static class Key extends AbstractSkyKey<RepositoryName> {
97+
/** {@link SkyKey} for {@link RepositoryMappingValue}. */
98+
@AutoValue
99+
abstract static class Key implements SkyKey {
97100

98101
private static final Interner<Key> interner = BlazeInterners.newWeakInterner();
99102

100-
private Key(RepositoryName arg) {
101-
super(arg);
102-
}
103+
/** The name of the repo to grab mappings for. */
104+
abstract RepositoryName repoName();
105+
106+
/**
107+
* Whether the root module should see repos defined in WORKSPACE. This only takes effect when
108+
* {@link #repoName} is the main repo.
109+
*/
110+
abstract boolean rootModuleShouldSeeWorkspaceRepos();
103111

104-
@AutoCodec.VisibleForSerialization
105112
@AutoCodec.Instantiator
106-
static Key create(RepositoryName arg) {
107-
return interner.intern(new Key(arg));
113+
static Key create(RepositoryName repoName, boolean rootModuleShouldSeeWorkspaceRepos) {
114+
return interner.intern(
115+
new AutoValue_RepositoryMappingValue_Key(repoName, rootModuleShouldSeeWorkspaceRepos));
108116
}
109117

110118
@Override

‎src/test/java/com/google/devtools/build/lib/skyframe/RepositoryMappingFunctionTest.java‎

Lines changed: 71 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -103,8 +103,7 @@ public RepositoryMappingValue withMappingForRootModule(
103103

104104
@Test
105105
public void testSimpleMapping() throws Exception {
106-
scratch.overwriteFile(
107-
"WORKSPACE",
106+
rewriteWorkspace(
108107
"workspace(name = 'good')",
109108
"local_repository(",
110109
" name = 'a_remote_repo',",
@@ -316,8 +315,7 @@ public void testRepoNameMapping_multipleVersionOverride_lookup() throws Exceptio
316315

317316
@Test
318317
public void testMultipleRepositoriesWithMapping() throws Exception {
319-
scratch.overwriteFile(
320-
"WORKSPACE",
318+
rewriteWorkspace(
321319
"workspace(name = 'good')",
322320
"local_repository(",
323321
" name = 'a_remote_repo',",
@@ -356,8 +354,7 @@ public void testMultipleRepositoriesWithMapping() throws Exception {
356354

357355
@Test
358356
public void testRepositoryWithMultipleMappings() throws Exception {
359-
scratch.overwriteFile(
360-
"WORKSPACE",
357+
rewriteWorkspace(
361358
"workspace(name = 'good')",
362359
"local_repository(",
363360
" name = 'a_remote_repo',",
@@ -381,9 +378,8 @@ public void testRepositoryWithMultipleMappings() throws Exception {
381378
}
382379

383380
@Test
384-
public void testMixtureOfBothSystems() throws Exception {
385-
scratch.overwriteFile(
386-
"WORKSPACE",
381+
public void testMixtureOfBothSystems_workspaceRepo() throws Exception {
382+
rewriteWorkspace(
387383
"workspace(name = 'root')",
388384
"local_repository(",
389385
" name = 'ws_repo',",
@@ -433,6 +429,72 @@ public void testMixtureOfBothSystems() throws Exception {
433429
.build()));
434430
}
435431

432+
@Test
433+
public void testMixtureOfBothSystems_mainRepo() throws Exception {
434+
rewriteWorkspace(
435+
"workspace(name = 'root')",
436+
"local_repository(",
437+
" name = 'ws_repo',",
438+
" path = '/ws_repo',",
439+
")");
440+
scratch.overwriteFile(
441+
"MODULE.bazel", "module(name='A',version='0.1')", "bazel_dep(name='B',version='1.0')");
442+
registry
443+
.addModule(
444+
createModuleKey("B", "1.0"),
445+
"module(name='B', version='1.0');bazel_dep(name='C', version='2.0')")
446+
.addModule(createModuleKey("C", "2.0"), "module(name='C', version='2.0')");
447+
448+
SkyKey skyKey = RepositoryMappingValue.key(RepositoryName.MAIN);
449+
assertThatEvaluationResult(eval(skyKey))
450+
.hasEntryThat(skyKey)
451+
.isEqualTo(
452+
withMappingForRootModule(
453+
ImmutableMap.of(
454+
RepositoryName.MAIN,
455+
RepositoryName.MAIN,
456+
RepositoryName.create("@A"),
457+
RepositoryName.MAIN,
458+
RepositoryName.create("@B"),
459+
RepositoryName.create("@B.1.0"),
460+
RepositoryName.create("@root"),
461+
RepositoryName.create("@root"),
462+
RepositoryName.create("@ws_repo"),
463+
RepositoryName.create("@ws_repo")),
464+
RepositoryName.MAIN));
465+
}
466+
467+
@Test
468+
public void testMixtureOfBothSystems_mainRepo_shouldNotSeeWorkspaceRepos() throws Exception {
469+
rewriteWorkspace(
470+
"workspace(name = 'root')",
471+
"local_repository(",
472+
" name = 'ws_repo',",
473+
" path = '/ws_repo',",
474+
")");
475+
scratch.overwriteFile(
476+
"MODULE.bazel", "module(name='A',version='0.1')", "bazel_dep(name='B',version='1.0')");
477+
registry
478+
.addModule(
479+
createModuleKey("B", "1.0"),
480+
"module(name='B', version='1.0');bazel_dep(name='C', version='2.0')")
481+
.addModule(createModuleKey("C", "2.0"), "module(name='C', version='2.0')");
482+
483+
SkyKey skyKey = RepositoryMappingValue.KEY_FOR_ROOT_MODULE_WITHOUT_WORKSPACE_REPOS;
484+
assertThatEvaluationResult(eval(skyKey))
485+
.hasEntryThat(skyKey)
486+
.isEqualTo(
487+
withMapping(
488+
ImmutableMap.of(
489+
RepositoryName.MAIN,
490+
RepositoryName.MAIN,
491+
RepositoryName.create("@A"),
492+
RepositoryName.MAIN,
493+
RepositoryName.create("@B"),
494+
RepositoryName.create("@B.1.0")),
495+
RepositoryName.MAIN));
496+
}
497+
436498
@Test
437499
public void testErrorWithMapping() throws Exception {
438500
reporter.removeHandler(failFastHandler);

0 commit comments

Comments
 (0)