From 1745889e6e398cc1e55022b61d3ac54c5d6086a6 Mon Sep 17 00:00:00 2001 From: Xdng Yng Date: Thu, 1 Feb 2024 02:42:33 -0800 Subject: [PATCH] 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. Closes #21131. PiperOrigin-RevId: 603310262 Change-Id: I806f383e8579fed3533fac9b181efd8b75e76fcd --- .../starlark/StarlarkRepositoryFunction.java | 15 + .../starlark/StarlarkRepositoryModule.java | 5 + .../build/lib/packages/RuleClass.java | 308 ++++++++++-------- .../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, 317 insertions(+), 145 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 be0c3e1f656027..a46d92d7ded829 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; @@ -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") @@ -330,6 +335,16 @@ 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 86694ea38f3ae2..dcc3b1ca30aa59 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 @@ -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, 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 3b447a65b332a2..0ed3b6d925bd53 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.events.NullEventHandler; import com.google.devtools.build.lib.packages.Attribute.ComputedDefault; @@ -161,9 +163,7 @@ public class RuleClass implements RuleClassData { */ public static final String APPLICABLE_LICENSES_ATTR = "applicable_licenses"; - /** - * A constraint for the package name of the Rule instances. - */ + /** A constraint for the package name of the Rule instances. */ public static class PackageNameConstraint implements PredicateWithMessage { public static final int ANY_SEGMENT = 0; @@ -173,8 +173,8 @@ public static class PackageNameConstraint implements PredicateWithMessage private final Set values; /** - * The pathSegment-th segment of the package must be one of the specified values. - * The path segment indexing starts from 1. + * The pathSegment-th segment of the package must be one of the specified values. The path + * segment indexing starts from 1. */ public PackageNameConstraint(int pathSegment, String... values) { this.values = ImmutableSet.copyOf(values); @@ -195,15 +195,20 @@ public boolean apply(Rule input) { @Override public String getErrorReason(Rule param) { if (pathSegment == ANY_SEGMENT) { - return param.getRuleClass() + " rules have to be under a " - + StringUtil.joinEnglishList(values, "or", "'") + " directory"; + return param.getRuleClass() + + " rules have to be under a " + + StringUtil.joinEnglishList(values, "or", "'") + + " directory"; } else if (pathSegment == 1) { return param.getRuleClass() + " rules are only allowed in " + StringUtil.joinEnglishList(Iterables.transform(values, s -> "//" + s), "or"); } else { - return param.getRuleClass() + " rules are only allowed in packages which " - + StringUtil.ordinal(pathSegment) + " is " + StringUtil.joinEnglishList(values, "or"); + return param.getRuleClass() + + " rules are only allowed in packages which " + + StringUtil.ordinal(pathSegment) + + " is " + + StringUtil.joinEnglishList(values, "or"); } } @@ -277,7 +282,7 @@ boolean isActive() { /** A factory or builder class for rule implementations. */ public interface ConfiguredTargetFactory< - TConfiguredTarget, TContext, TActionConflictException extends Throwable> { + ConfiguredTargetT, ContextT, ActionConflictExceptionT extends Throwable> { /** * Returns a fully initialized configured target instance using the given context, or {@code * null} on certain rule errors (typically if {@code ruleContext.hasErrors()} becomes {@code @@ -288,8 +293,8 @@ public interface ConfiguredTargetFactory< * @throws TActionConflictException if there were conflicts during action registration */ @Nullable - TConfiguredTarget create(TContext ruleContext) - throws InterruptedException, RuleErrorException, TActionConflictException; + ConfiguredTargetT create(ContextT ruleContext) + throws InterruptedException, RuleErrorException, ActionConflictExceptionT; /** * Exception indicating that configured target creation could not be completed. General error @@ -391,27 +396,24 @@ public RuleErrorException(String message, Throwable cause) { public static final String CONFIG_SETTING_DEPS_ATTRIBUTE = "$config_dependencies"; /** - * A support class to make it easier to create {@code RuleClass} instances. - * This class follows the 'fluent builder' pattern. + * A support class to make it easier to create {@code RuleClass} instances. This class follows the + * 'fluent builder' pattern. * - *

The {@link #addAttribute} method will throw an exception if an attribute - * of that name already exists. Use {@link #overrideAttribute} in that case. + *

The {@link #addAttribute} method will throw an exception if an attribute of that name + * already exists. Use {@link #overrideAttribute} in that case. */ public static final class Builder { private static final Pattern RULE_NAME_PATTERN = Pattern.compile("[A-Za-z_][A-Za-z0-9_]*"); - /** - * The type of the rule class, which determines valid names and required - * attributes. - */ + /** The type of the rule class, which determines valid names and required attributes. */ public enum RuleClassType { /** - * Abstract rules are intended for rule classes that are just used to - * factor out common attributes, and for rule classes that are used only - * internally. These rules cannot be instantiated by a BUILD file. + * Abstract rules are intended for rule classes that are just used to factor out common + * attributes, and for rule classes that are used only internally. These rules cannot be + * instantiated by a BUILD file. * - *

The rule name must contain a '$' and {@link - * TargetUtils#isTestRuleName} must return false for the name. + *

The rule name must contain a '$' and {@link TargetUtils#isTestRuleName} must return + * false for the name. */ ABSTRACT { @Override @@ -427,9 +429,8 @@ public void checkAttributes(Map attributes) { }, /** - * Invisible rule classes should contain a dollar sign so that they cannot be instantiated - * by the user. They are different from abstract rules in that they can be instantiated - * at will. + * Invisible rule classes should contain a dollar sign so that they cannot be instantiated by + * the user. They are different from abstract rules in that they can be instantiated at will. */ INVISIBLE { @Override @@ -444,34 +445,40 @@ public void checkAttributes(Map attributes) { }, /** - * Normal rules are instantiable by BUILD files. Their names must therefore - * obey the rules for identifiers in the BUILD language. In addition, - * {@link TargetUtils#isTestRuleName} must return false for the name. + * Normal rules are instantiable by BUILD files. Their names must therefore obey the rules for + * identifiers in the BUILD language. In addition, {@link TargetUtils#isTestRuleName} must + * return false for the name. */ NORMAL { @Override public void checkName(String name) { Preconditions.checkArgument( !TargetUtils.isTestRuleName(name) && RULE_NAME_PATTERN.matcher(name).matches(), - "Invalid rule name: %s", name); + "Invalid rule name: %s", + name); } @Override public void checkAttributes(Map attributes) { for (Attribute attribute : REQUIRED_ATTRIBUTES_FOR_NORMAL_RULES) { Attribute presentAttribute = attributes.get(attribute.getName()); - Preconditions.checkState(presentAttribute != null, - "Missing mandatory '%s' attribute in normal rule class.", attribute.getName()); - Preconditions.checkState(presentAttribute.getType().equals(attribute.getType()), + Preconditions.checkState( + presentAttribute != null, + "Missing mandatory '%s' attribute in normal rule class.", + attribute.getName()); + Preconditions.checkState( + presentAttribute.getType().equals(attribute.getType()), "Mandatory attribute '%s' in normal rule class has incorrect type (expected" - + " %s).", attribute.getName(), attribute.getType()); + + " %s).", + attribute.getName(), + attribute.getType()); } } }, /** - * Workspace rules can only be instantiated from a WORKSPACE file. Their names obey the - * rule for identifiers. + * Workspace rules can only be instantiated from a WORKSPACE file. Their names obey the rule + * for identifiers. */ WORKSPACE { @Override @@ -486,10 +493,9 @@ public void checkAttributes(Map attributes) { }, /** - * Test rules are instantiable by BUILD files and are handled specially - * when run with the 'test' command. Their names must obey the rules - * for identifiers in the BUILD language and {@link - * TargetUtils#isTestRuleName} must return true for the name. + * Test rules are instantiable by BUILD files and are handled specially when run with the + * 'test' command. Their names must obey the rules for identifiers in the BUILD language and + * {@link TargetUtils#isTestRuleName} must return true for the name. * *

In addition, test rules must contain certain attributes. See {@link * Builder#REQUIRED_ATTRIBUTES_FOR_TESTS}. @@ -497,19 +503,23 @@ public void checkAttributes(Map attributes) { TEST { @Override public void checkName(String name) { - Preconditions.checkArgument(TargetUtils.isTestRuleName(name) - && RULE_NAME_PATTERN.matcher(name).matches()); + Preconditions.checkArgument( + TargetUtils.isTestRuleName(name) && RULE_NAME_PATTERN.matcher(name).matches()); } @Override public void checkAttributes(Map attributes) { for (Attribute attribute : REQUIRED_ATTRIBUTES_FOR_TESTS) { Attribute presentAttribute = attributes.get(attribute.getName()); - Preconditions.checkState(presentAttribute != null, - "Missing mandatory '%s' attribute in test rule class.", attribute.getName()); - Preconditions.checkState(presentAttribute.getType().equals(attribute.getType()), + Preconditions.checkState( + presentAttribute != null, + "Missing mandatory '%s' attribute in test rule class.", + attribute.getName()); + Preconditions.checkState( + presentAttribute.getType().equals(attribute.getType()), "Mandatory attribute '%s' in test rule class has incorrect type (expected %s).", - attribute.getName(), attribute.getType()); + attribute.getName(), + attribute.getType()); } } }, @@ -521,7 +531,7 @@ public void checkAttributes(Map attributes) { * class is deserialized, the rule is assigned a placeholder rule class. This is compatible * with our limited set of package serialization use cases. * - * Placeholder rule class names obey the rule for identifiers. + *

Placeholder rule class names obey the rule for identifiers. */ PLACEHOLDER { @Override @@ -784,10 +794,17 @@ public String toString() { NO_TOOLCHAINS_TO_REGISTER; private Function> optionReferenceFunction = 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 field is non-null iff the rule is a Starlark repo rule. */ + @Nullable + private ImmutableTable + ruleDefinitionEnvironmentRepoMappingEntries; + private final ConfigurationFragmentPolicy.Builder configurationFragmentPolicy = new ConfigurationFragmentPolicy.Builder(); @@ -869,8 +886,10 @@ public Builder(String name, RuleClassType type, boolean starlark, RuleClass... p // TODO(bazel-team): move this testonly attribute setting to somewhere else // preferably to some base RuleClass implementation. if (this.type.equals(RuleClassType.TEST)) { - Attribute.Builder testOnlyAttr = attr("testonly", BOOLEAN).value(true) - .nonconfigurable("policy decision: this shouldn't depend on the configuration"); + Attribute.Builder testOnlyAttr = + attr("testonly", BOOLEAN) + .value(true) + .nonconfigurable("policy decision: this shouldn't depend on the configuration"); if (attributes.containsKey("testonly")) { override(testOnlyAttr); } else { @@ -880,8 +899,8 @@ public Builder(String name, RuleClassType type, boolean starlark, RuleClass... p } /** - * Checks that required attributes for test rules are present, creates the - * {@link RuleClass} object and returns it. + * Checks that required attributes for test rules are present, creates the {@link RuleClass} + * object and returns it. * * @throws IllegalStateException if any of the required attributes is missing */ @@ -984,6 +1003,7 @@ public RuleClass build(String name, String key) { optionReferenceFunction, ruleDefinitionEnvironmentLabel, ruleDefinitionEnvironmentDigest, + ruleDefinitionEnvironmentRepoMappingEntries, configurationFragmentPolicy.build(), supportsConstraintChecking, toolchainTypes, @@ -1144,8 +1164,10 @@ public Builder setWorkspaceOnly() { */ @CanIgnoreReturnValue public Builder setOutputToGenfiles() { - Preconditions.checkState(type != RuleClassType.ABSTRACT, - "Setting not inherited property (output to genrules) of abstract rule class '%s'", name); + Preconditions.checkState( + type != RuleClassType.ABSTRACT, + "Setting not inherited property (output to genrules) of abstract rule class '%s'", + name); this.outputsToBindir = false; return this; } @@ -1161,22 +1183,22 @@ public Builder setOutputToGenfiles() { */ @CanIgnoreReturnValue public Builder setImplicitOutputsFunction(ImplicitOutputsFunction implicitOutputsFunction) { - Preconditions.checkState(type != RuleClassType.ABSTRACT, + Preconditions.checkState( + type != RuleClassType.ABSTRACT, "Setting not inherited property (implicit output function) of abstract rule class '%s'", name); this.implicitOutputsFunction = implicitOutputsFunction; return this; } - /** - * Applies the given transition factory to all incoming edges for this rule class. - */ + /** Applies the given transition factory to all incoming edges for this rule class. */ @CanIgnoreReturnValue public Builder cfg(TransitionFactory transitionFactory) { - Preconditions.checkState(type != RuleClassType.ABSTRACT, - "Setting not inherited property (cfg) of abstract rule class '%s'", name); - Preconditions.checkState(this.transitionFactory == null, - "Property cfg has already been set"); + Preconditions.checkState( + type != RuleClassType.ABSTRACT, + "Setting not inherited property (cfg) of abstract rule class '%s'", + name); + Preconditions.checkState(this.transitionFactory == null, "Property cfg has already been set"); Preconditions.checkNotNull(transitionFactory); Preconditions.checkArgument(!transitionFactory.isSplit()); this.transitionFactory = transitionFactory; @@ -1258,13 +1280,18 @@ public Builder addAttribute(Attribute attribute) { private void overrideAttribute(Attribute attribute) { String attrName = attribute.getName(); - Preconditions.checkState(attributes.containsKey(attrName), - "No such attribute '%s' to override in ruleclass '%s'.", attrName, name); + Preconditions.checkState( + attributes.containsKey(attrName), + "No such attribute '%s' to override in ruleclass '%s'.", + attrName, + name); Type origType = attributes.get(attrName).getType(); Type newType = attribute.getType(); - Preconditions.checkState(origType.equals(newType), + Preconditions.checkState( + origType.equals(newType), "The type of the new attribute '%s' is different from the original one '%s'.", - newType, origType); + newType, + origType); attributes.put(attrName, attribute); } @@ -1410,6 +1437,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. * @@ -1417,8 +1451,8 @@ public Label getRuleDefinitionEnvironmentLabel() { */ @CanIgnoreReturnValue public Builder removeAttribute(String name) { - Preconditions.checkState(attributes.containsKey(name), "No such attribute '%s' to remove.", - name); + Preconditions.checkState( + attributes.containsKey(name), "No such attribute '%s' to remove.", name); attributes.remove(name); return this; } @@ -1510,11 +1544,9 @@ public Builder compatibleWith(Label... environments) { */ @CanIgnoreReturnValue public Builder restrictedTo(Label firstEnvironment, Label... otherEnvironments) { - ImmutableList

Returns null if the named attribute is not defined for this class of Rule. */ @@ -2013,10 +2042,7 @@ Integer getAttributeIndex(String attrName) { return attributeIndex.get(attrName); } - /** - * Returns the attribute whose index is 'attrIndex'. Fails if attrIndex is - * not in range. - */ + /** Returns the attribute whose index is 'attrIndex'. Fails if attrIndex is not in range. */ Attribute getAttribute(int attrIndex) { return attributes.get(attrIndex); } @@ -2025,8 +2051,9 @@ Attribute getAttribute(int attrIndex) { * Returns the attribute whose name is 'attrName'; fails with NullPointerException if not found. */ public Attribute getAttributeByName(String attrName) { - Integer attrIndex = Preconditions.checkNotNull(getAttributeIndex(attrName), - "Attribute %s does not exist", attrName); + Integer attrIndex = + Preconditions.checkNotNull( + getAttributeIndex(attrName), "Attribute %s does not exist", attrName); return attributes.get(attrIndex); } @@ -2075,16 +2102,12 @@ public AdvertisedProviderSet getAdvertisedProviders() { return advertisedProviders; } - /** - * Returns this rule's policy for configuration fragment access. - */ + /** Returns this rule's policy for configuration fragment access. */ public ConfigurationFragmentPolicy getConfigurationFragmentPolicy() { return configurationFragmentPolicy; } - /** - * Returns true if rules of this type can be used with the constraint enforcement system. - */ + /** Returns true if rules of this type can be used with the constraint enforcement system. */ public boolean supportsConstraintChecking() { return supportsConstraintChecking; } @@ -2310,7 +2333,7 @@ private void populateDefaultRuleAttributeValues( attrsWithComputedDefaults.add(attr); } else if (attr.isLateBound()) { - rule.setAttributeValue(attr, attr.getLateBoundDefault(), /*explicit=*/ false); + rule.setAttributeValue(attr, attr.getLateBoundDefault(), /* explicit= */ false); } else if (attr.getName().equals(APPLICABLE_LICENSES_ATTR) && attr.getType() == BuildType.LABEL_LIST) { @@ -2401,13 +2424,13 @@ private void populateDefaultRuleAttributeValues( } else { valueToSet = defaultValue; } - rule.setAttributeValue(attr, valueToSet, /*explicit=*/ false); + rule.setAttributeValue(attr, valueToSet, /* explicit= */ false); } } /** - * Collects all labels used as keys for configurable attributes and places them into - * the special implicit attribute that tracks them. + * Collects all labels used as keys for configurable attributes and places them into the special + * implicit attribute that tracks them. */ private static void populateConfigDependenciesAttribute(Rule rule) { RawAttributeMapper attributes = RawAttributeMapper.of(rule); @@ -2425,13 +2448,13 @@ private static void populateConfigDependenciesAttribute(Rule rule) { } } - rule.setAttributeValue(configDepsAttribute, ImmutableList.copyOf(configLabels), - /*explicit=*/false); + rule.setAttributeValue( + configDepsAttribute, ImmutableList.copyOf(configLabels), /* explicit= */ false); } /** - * Report an error for each label that appears more than once in a LABEL_LIST attribute - * of the given rule. + * Report an error for each label that appears more than once in a LABEL_LIST attribute of the + * given rule. * * @param rule The rule. * @param eventHandler The eventHandler to use to report the duplicated deps. @@ -2454,8 +2477,8 @@ private static void checkForDuplicateLabels(Rule rule, EventHandler eventHandler } /** - * Report an error if the rule has a timeout or size attribute that is not a - * legal value. These attributes appear on all tests. + * Report an error if the rule has a timeout or size attribute that is not a legal value. These + * attributes appear on all tests. * * @param rule the rule to check * @param eventHandler the eventHandler to use to report the duplicated deps @@ -2465,8 +2488,8 @@ private static void checkForValidSizeAndTimeoutValues(Rule rule, EventHandler ev String size = NonconfigurableAttributeMapper.of(rule).get("size", Type.STRING); if (TestSize.getTestSize(size) == null) { rule.reportError( - String.format("In rule '%s', size '%s' is not a valid size.", rule.getName(), size), - eventHandler); + String.format("In rule '%s', size '%s' is not a valid size.", rule.getName(), size), + eventHandler); } } if (rule.getRuleClassObject().hasAttr("timeout", Type.STRING)) { @@ -2494,24 +2517,21 @@ private static void checkAllowedValues( if (attribute.checkAllowedValues()) { PredicateWithMessage allowedValues = attribute.getAllowedValues(); Iterable values = - AggregatingAttributeMapper.of(rule).visitAttribute( - attribute.getName(), attribute.getType()); + AggregatingAttributeMapper.of(rule) + .visitAttribute(attribute.getName(), attribute.getType()); for (Object value : values) { if (!allowedValues.apply(value)) { rule.reportError( String.format( "%s: invalid value in '%s' attribute: %s", - rule.getLabel(), - attribute.getName(), - allowedValues.getErrorReason(value)), + rule.getLabel(), attribute.getName(), allowedValues.getErrorReason(value)), eventHandler); } } } } - private static void checkAspectAllowedValues( - Rule rule, EventHandler eventHandler) { + private static void checkAspectAllowedValues(Rule rule, EventHandler eventHandler) { if (rule.hasAspects()) { for (Attribute attrOfRule : rule.getAttributes()) { for (Aspect aspect : attrOfRule.getAspects(rule)) { @@ -2594,9 +2614,7 @@ Function> getExternalBindingsFunction() { return toolchainsToRegisterFunction; } - /** - * Returns a function that computes the options referenced by a rule. - */ + /** Returns a function that computes the options referenced by a rule. */ public Function> getOptionReferenceFunction() { return optionReferenceFunction; } @@ -2633,6 +2651,12 @@ 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 381968d6481fb8..9764394f6e2789 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/BUILD +++ b/src/main/java/com/google/devtools/build/lib/rules/BUILD @@ -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", 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 92030f4fb0a173..b318ad2bbdde7c 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 @@ -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 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..ac0e193dabf893 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 fe2796cbcdd728..4a86ab2afbc2ba 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 @@ -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(), diff --git a/src/test/shell/bazel/starlark_repository_test.sh b/src/test/shell/bazel/starlark_repository_test.sh index 8493fe92f86ca0..a6e348b76177eb 100755 --- a/src/test/shell/bazel/starlark_repository_test.sh +++ b/src/test/shell/bazel/starlark_repository_test.sh @@ -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 < 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"