Skip to content

Commit

Permalink
Make repo marker files sensitive to repo mapping changes
Browse files Browse the repository at this point in the history
Similar to the fix for bazelbuild#20721, we write recorded repo mapping entries into the marker file so that repo fetching is sensitive to any relevant repo mapping changes.

Fixes bazelbuild#20722.

Closes bazelbuild#21131.

PiperOrigin-RevId: 603310262
Change-Id: I806f383e8579fed3533fac9b181efd8b75e76fcd
  • Loading branch information
Wyverald authored and bazel-io committed Feb 1, 2024
1 parent 548c171 commit 1745889
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 1745889

Please sign in to comment.