From 7d09b4a15985052670244c277e4357557b4d0039 Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 21 Dec 2021 12:16:45 -0800 Subject: [PATCH] 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: 417668153 --- .../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, 38 insertions(+), 123 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 a3aa4ccc8bda09..dd2b6cf0c10c07 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD @@ -2207,7 +2207,6 @@ 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 0c276e8291ad26..3537effcecc914 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,7 +29,6 @@ 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; @@ -814,7 +813,6 @@ 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 @@ -830,41 +828,30 @@ 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(repoName, ImmutableMap.of())); + workspaceFileValue + .getRepositoryMapping() + .getOrDefault(enclosingFileLabel.getRepository(), 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) { - 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(); - } + if (key instanceof BzlLoadValue.KeyForBzlmod + && enclosingFileLabel.getRepository().getName().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; } // 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(repoName)); + (RepositoryMappingValue) + env.getValue(RepositoryMappingValue.key(packageIdentifier.getRepository())); 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 57a19d58e1b254..81dc320965a461 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 = ((RepositoryMappingValue.Key) skyKey).repoName(); + RepositoryName repositoryName = (RepositoryName) skyKey.argument(); BazelModuleResolutionValue bazelModuleResolutionValue = null; if (Preconditions.checkNotNull(RepositoryDelegatorFunction.ENABLE_BZLMOD.get(env))) { @@ -56,10 +56,9 @@ public SkyValue compute(SkyKey skyKey, Environment env) return null; } - 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. + // 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()) { 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 6d452913e00ba9..6ab436eeea99aa 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,15 +14,14 @@ 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; @@ -49,8 +48,6 @@ * 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; @@ -66,8 +63,7 @@ public RepositoryMapping getRepositoryMapping() { /** Returns the {@link Key} for {@link RepositoryMappingValue}s. */ public static Key key(RepositoryName repositoryName) { - return RepositoryMappingValue.Key.create( - repositoryName, /*rootModuleShouldSeeWorkspaceRepos=*/ true); + return RepositoryMappingValue.Key.create(repositoryName); } /** Returns a {@link RepositoryMappingValue} for a workspace with the given name. */ @@ -94,25 +90,21 @@ public String toString() { return repositoryMapping.toString(); } - /** {@link SkyKey} for {@link RepositoryMappingValue}. */ - @AutoValue - abstract static class Key implements SkyKey { + /** {@link com.google.devtools.build.skyframe.SkyKey} for {@link RepositoryMappingValue}. */ + @AutoCodec.VisibleForSerialization + @AutoCodec + static class Key extends AbstractSkyKey { private static final Interner interner = BlazeInterners.newWeakInterner(); - /** 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(); + private Key(RepositoryName arg) { + super(arg); + } + @AutoCodec.VisibleForSerialization @AutoCodec.Instantiator - static Key create(RepositoryName repoName, boolean rootModuleShouldSeeWorkspaceRepos) { - return interner.intern( - new AutoValue_RepositoryMappingValue_Key(repoName, rootModuleShouldSeeWorkspaceRepos)); + static Key create(RepositoryName arg) { + return interner.intern(new Key(arg)); } @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 49a04f58995e71..5c426d23b7ca25 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,7 +103,8 @@ public RepositoryMappingValue withMappingForRootModule( @Test public void testSimpleMapping() throws Exception { - rewriteWorkspace( + scratch.overwriteFile( + "WORKSPACE", "workspace(name = 'good')", "local_repository(", " name = 'a_remote_repo',", @@ -315,7 +316,8 @@ public void testRepoNameMapping_multipleVersionOverride_lookup() throws Exceptio @Test public void testMultipleRepositoriesWithMapping() throws Exception { - rewriteWorkspace( + scratch.overwriteFile( + "WORKSPACE", "workspace(name = 'good')", "local_repository(", " name = 'a_remote_repo',", @@ -354,7 +356,8 @@ public void testMultipleRepositoriesWithMapping() throws Exception { @Test public void testRepositoryWithMultipleMappings() throws Exception { - rewriteWorkspace( + scratch.overwriteFile( + "WORKSPACE", "workspace(name = 'good')", "local_repository(", " name = 'a_remote_repo',", @@ -378,8 +381,9 @@ public void testRepositoryWithMultipleMappings() throws Exception { } @Test - public void testMixtureOfBothSystems_workspaceRepo() throws Exception { - rewriteWorkspace( + public void testMixtureOfBothSystems() throws Exception { + scratch.overwriteFile( + "WORKSPACE", "workspace(name = 'root')", "local_repository(", " name = 'ws_repo',", @@ -429,72 +433,6 @@ public void testMixtureOfBothSystems_workspaceRepo() 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);