Skip to content

Commit

Permalink
Fix edge cases in lockfile handling
Browse files Browse the repository at this point in the history
* Don't run the core logic when `--lockfile_mode` is `off` or `error` but the command doesn't forward options to Skyframe.
* Honor `reproducible` per extension eval factor, not per extension.
* Fix encoding conflict between isolation key and `use_repo_rule`'s fake extension names

This doesn't require a lockfile version bump as `use_repo_rule`'s fake extension (so far) isn't included in the lockfile.

Work towards bazelbuild#24723

Closes bazelbuild#24754.

PiperOrigin-RevId: 712623562
Change-Id: I61fd439539031a01ddec4488276ff2d0484849f2
  • Loading branch information
fmeum authored and copybara-github committed Jan 6, 2025
1 parent a87bfef commit cfda178
Show file tree
Hide file tree
Showing 9 changed files with 160 additions and 32 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,14 @@
import static com.google.devtools.build.lib.bazel.bzlmod.BazelLockFileFunction.LOCKFILE_MODE;
import static java.nio.charset.StandardCharsets.UTF_8;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSortedMap;
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
import com.google.common.flogger.GoogleLogger;
import com.google.devtools.build.lib.bazel.repository.RepositoryOptions;
import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.LockfileMode;
import com.google.devtools.build.lib.cmdline.LabelConstants;
import com.google.devtools.build.lib.runtime.BlazeModule;
Expand All @@ -36,6 +40,7 @@
import java.util.HashMap;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import java.util.function.Predicate;

/**
* Module collecting Bazel module and module extensions resolution results and updating the
Expand All @@ -45,13 +50,18 @@ public class BazelLockFileModule extends BlazeModule {

private SkyframeExecutor executor;
private Path workspaceRoot;
private LockfileMode optionsLockfileMode;

private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();

private static final ImmutableSet<LockfileMode> ENABLED_IN_MODES =
Sets.immutableEnumSet(LockfileMode.UPDATE, LockfileMode.REFRESH);

@Override
public void beforeCommand(CommandEnvironment env) {
executor = env.getSkyframeExecutor();
workspaceRoot = env.getWorkspace();
optionsLockfileMode = env.getOptions().getOptions(RepositoryOptions.class).lockfileMode;
}

@Override
Expand All @@ -67,11 +77,12 @@ public void afterCommand() {
// No command run on this server has triggered module resolution yet.
return;
}
LockfileMode lockfileMode = (LockfileMode) lockfileModeValue.get();
// Check the Skyframe value instead of the option since some commands (e.g. shutdown) don't
// propagate the options to Skyframe, but we can only operate on Skyframe values that were
// generated in UPDATE mode.
if (lockfileMode != LockfileMode.UPDATE && lockfileMode != LockfileMode.REFRESH) {
// Check the Skyframe value in addition to the option since some commands (e.g. shutdown)
// don't propagate the options to Skyframe, but we can only operate on Skyframe values that
// were generated in UPDATE mode.
LockfileMode skyframeLockfileMode = (LockfileMode) lockfileModeValue.get();
if (!(ENABLED_IN_MODES.contains(optionsLockfileMode)
&& ENABLED_IN_MODES.contains(skyframeLockfileMode))) {
return;
}
moduleResolutionValue =
Expand Down Expand Up @@ -111,7 +122,9 @@ public void afterCommand() {
});
var combinedExtensionInfos =
combineModuleExtensions(
oldLockfile.getModuleExtensions(), newExtensionInfos, depGraphValue);
oldLockfile.getModuleExtensions(),
newExtensionInfos,
/* hasUsages= */ depGraphValue.getExtensionUsagesTable()::containsRow);

// Create an updated version of the lockfile, keeping only the extension results from the old
// lockfile that are still up-to-date and adding the newly resolved extension results.
Expand All @@ -136,15 +149,14 @@ public void afterCommand() {
* Combines the old extensions stored in the lockfile -if they are still valid- with the new
* extensions from the events (if any)
*/
private ImmutableMap<
@VisibleForTesting
static ImmutableMap<
ModuleExtensionId, ImmutableMap<ModuleExtensionEvalFactors, LockFileModuleExtension>>
combineModuleExtensions(
ImmutableMap<
ModuleExtensionId,
ImmutableMap<ModuleExtensionEvalFactors, LockFileModuleExtension>>
Map<ModuleExtensionId, ImmutableMap<ModuleExtensionEvalFactors, LockFileModuleExtension>>
oldExtensionInfos,
Map<ModuleExtensionId, LockFileModuleExtension.WithFactors> newExtensionInfos,
BazelDepGraphValue depGraphValue) {
Predicate<ModuleExtensionId> hasUsages) {
Map<ModuleExtensionId, ImmutableMap<ModuleExtensionEvalFactors, LockFileModuleExtension>>
updatedExtensionMap = new HashMap<>();

Expand All @@ -153,7 +165,7 @@ public void afterCommand() {
// Other information such as transitive .bzl hash and usages hash are *not* checked here.
for (var entry : oldExtensionInfos.entrySet()) {
var moduleExtensionId = entry.getKey();
if (!depGraphValue.getExtensionUsagesTable().containsRow(moduleExtensionId)) {
if (!hasUsages.test(moduleExtensionId)) {
// Extensions without any usages are not needed anymore.
continue;
}
Expand All @@ -163,15 +175,17 @@ public void afterCommand() {
updatedExtensionMap.put(moduleExtensionId, entry.getValue());
continue;
}
if (!newExtensionInfo.moduleExtension().shouldLockExtension()) {
// Extension has become reproducible and should not be locked anymore.
continue;
}
var newFactors = newExtensionInfo.extensionFactors();
// Factor results can be individually marked as reproducible and should be dropped if so.
ImmutableSortedMap<ModuleExtensionEvalFactors, LockFileModuleExtension>
perFactorResultsToKeep =
ImmutableSortedMap.copyOf(
Maps.filterKeys(entry.getValue(), newFactors::hasSameDependenciesAs));
Maps.filterKeys(
entry.getValue(),
existingFactors ->
newFactors.hasSameDependenciesAs(existingFactors)
&& !(newFactors.equals(existingFactors)
&& newExtensionInfo.moduleExtension().isReproducible())));
if (perFactorResultsToKeep.isEmpty()) {
continue;
}
Expand All @@ -181,7 +195,7 @@ public void afterCommand() {
// Add the new resolved extensions
for (var extensionIdAndInfo : newExtensionInfos.entrySet()) {
LockFileModuleExtension extension = extensionIdAndInfo.getValue().moduleExtension();
if (!extension.shouldLockExtension()) {
if (extension.isReproducible()) {
continue;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
import static com.google.common.collect.ImmutableSet.toImmutableSet;

import com.google.common.base.Preconditions;
import com.google.common.base.Splitter;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
Expand All @@ -38,7 +37,6 @@
import com.google.devtools.build.lib.skyframe.BzlLoadFunction;
import com.google.devtools.build.lib.skyframe.BzlLoadValue;
import com.google.devtools.build.skyframe.SkyFunction.Environment;
import java.util.Iterator;
import java.util.Map.Entry;
import java.util.Optional;
import javax.annotation.Nullable;
Expand Down Expand Up @@ -104,18 +102,20 @@ static InnateRunnableExtension load(
RepositoryMapping repoMapping = usagesValue.getRepoMappings().get(moduleKey);
Label.RepoContext repoContext = Label.RepoContext.of(repoMapping.ownerRepo(), repoMapping);

// The name of the extension is of the form "<bzl_file_label>%<rule_name>".
Iterator<String> parts = Splitter.on('%').split(extensionId.extensionName()).iterator();
// The name of the extension is of the form "<bzl_file_label> <rule_name>". Rule names cannot
// contain spaces, so we can split on the last space.
int lastSpace = extensionId.extensionName().lastIndexOf(' ');
String rawLabel = extensionId.extensionName().substring(0, lastSpace);
String ruleName = extensionId.extensionName().substring(lastSpace + 1);
Location location = tags.getFirst().getLocation();
Label bzlLabel;
try {
bzlLabel = Label.parseWithRepoContext(parts.next(), repoContext);
bzlLabel = Label.parseWithRepoContext(rawLabel, repoContext);
BzlLoadFunction.checkValidLoadLabel(bzlLabel, starlarkSemantics);
} catch (LabelSyntaxException e) {
throw ExternalDepsException.withCauseAndMessage(
Code.BAD_MODULE, e, "bad repo rule .bzl file label at %s", location);
}
String ruleName = parts.next();

// Load the .bzl file.
BzlLoadValue loadedBzl;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,8 @@ public static Builder builder() {
public abstract ImmutableTable<RepositoryName, String, RepositoryName>
getRecordedRepoMappingEntries();

public boolean shouldLockExtension() {
return getModuleExtensionMetadata().isEmpty()
|| !getModuleExtensionMetadata().get().getReproducible();
public boolean isReproducible() {
return getModuleExtensionMetadata().map(ModuleExtensionMetadata::getReproducible).orElse(false);
}

/** Builder type for {@link LockFileModuleExtension}. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ public static ModuleExtensionId create(
}

public boolean isInnate() {
return extensionName().contains("%");
return extensionName().contains(" ");
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -577,7 +577,7 @@ private static void injectRepos(
new ModuleThreadContext.ModuleExtensionUsageBuilder(
context,
"//:MODULE.bazel",
"@bazel_tools//tools/build_defs/repo:local.bzl%local_repository",
"@bazel_tools//tools/build_defs/repo:local.bzl local_repository",
/* isolate= */ false);
ModuleFileGlobals.ModuleExtensionProxy extensionProxy =
new ModuleFileGlobals.ModuleExtensionProxy(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -785,7 +785,7 @@ public RepoRuleProxy useRepoRule(String bzlFile, String ruleName, StarlarkThread
ModuleThreadContext context = ModuleThreadContext.fromOrFail(thread, "use_repo_rule()");
context.setNonModuleCalled();
// Not a valid Starlark identifier so that it can't collide with a real extension.
String extensionName = bzlFile + '%' + ruleName;
String extensionName = bzlFile + ' ' + ruleName;
// Find or create the builder for the singular "innate" extension of this repo rule for this
// module.
for (ModuleExtensionUsageBuilder usageBuilder : context.getExtensionUsageBuilders()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,14 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/analysis:blaze_version_info",
"//src/main/java/com/google/devtools/build/lib/analysis:server_directories",
"//src/main/java/com/google/devtools/build/lib/authandtls",
"//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:bazel_lockfile_module",
"//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:common",
"//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:exception",
"//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:extension_eval_impl",
"//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:inspection",
"//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:inspection_impl",
"//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:module_extension",
"//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:module_extension_metadata",
"//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:registry",
"//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:repo_rule_value",
"//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:resolution",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
// Copyright 2024 The Bazel Authors. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package com.google.devtools.build.lib.bazel.bzlmod;

import static com.google.common.truth.Truth.assertThat;

import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.cmdline.Label;
import java.util.Optional;
import net.starlark.java.eval.Starlark;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;

/** Tests for {@link BazelLockFileModule}. */
@RunWith(JUnit4.class)
public class BazelLockFileModuleTest {

private ModuleExtensionId extensionId;
private LockFileModuleExtension nonReproducibleResult;
private LockFileModuleExtension reproducibleResult;
private ModuleExtensionEvalFactors evalFactors;
private ModuleExtensionEvalFactors otherEvalFactors;

@Before
public void setUp() throws Exception {
extensionId =
ModuleExtensionId.create(
Label.parseCanonicalUnchecked("//:ext.bzl"), "ext", Optional.empty());
nonReproducibleResult =
LockFileModuleExtension.builder()
.setBzlTransitiveDigest(new byte[] {1, 2, 3})
.setUsagesDigest(new byte[] {4, 5, 6})
.setRecordedFileInputs(ImmutableMap.of())
.setRecordedDirentsInputs(ImmutableMap.of())
.setEnvVariables(ImmutableMap.of())
.setGeneratedRepoSpecs(ImmutableMap.of())
.build();
reproducibleResult =
LockFileModuleExtension.builder()
.setBzlTransitiveDigest(new byte[] {1, 2, 3})
.setUsagesDigest(new byte[] {4, 5, 6})
.setRecordedFileInputs(ImmutableMap.of())
.setRecordedDirentsInputs(ImmutableMap.of())
.setEnvVariables(ImmutableMap.of())
.setGeneratedRepoSpecs(ImmutableMap.of())
.setModuleExtensionMetadata(
Optional.of(
ModuleExtensionMetadata.create(
Starlark.NONE, Starlark.NONE, /* reproducible= */ true)))
.build();
evalFactors = ModuleExtensionEvalFactors.create("linux", "x86_64");
otherEvalFactors = ModuleExtensionEvalFactors.create("linux", "aarch64");
}

@Test
public void combineModuleExtensionsReproducibleFactorAdded() {
var oldExtensionInfos =
ImmutableMap.of(extensionId, ImmutableMap.of(evalFactors, nonReproducibleResult));
var newExtensionInfos =
ImmutableMap.of(
extensionId,
new LockFileModuleExtension.WithFactors(otherEvalFactors, reproducibleResult));

assertThat(
BazelLockFileModule.combineModuleExtensions(
oldExtensionInfos, newExtensionInfos, id -> true))
.isEqualTo(oldExtensionInfos);
}

@Test
public void combineModuleExtensionsFactorBecomesReproducible() {
var oldExtensionInfos =
ImmutableMap.of(extensionId, ImmutableMap.of(evalFactors, nonReproducibleResult));
var newExtensionInfos =
ImmutableMap.of(
extensionId, new LockFileModuleExtension.WithFactors(evalFactors, reproducibleResult));

assertThat(
BazelLockFileModule.combineModuleExtensions(
oldExtensionInfos, newExtensionInfos, id -> true))
.isEmpty();
}

@Test
public void combineModuleExtensionsFactorBecomesNonReproducible() {
var oldExtensionInfos =
ImmutableMap.of(extensionId, ImmutableMap.of(evalFactors, reproducibleResult));
var newExtensionInfos =
ImmutableMap.of(
extensionId,
new LockFileModuleExtension.WithFactors(evalFactors, nonReproducibleResult));

assertThat(
BazelLockFileModule.combineModuleExtensions(
oldExtensionInfos, newExtensionInfos, id -> true))
.isEqualTo(
ImmutableMap.of(extensionId, ImmutableMap.of(evalFactors, nonReproducibleResult)));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1205,7 +1205,7 @@ public void testModuleExtensions_innate() throws Exception {
.addExtensionUsage(
ModuleExtensionUsage.builder()
.setExtensionBzlFile("//:MODULE.bazel")
.setExtensionName("//:repo.bzl%repo")
.setExtensionName("//:repo.bzl repo")
.setIsolationKey(Optional.empty())
.setRepoOverrides(ImmutableMap.of())
.addProxy(
Expand Down Expand Up @@ -1234,7 +1234,7 @@ public void testModuleExtensions_innate() throws Exception {
.addExtensionUsage(
ModuleExtensionUsage.builder()
.setExtensionBzlFile("//:MODULE.bazel")
.setExtensionName("@bazel_tools//:http.bzl%http_archive")
.setExtensionName("@bazel_tools//:http.bzl http_archive")
.setIsolationKey(Optional.empty())
.setRepoOverrides(ImmutableMap.of())
.addProxy(
Expand Down

0 comments on commit cfda178

Please sign in to comment.