Skip to content

Commit

Permalink
Bzlmod: When evaluating extensions in the main repo, do not load WORK…
Browse files Browse the repository at this point in the history 
…SPACE

(bazelbuild#13316)

See new comment in BzlLoadFunction.java for details. bazelbuild/bazel-central-registry#47 (comment) also has a bit more context.

PiperOrigin-RevId: 417633822
  • Loading branch information
@Wyverald @copybara-github
Wyverald authored and copybara-github committed Dec 21, 2021
1 parent da29398 commit 13112c0
Show file tree
Hide file tree
Showing 5 changed files with 123 additions and 38 deletions.
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/skyframe/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
],
)
Expand Down
39 changes: 26 additions & 13 deletions src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand All @@ -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().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;
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;
}
Expand Down
9 changes: 5 additions & 4 deletions src/main/java/com/google/devtools/build/lib/skyframe/RepositoryMappingFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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))) {
Expand All @@ -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()) {
Expand Down
32 changes: 20 additions & 12 deletions src/main/java/com/google/devtools/build/lib/skyframe/RepositoryMappingValue.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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;

Expand All @@ -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. */
Expand All @@ -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<RepositoryName> {
/** {@link SkyKey} for {@link RepositoryMappingValue}. */
@AutoValue
abstract static class Key implements SkyKey {

private static final Interner<Key> 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
Expand Down
80 changes: 71 additions & 9 deletions src/test/java/com/google/devtools/build/lib/skyframe/RepositoryMappingFunctionTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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',",
Expand Down Expand Up @@ -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',",
Expand Down Expand Up @@ -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',",
Expand All @@ -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',",
Expand Down Expand Up @@ -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);
Expand Down

0 comments on commit 13112c0

Please sign in to comment.