Skip to content

Commit

Permalink
[7.1.0] Make repo marker files sensitive to repo mapping changes (#21172
Browse files Browse the repository at this point in the history
)

Similar to the fix for #20721,
we write recorded repo mapping entries into the marker file so that repo
fetching is sensitive to any relevant repo mapping changes.

Fixes #20722.

Closes #21131.

Commit
9edaddd

PiperOrigin-RevId: 603310262
Change-Id: I806f383e8579fed3533fac9b181efd8b75e76fcd

Co-authored-by: Xdng Yng <wyverald@gmail.com>
  • Loading branch information
bazel-io and Wyverald authored Feb 1, 2024
1 parent 548c171 commit 2f4bad0
Show file tree
Hide file tree
Showing 8 changed files with 317 additions and 145 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Table;
import com.google.devtools.build.lib.analysis.BlazeDirectories;
import com.google.devtools.build.lib.analysis.RuleDefinition;
import com.google.devtools.build.lib.bazel.repository.RepositoryResolvedEvent;
Expand Down Expand Up @@ -242,6 +243,10 @@ private RepositoryDirectoryValue.Builder fetchInternal(
try (Mutability mu = Mutability.create("Starlark repository")) {
StarlarkThread thread = new StarlarkThread(mu, starlarkSemantics);
thread.setPrintHandler(Event.makeDebugPrintHandler(env.getListener()));
var repoMappingRecorder = new Label.RepoMappingRecorder();
repoMappingRecorder.mergeEntries(
rule.getRuleClassObject().getRuleDefinitionEnvironmentRepoMappingEntries());
thread.setThreadLocal(Label.RepoMappingRecorder.class, repoMappingRecorder);

new BazelStarlarkContext(
BazelStarlarkContext.Phase.LOADING, // ("fetch")
Expand Down Expand Up @@ -330,6 +335,16 @@ private RepositoryDirectoryValue.Builder fetchInternal(
markerData.put("ENV:" + envKey, clientEnvironment.get(envKey));
}

for (Table.Cell<RepositoryName, String, RepositoryName> repoMappings :
repoMappingRecorder.recordedEntries().cellSet()) {
markerData.put(
"REPO_MAPPING:"
+ repoMappings.getRowKey().getName()
+ ","
+ repoMappings.getColumnKey(),
repoMappings.getValue().getName());
}

env.getListener().post(resolved);
} catch (NeedsSkyframeRestartException e) {
// A dependency is missing, cleanup and returns null
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,11 @@ public StarlarkCallable repositoryRule(
BazelModuleContext moduleContext = BazelModuleContext.ofInnermostBzlOrThrow(thread);
builder.setRuleDefinitionEnvironmentLabelAndDigest(
moduleContext.label(), moduleContext.bzlTransitiveDigest());
Label.RepoMappingRecorder repoMappingRecorder =
thread.getThreadLocal(Label.RepoMappingRecorder.class);
if (repoMappingRecorder != null) {
builder.setRuleDefinitionEnvironmentRepoMappingEntries(repoMappingRecorder.recordedEntries());
}
builder.setWorkspaceOnly();
return new RepositoryRuleFunction(
builder,
Expand Down
308 changes: 166 additions & 142 deletions src/main/java/com/google/devtools/build/lib/packages/RuleClass.java

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/rules/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -414,6 +414,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/skyframe:package_lookup_function",
"//src/main/java/com/google/devtools/build/lib/skyframe:package_lookup_value",
"//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_value",
"//src/main/java/com/google/devtools/build/lib/skyframe:repository_mapping_value",
"//src/main/java/com/google/devtools/build/lib/skyframe:sky_functions",
"//src/main/java/com/google/devtools/build/lib/skyframe:sky_value_dirtiness_checker",
"//src/main/java/com/google/devtools/build/lib/util",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ public final class RepositoryDelegatorFunction implements SkyFunction {

// The marker file version is inject in the rule key digest so the rule key is always different
// when we decide to update the format.
private static final int MARKER_FILE_VERSION = 3;
private static final int MARKER_FILE_VERSION = 4;

// Mapping of rule class name to RepositoryFunction.
private final ImmutableMap<String, RepositoryFunction> handlers;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package com.google.devtools.build.lib.rules.repository;

import static com.google.common.base.Preconditions.checkState;
import static com.google.common.collect.ImmutableSet.toImmutableSet;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
Expand All @@ -40,6 +41,7 @@
import com.google.devtools.build.lib.skyframe.PackageLookupFunction;
import com.google.devtools.build.lib.skyframe.PackageLookupValue;
import com.google.devtools.build.lib.skyframe.PrecomputedValue;
import com.google.devtools.build.lib.skyframe.RepositoryMappingValue;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
Expand All @@ -50,6 +52,7 @@
import com.google.devtools.build.skyframe.SkyFunctionException;
import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyframeLookupResult;
import java.io.IOException;
import java.util.Collection;
import java.util.LinkedHashMap;
Expand Down Expand Up @@ -205,15 +208,49 @@ private static ImmutableSet<String> getEnviron(Rule rule) {
public boolean verifyMarkerData(Rule rule, Map<String, String> markerData, Environment env)
throws InterruptedException {
return verifyEnvironMarkerData(markerData, env, getEnviron(rule))
&& verifyMarkerDataForFiles(rule, markerData, env)
&& verifySemanticsMarkerData(markerData, env);
&& verifySemanticsMarkerData(markerData, env)
&& verifyRepoMappingMarkerData(markerData, env)
&& verifyMarkerDataForFiles(rule, markerData, env);
}

protected boolean verifySemanticsMarkerData(Map<String, String> markerData, Environment env)
throws InterruptedException {
return true;
}

private static boolean verifyRepoMappingMarkerData(
Map<String, String> markerData, Environment env) throws InterruptedException {
ImmutableSet<SkyKey> skyKeys =
markerData.keySet().stream()
.filter(k -> k.startsWith("REPO_MAPPING:"))
// grab the part after the 'REPO_MAPPING:' and before the comma
.map(k -> k.substring("REPO_MAPPING:".length(), k.indexOf(',')))
.map(k -> RepositoryMappingValue.key(RepositoryName.createUnvalidated(k)))
.collect(toImmutableSet());
SkyframeLookupResult result = env.getValuesAndExceptions(skyKeys);
if (env.valuesMissing()) {
return false;
}
for (Map.Entry<String, String> entry : markerData.entrySet()) {
if (!entry.getKey().startsWith("REPO_MAPPING:")) {
continue;
}
int commaIndex = entry.getKey().indexOf(',');
RepositoryName fromRepo =
RepositoryName.createUnvalidated(
entry.getKey().substring("REPO_MAPPING:".length(), commaIndex));
String apparentRepoName = entry.getKey().substring(commaIndex + 1);
RepositoryMappingValue repoMappingValue =
(RepositoryMappingValue) result.get(RepositoryMappingValue.key(fromRepo));
if (repoMappingValue == RepositoryMappingValue.NOT_FOUND_VALUE
|| !RepositoryName.createUnvalidated(entry.getValue())
.equals(repoMappingValue.getRepositoryMapping().get(apparentRepoName))) {
return false;
}
}
return true;
}

private static boolean verifyLabelMarkerData(Rule rule, String key, String value, Environment env)
throws InterruptedException {
Preconditions.checkArgument(key.startsWith("FILE:"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1050,6 +1050,7 @@ private static RuleClass newRuleClass(
/* optionReferenceFunction= */ RuleClass.NO_OPTION_REFERENCE,
/* ruleDefinitionEnvironmentLabel= */ null,
/* ruleDefinitionEnvironmentDigest= */ null,
/* ruleDefinitionEnvironmentRepoMappingEntries= */ null,
new ConfigurationFragmentPolicy.Builder()
.requiresConfigurationFragments(allowedConfigurationFragments)
.build(),
Expand Down
89 changes: 89 additions & 0 deletions src/test/shell/bazel/starlark_repository_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -2611,4 +2611,93 @@ EOF
assert_contains 'WORKSPACE' output
}

function test_repo_mapping_change_in_rule_impl() {
# regression test for #20722
create_new_workspace
cat > MODULE.bazel <<EOF
r = use_repo_rule("//:r.bzl", "r")
r(name = "r")
bazel_dep(name="foo", repo_name="data")
local_path_override(module_name="foo", path="foo")
bazel_dep(name="bar")
local_path_override(module_name="bar", path="bar")
EOF
touch BUILD
cat > r.bzl <<EOF
def _r(rctx):
print("I see: " + str(Label("@data")))
rctx.file("BUILD", "filegroup(name='r')")
r=repository_rule(_r)
EOF
mkdir foo
cat > foo/MODULE.bazel <<EOF
module(name="foo")
EOF
mkdir bar
cat > bar/MODULE.bazel <<EOF
module(name="bar")
EOF

bazel build @r >& $TEST_log || fail "expected bazel to succeed"
expect_log "I see: @@foo~override//:data"

# So far, so good. Now we make `@data` point to bar instead!
cat > MODULE.bazel <<EOF
r = use_repo_rule("//:r.bzl", "r")
r(name = "r")
bazel_dep(name="foo")
local_path_override(module_name="foo", path="foo")
bazel_dep(name="bar", repo_name="data")
local_path_override(module_name="bar", path="bar")
EOF
# for the repo `r`, nothing except the repo mapping has changed.
bazel build @r >& $TEST_log || fail "expected bazel to succeed"
expect_log "I see: @@bar~override//:data"
}

function test_repo_mapping_change_in_bzl_init() {
# same as above, but tests .bzl init time repo mapping usages
create_new_workspace
cat > MODULE.bazel <<EOF
r = use_repo_rule("//:r.bzl", "r")
r(name = "r")
bazel_dep(name="foo", repo_name="data")
local_path_override(module_name="foo", path="foo")
bazel_dep(name="bar")
local_path_override(module_name="bar", path="bar")
EOF
touch BUILD
cat > r.bzl <<EOF
CONSTANT = Label("@data")
def _r(rctx):
print("I see: " + str(CONSTANT))
rctx.file("BUILD", "filegroup(name='r')")
r=repository_rule(_r)
EOF
mkdir foo
cat > foo/MODULE.bazel <<EOF
module(name="foo")
EOF
mkdir bar
cat > bar/MODULE.bazel <<EOF
module(name="bar")
EOF

bazel build @r >& $TEST_log || fail "expected bazel to succeed"
expect_log "I see: @@foo~override//:data"

# So far, so good. Now we make `@data` point to bar instead!
cat > MODULE.bazel <<EOF
r = use_repo_rule("//:r.bzl", "r")
r(name = "r")
bazel_dep(name="foo")
local_path_override(module_name="foo", path="foo")
bazel_dep(name="bar", repo_name="data")
local_path_override(module_name="bar", path="bar")
EOF
# for the repo `r`, nothing except the repo mapping has changed.
bazel build @r >& $TEST_log || fail "expected bazel to succeed"
expect_log "I see: @@bar~override//:data"
}

run_suite "local repository tests"

0 comments on commit 2f4bad0

Please sign in to comment.