From d7b576d405ce762b0baff1956a8efced95396e24 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?X=C3=B9d=C5=8Dng=20Y=C3=A1ng?= Date: Mon, 29 Jan 2024 18:13:54 -0500 Subject: [PATCH 1/4] Make repo marker files sensitive to repo mapping changes Similar to the fix for https://github.com/bazelbuild/bazel/issues/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. --- .../starlark/StarlarkRepositoryFunction.java | 8 ++ .../starlark/StarlarkRepositoryModule.java | 4 + .../build/lib/packages/RuleClass.java | 22 ++++- .../com/google/devtools/build/lib/rules/BUILD | 1 + .../RepositoryDelegatorFunction.java | 2 +- .../rules/repository/RepositoryFunction.java | 41 ++++++++- .../build/lib/packages/RuleClassTest.java | 1 + .../shell/bazel/starlark_repository_test.sh | 89 +++++++++++++++++++ 8 files changed, 163 insertions(+), 5 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java index 0bc2d7b5dfbd92..ff4c09257a1dcd 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java @@ -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; @@ -240,6 +241,9 @@ 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") @@ -328,6 +332,10 @@ private RepositoryDirectoryValue.Builder fetchInternal( markerData.put("ENV:" + envKey, clientEnvironment.get(envKey)); } + for (Table.Cell 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 diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java index 8ae318f024de68..3cf800117aae36 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java @@ -112,6 +112,10 @@ 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, diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java index 550cd94c2db182..381229ca0933fa 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java +++ b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java @@ -29,6 +29,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.ImmutableTable; import com.google.common.collect.Interner; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; @@ -38,6 +39,7 @@ import com.google.devtools.build.lib.analysis.config.transitions.TransitionFactory; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.LabelSyntaxException; +import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.events.EventHandler; import com.google.devtools.build.lib.packages.Attribute.ComputedDefault; import com.google.devtools.build.lib.packages.Attribute.StarlarkComputedDefaultTemplate; @@ -786,8 +788,9 @@ public String toString() { NO_OPTION_REFERENCE; /** This field and the next are null iff the rule is native. */ @Nullable private Label ruleDefinitionEnvironmentLabel; - @Nullable private byte[] ruleDefinitionEnvironmentDigest = null; + /** This filed is non-null iff the rule is a Starlark repo rule. */ + @Nullable private ImmutableTable ruleDefinitionEnvironmentRepoMappingEntries; private final ConfigurationFragmentPolicy.Builder configurationFragmentPolicy = new ConfigurationFragmentPolicy.Builder(); @@ -984,6 +987,7 @@ public RuleClass build(String name, String key) { optionReferenceFunction, ruleDefinitionEnvironmentLabel, ruleDefinitionEnvironmentDigest, + ruleDefinitionEnvironmentRepoMappingEntries, configurationFragmentPolicy.build(), supportsConstraintChecking, toolchainTypes, @@ -1410,6 +1414,13 @@ public Label getRuleDefinitionEnvironmentLabel() { return this.ruleDefinitionEnvironmentLabel; } + @CanIgnoreReturnValue + public Builder setRuleDefinitionEnvironmentRepoMappingEntries( + ImmutableTable recordedRepoMappingEntries) { + this.ruleDefinitionEnvironmentRepoMappingEntries = recordedRepoMappingEntries; + return this; + } + /** * Removes an attribute with the same name from this rule class. * @@ -1762,8 +1773,8 @@ public Attribute.Builder copy(String name) { * Starlark executable RuleClasses. */ @Nullable private final Label ruleDefinitionEnvironmentLabel; - @Nullable private final byte[] ruleDefinitionEnvironmentDigest; + @Nullable private final ImmutableTable ruleDefinitionEnvironmentRepoMappingEntries; private final OutputFile.Kind outputFileKind; /** @@ -1835,6 +1846,7 @@ public Attribute.Builder copy(String name) { Function> optionReferenceFunction, @Nullable Label ruleDefinitionEnvironmentLabel, @Nullable byte[] ruleDefinitionEnvironmentDigest, + @Nullable ImmutableTable ruleDefinitionEnvironmentRepoMappingEntries, ConfigurationFragmentPolicy configurationFragmentPolicy, boolean supportsConstraintChecking, Set toolchainTypes, @@ -1870,6 +1882,7 @@ public Attribute.Builder copy(String name) { this.optionReferenceFunction = optionReferenceFunction; this.ruleDefinitionEnvironmentLabel = ruleDefinitionEnvironmentLabel; this.ruleDefinitionEnvironmentDigest = ruleDefinitionEnvironmentDigest; + this.ruleDefinitionEnvironmentRepoMappingEntries = ruleDefinitionEnvironmentRepoMappingEntries; this.outputFileKind = outputFileKind; this.attributes = attributes; this.workspaceOnly = workspaceOnly; @@ -2635,6 +2648,11 @@ public byte[] getRuleDefinitionEnvironmentDigest() { return ruleDefinitionEnvironmentDigest; } + @Nullable + public ImmutableTable getRuleDefinitionEnvironmentRepoMappingEntries() { + return ruleDefinitionEnvironmentRepoMappingEntries; + } + /** Returns true if this RuleClass is a Starlark-defined RuleClass. */ @Override public boolean isStarlark() { diff --git a/src/main/java/com/google/devtools/build/lib/rules/BUILD b/src/main/java/com/google/devtools/build/lib/rules/BUILD index 768f7b0f724449..4824d75377efbb 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/BUILD +++ b/src/main/java/com/google/devtools/build/lib/rules/BUILD @@ -415,6 +415,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", diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java index ab735e2d462f73..e4977a8d8b3ed8 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java @@ -93,7 +93,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 handlers; diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java index c5ab58ca1a7c09..8004e4c0a87649 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java @@ -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; @@ -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; @@ -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; @@ -205,8 +208,9 @@ private static ImmutableSet getEnviron(Rule rule) { public boolean verifyMarkerData(Rule rule, Map 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 markerData, Environment env) @@ -214,6 +218,39 @@ protected boolean verifySemanticsMarkerData(Map markerData, Envi return true; } + private static boolean verifyRepoMappingMarkerData(Map markerData, + Environment env) + throws InterruptedException { + ImmutableSet 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 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:")); diff --git a/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java b/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java index b840fea63995b3..e1f644268f5981 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java @@ -1053,6 +1053,7 @@ private static RuleClass newRuleClass( /* optionReferenceFunction= */ RuleClass.NO_OPTION_REFERENCE, /* ruleDefinitionEnvironmentLabel= */ null, /* ruleDefinitionEnvironmentDigest= */ null, + /* ruleDefinitionEnvironmentRepoMappingEntries= */ null, new ConfigurationFragmentPolicy.Builder() .requiresConfigurationFragments(allowedConfigurationFragments) .build(), diff --git a/src/test/shell/bazel/starlark_repository_test.sh b/src/test/shell/bazel/starlark_repository_test.sh index 2a4e2a763b484d..12170039ec80a1 100755 --- a/src/test/shell/bazel/starlark_repository_test.sh +++ b/src/test/shell/bazel/starlark_repository_test.sh @@ -2615,4 +2615,93 @@ EOF assert_contains 'WORKSPACE' output } +function test_repo_mapping_change_in_rule_impl() { + # regression test for #20722 + create_new_workspace + cat > MODULE.bazel < r.bzl < foo/MODULE.bazel < bar/MODULE.bazel <& $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 <& $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 < r.bzl < foo/MODULE.bazel < bar/MODULE.bazel <& $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 <& $TEST_log || fail "expected bazel to succeed" + expect_log "I see: @@bar~override//:data" +} + run_suite "local repository tests" From 707a975ff7c6100cf604d43f58cb4210eea2bc19 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?X=C3=B9d=C5=8Dng=20Y=C3=A1ng?= Date: Tue, 30 Jan 2024 08:46:56 -0500 Subject: [PATCH 2/4] Update src/main/java/com/google/devtools/build/lib/packages/RuleClass.java Co-authored-by: Fabian Meumertzheim --- .../java/com/google/devtools/build/lib/packages/RuleClass.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java index 381229ca0933fa..21205cf797f499 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java +++ b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java @@ -789,7 +789,7 @@ public String toString() { /** This field and the next are null iff the rule is native. */ @Nullable private Label ruleDefinitionEnvironmentLabel; @Nullable private byte[] ruleDefinitionEnvironmentDigest = null; - /** This filed is non-null iff the rule is a Starlark repo rule. */ + /** This field is non-null iff the rule is a Starlark repo rule. */ @Nullable private ImmutableTable ruleDefinitionEnvironmentRepoMappingEntries; private final ConfigurationFragmentPolicy.Builder configurationFragmentPolicy = new ConfigurationFragmentPolicy.Builder(); From eae938e8f683468de7462f00e720592d2c425cce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?X=C3=B9d=C5=8Dng=20Y=C3=A1ng?= Date: Tue, 30 Jan 2024 15:50:12 -0500 Subject: [PATCH 3/4] fix TODO in repository_rule --- .../repository/starlark/StarlarkRepositoryModule.java | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java index 3cf800117aae36..c9d09652abe710 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java @@ -76,7 +76,7 @@ public StarlarkCallable repositoryRule( Object doc, // or Starlark.NONE StarlarkThread thread) throws EvalException { - BazelStarlarkContext.checkLoadingOrWorkspacePhase(thread, "repository_rule"); + var bzlInitContext = BzlInitThreadContext.fromOrFail(thread, "repository_rule"); // We'll set the name later, pass the empty string for now. RuleClass.Builder builder = new RuleClass.Builder("", RuleClassType.WORKSPACE, true); @@ -107,11 +107,8 @@ public StarlarkCallable repositoryRule( } } builder.setConfiguredTargetFunction(implementation); - // TODO(b/291752414): If we care about the digest of repository rules, we should be using the - // transitive bzl digest of the module of the outermost stack frame, not the innermost. - BazelModuleContext moduleContext = BazelModuleContext.ofInnermostBzlOrThrow(thread); builder.setRuleDefinitionEnvironmentLabelAndDigest( - moduleContext.label(), moduleContext.bzlTransitiveDigest()); + bzlInitContext.getBzlFile(), bzlInitContext.getTransitiveDigest()); Label.RepoMappingRecorder repoMappingRecorder = thread.getThreadLocal(Label.RepoMappingRecorder.class); if (repoMappingRecorder != null) { builder.setRuleDefinitionEnvironmentRepoMappingEntries(repoMappingRecorder.recordedEntries()); From 9e72e068a3822086479c19a815f897d66b43e645 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?X=C3=B9d=C5=8Dng=20Y=C3=A1ng?= Date: Tue, 30 Jan 2024 16:30:15 -0500 Subject: [PATCH 4/4] Revert "fix TODO in repository_rule" This reverts commit eae938e8f683468de7462f00e720592d2c425cce. --- .../repository/starlark/StarlarkRepositoryModule.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java index c9d09652abe710..3cf800117aae36 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java @@ -76,7 +76,7 @@ public StarlarkCallable repositoryRule( Object doc, // or Starlark.NONE StarlarkThread thread) throws EvalException { - var bzlInitContext = BzlInitThreadContext.fromOrFail(thread, "repository_rule"); + BazelStarlarkContext.checkLoadingOrWorkspacePhase(thread, "repository_rule"); // We'll set the name later, pass the empty string for now. RuleClass.Builder builder = new RuleClass.Builder("", RuleClassType.WORKSPACE, true); @@ -107,8 +107,11 @@ public StarlarkCallable repositoryRule( } } builder.setConfiguredTargetFunction(implementation); + // TODO(b/291752414): If we care about the digest of repository rules, we should be using the + // transitive bzl digest of the module of the outermost stack frame, not the innermost. + BazelModuleContext moduleContext = BazelModuleContext.ofInnermostBzlOrThrow(thread); builder.setRuleDefinitionEnvironmentLabelAndDigest( - bzlInitContext.getBzlFile(), bzlInitContext.getTransitiveDigest()); + moduleContext.label(), moduleContext.bzlTransitiveDigest()); Label.RepoMappingRecorder repoMappingRecorder = thread.getThreadLocal(Label.RepoMappingRecorder.class); if (repoMappingRecorder != null) { builder.setRuleDefinitionEnvironmentRepoMappingEntries(repoMappingRecorder.recordedEntries());