Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[7.1.0] Make repo marker files sensitive to repo mapping changes #21172

Merged
merged 1 commit into from
Feb 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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"
Loading