From 302971e1b3d803069ac949c0085c0d2a3916c8ab Mon Sep 17 00:00:00 2001 From: wyv Date: Wed, 22 Dec 2021 07:51:58 -0800 Subject: [PATCH] Automated rollback of commit 7d09b4a15985052670244c277e4357557b4d0039. *** 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 13112c027c0064cf0a29256e80560cb6630af94d. *** Reason for rollback *** Causes [failures](https://buildkite.com/bazel/google-bazel-presubmit/builds/52620) in Presubmit of unknown commit, itself a Rollback of https://github.com/bazelbuild/bazel/commit/78d01316b22667e9d1758472c91dfee35cc189bd *** Original change description *** Bzlmod: When evaluating extensions in the main repo, do not load WORKSPACE (https://github.com/bazelbuild/bazel/issues/13316) See new comment in BzlLoadFunction.java for details. https://github.com/bazelbuild/bazel-central-registry/pull/47#issuecomment-998883652 also has a bit more context. *** PiperOrigin-RevId: 417820112 --- .../google/devtools/build/lib/skyframe/BUILD | 1 + .../build/lib/skyframe/BzlLoadFunction.java | 39 ++++++--- .../skyframe/RepositoryMappingFunction.java | 9 ++- .../lib/skyframe/RepositoryMappingValue.java | 32 +++++--- .../RepositoryMappingFunctionTest.java | 80 ++++++++++++++++--- 5 files changed, 123 insertions(+), 38 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD index dd2b6cf0c10c07..a3aa4ccc8bda09 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD @@ -2207,6 +2207,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/concurrent", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec", "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", + "//third_party:auto_value", "//third_party:guava", ], ) diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java index 3de8e895e7b4cc..0c276e8291ad26 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java @@ -29,6 +29,7 @@ import com.google.devtools.build.lib.cmdline.LabelSyntaxException; import com.google.devtools.build.lib.cmdline.PackageIdentifier; import com.google.devtools.build.lib.cmdline.RepositoryMapping; +import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.concurrent.BlazeInterners; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.EventHandler; @@ -813,6 +814,7 @@ private static RepositoryMapping getRepositoryMapping(BzlLoadValue.Key key, Envi } Label enclosingFileLabel = key.getLabel(); + RepositoryName repoName = enclosingFileLabel.getRepository(); if (key instanceof BzlLoadValue.KeyForWorkspace) { // Still during workspace file evaluation @@ -828,30 +830,41 @@ private static RepositoryMapping getRepositoryMapping(BzlLoadValue.Key key, Envi // Note: we know for sure that the requested WorkspaceFileValue is fully computed so we do // not need to check if it is null return RepositoryMapping.createAllowingFallback( - workspaceFileValue - .getRepositoryMapping() - .getOrDefault(enclosingFileLabel.getRepository(), ImmutableMap.of())); + workspaceFileValue.getRepositoryMapping().getOrDefault(repoName, ImmutableMap.of())); // NOTE(wyv): this means that, in the WORKSPACE file, we can't load from a repo generated by // bzlmod. If that's a problem, we should "fall back" to the bzlmod case below. } } - if (key instanceof BzlLoadValue.KeyForBzlmod - && enclosingFileLabel.getRepository().strippedName().equals("bazel_tools")) { - // Special case: we're only here to get the @bazel_tools repo (for example, for http_archive). - // This repo shouldn't have visibility into anything else (during repo generation), so we just - // return an empty repo mapping. - // TODO(wyv): disallow fallback. - return RepositoryMapping.ALWAYS_FALLBACK; + if (key instanceof BzlLoadValue.KeyForBzlmod) { + if (repoName.equals(RepositoryName.BAZEL_TOOLS)) { + // Special case: we're only here to get the @bazel_tools repo (for example, for + // http_archive). This repo shouldn't have visibility into anything else (during repo + // generation), so we just return an empty repo mapping. + // TODO(wyv): disallow fallback. + return RepositoryMapping.ALWAYS_FALLBACK; + } + if (repoName.isMain()) { + // Special case: when we try to run an extension in the main repo, we need to grab the repo + // mapping for the main repo, which normally would include all WORKSPACE repos. This is + // problematic if the reason we're running an extension at all is that we're trying to do a + // `load` in WORKSPACE. So we specifically say that, to run an extension in the main repo, + // we ask for a repo mapping *without* WORKSPACE repos. + RepositoryMappingValue repositoryMappingValue = + (RepositoryMappingValue) + env.getValue(RepositoryMappingValue.KEY_FOR_ROOT_MODULE_WITHOUT_WORKSPACE_REPOS); + if (repositoryMappingValue == null) { + return null; + } + return repositoryMappingValue.getRepositoryMapping(); + } } // This is either a .bzl loaded from BUILD files, or a .bzl loaded for bzlmod (in which case the // .bzl file *has* to be from a Bazel module anyway). So we can just use the full repo mapping // from RepositoryMappingFunction. - PackageIdentifier packageIdentifier = enclosingFileLabel.getPackageIdentifier(); RepositoryMappingValue repositoryMappingValue = - (RepositoryMappingValue) - env.getValue(RepositoryMappingValue.key(packageIdentifier.getRepository())); + (RepositoryMappingValue) env.getValue(RepositoryMappingValue.key(repoName)); if (repositoryMappingValue == null) { return null; } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RepositoryMappingFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/RepositoryMappingFunction.java index 594a7a26be6d5f..e2da4dd3c2d9f1 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/RepositoryMappingFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/RepositoryMappingFunction.java @@ -46,7 +46,7 @@ public class RepositoryMappingFunction implements SkyFunction { @Override public SkyValue compute(SkyKey skyKey, Environment env) throws SkyFunctionException, InterruptedException { - RepositoryName repositoryName = (RepositoryName) skyKey.argument(); + RepositoryName repositoryName = ((RepositoryMappingValue.Key) skyKey).repoName(); BazelModuleResolutionValue bazelModuleResolutionValue = null; if (Preconditions.checkNotNull(RepositoryDelegatorFunction.ENABLE_BZLMOD.get(env))) { @@ -56,9 +56,10 @@ public SkyValue compute(SkyKey skyKey, Environment env) return null; } - // The root module should be able to see repos defined in WORKSPACE. Therefore, we find all - // workspace repos and add them as extra visible repos in root module's repo mappings. - if (repositoryName.isMain()) { + if (repositoryName.isMain() + && ((RepositoryMappingValue.Key) skyKey).rootModuleShouldSeeWorkspaceRepos()) { + // The root module should be able to see repos defined in WORKSPACE. Therefore, we find all + // workspace repos and add them as extra visible repos in root module's repo mappings. SkyKey externalPackageKey = PackageValue.key(LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER); PackageValue externalPackageValue = (PackageValue) env.getValue(externalPackageKey); if (env.valuesMissing()) { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RepositoryMappingValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/RepositoryMappingValue.java index 6ab436eeea99aa..6d452913e00ba9 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/RepositoryMappingValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/RepositoryMappingValue.java @@ -14,14 +14,15 @@ package com.google.devtools.build.lib.skyframe; +import com.google.auto.value.AutoValue; import com.google.common.base.Preconditions; import com.google.common.collect.Interner; import com.google.devtools.build.lib.cmdline.RepositoryMapping; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.concurrent.BlazeInterners; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; -import com.google.devtools.build.skyframe.AbstractSkyKey; import com.google.devtools.build.skyframe.SkyFunctionName; +import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; import java.util.Objects; @@ -48,6 +49,8 @@ * packages. If the mappings are changed then the external packages need to be reloaded. */ public class RepositoryMappingValue implements SkyValue { + public static final Key KEY_FOR_ROOT_MODULE_WITHOUT_WORKSPACE_REPOS = + Key.create(RepositoryName.MAIN, /*rootModuleShouldSeeWorkspaceRepos=*/ false); private final RepositoryMapping repositoryMapping; @@ -63,7 +66,8 @@ public RepositoryMapping getRepositoryMapping() { /** Returns the {@link Key} for {@link RepositoryMappingValue}s. */ public static Key key(RepositoryName repositoryName) { - return RepositoryMappingValue.Key.create(repositoryName); + return RepositoryMappingValue.Key.create( + repositoryName, /*rootModuleShouldSeeWorkspaceRepos=*/ true); } /** Returns a {@link RepositoryMappingValue} for a workspace with the given name. */ @@ -90,21 +94,25 @@ public String toString() { return repositoryMapping.toString(); } - /** {@link com.google.devtools.build.skyframe.SkyKey} for {@link RepositoryMappingValue}. */ - @AutoCodec.VisibleForSerialization - @AutoCodec - static class Key extends AbstractSkyKey { + /** {@link SkyKey} for {@link RepositoryMappingValue}. */ + @AutoValue + abstract static class Key implements SkyKey { private static final Interner interner = BlazeInterners.newWeakInterner(); - private Key(RepositoryName arg) { - super(arg); - } + /** The name of the repo to grab mappings for. */ + abstract RepositoryName repoName(); + + /** + * Whether the root module should see repos defined in WORKSPACE. This only takes effect when + * {@link #repoName} is the main repo. + */ + abstract boolean rootModuleShouldSeeWorkspaceRepos(); - @AutoCodec.VisibleForSerialization @AutoCodec.Instantiator - static Key create(RepositoryName arg) { - return interner.intern(new Key(arg)); + static Key create(RepositoryName repoName, boolean rootModuleShouldSeeWorkspaceRepos) { + return interner.intern( + new AutoValue_RepositoryMappingValue_Key(repoName, rootModuleShouldSeeWorkspaceRepos)); } @Override diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/RepositoryMappingFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/RepositoryMappingFunctionTest.java index b6030c72b25de2..0ec34a18f0be9f 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/RepositoryMappingFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/RepositoryMappingFunctionTest.java @@ -103,8 +103,7 @@ public RepositoryMappingValue withMappingForRootModule( @Test public void testSimpleMapping() throws Exception { - scratch.overwriteFile( - "WORKSPACE", + rewriteWorkspace( "workspace(name = 'good')", "local_repository(", " name = 'a_remote_repo',", @@ -316,8 +315,7 @@ public void testRepoNameMapping_multipleVersionOverride_lookup() throws Exceptio @Test public void testMultipleRepositoriesWithMapping() throws Exception { - scratch.overwriteFile( - "WORKSPACE", + rewriteWorkspace( "workspace(name = 'good')", "local_repository(", " name = 'a_remote_repo',", @@ -356,8 +354,7 @@ public void testMultipleRepositoriesWithMapping() throws Exception { @Test public void testRepositoryWithMultipleMappings() throws Exception { - scratch.overwriteFile( - "WORKSPACE", + rewriteWorkspace( "workspace(name = 'good')", "local_repository(", " name = 'a_remote_repo',", @@ -381,9 +378,8 @@ public void testRepositoryWithMultipleMappings() throws Exception { } @Test - public void testMixtureOfBothSystems() throws Exception { - scratch.overwriteFile( - "WORKSPACE", + public void testMixtureOfBothSystems_workspaceRepo() throws Exception { + rewriteWorkspace( "workspace(name = 'root')", "local_repository(", " name = 'ws_repo',", @@ -433,6 +429,72 @@ public void testMixtureOfBothSystems() throws Exception { .build())); } + @Test + public void testMixtureOfBothSystems_mainRepo() throws Exception { + rewriteWorkspace( + "workspace(name = 'root')", + "local_repository(", + " name = 'ws_repo',", + " path = '/ws_repo',", + ")"); + scratch.overwriteFile( + "MODULE.bazel", "module(name='A',version='0.1')", "bazel_dep(name='B',version='1.0')"); + registry + .addModule( + createModuleKey("B", "1.0"), + "module(name='B', version='1.0');bazel_dep(name='C', version='2.0')") + .addModule(createModuleKey("C", "2.0"), "module(name='C', version='2.0')"); + + SkyKey skyKey = RepositoryMappingValue.key(RepositoryName.MAIN); + assertThatEvaluationResult(eval(skyKey)) + .hasEntryThat(skyKey) + .isEqualTo( + withMappingForRootModule( + ImmutableMap.of( + RepositoryName.MAIN, + RepositoryName.MAIN, + RepositoryName.create("@A"), + RepositoryName.MAIN, + RepositoryName.create("@B"), + RepositoryName.create("@B.1.0"), + RepositoryName.create("@root"), + RepositoryName.create("@root"), + RepositoryName.create("@ws_repo"), + RepositoryName.create("@ws_repo")), + RepositoryName.MAIN)); + } + + @Test + public void testMixtureOfBothSystems_mainRepo_shouldNotSeeWorkspaceRepos() throws Exception { + rewriteWorkspace( + "workspace(name = 'root')", + "local_repository(", + " name = 'ws_repo',", + " path = '/ws_repo',", + ")"); + scratch.overwriteFile( + "MODULE.bazel", "module(name='A',version='0.1')", "bazel_dep(name='B',version='1.0')"); + registry + .addModule( + createModuleKey("B", "1.0"), + "module(name='B', version='1.0');bazel_dep(name='C', version='2.0')") + .addModule(createModuleKey("C", "2.0"), "module(name='C', version='2.0')"); + + SkyKey skyKey = RepositoryMappingValue.KEY_FOR_ROOT_MODULE_WITHOUT_WORKSPACE_REPOS; + assertThatEvaluationResult(eval(skyKey)) + .hasEntryThat(skyKey) + .isEqualTo( + withMapping( + ImmutableMap.of( + RepositoryName.MAIN, + RepositoryName.MAIN, + RepositoryName.create("@A"), + RepositoryName.MAIN, + RepositoryName.create("@B"), + RepositoryName.create("@B.1.0")), + RepositoryName.MAIN)); + } + @Test public void testErrorWithMapping() throws Exception { reporter.removeHandler(failFastHandler);