From c9f9eedc313fc043755dd30ae7e01478eed7c417 Mon Sep 17 00:00:00 2001 From: John Cater Date: Tue, 20 Apr 2021 08:29:44 -0700 Subject: [PATCH] Fix label_flag and label_setting to not have a dependency on the default value. This prevents an extra analysis, since the dependency should only be on the value being used. Fixes #11291. Closes #13372. PiperOrigin-RevId: 369445041 --- .../build/lib/analysis/config/CoreOptionConverters.java | 2 ++ .../google/devtools/build/lib/packages/BuildSetting.java | 5 +++++ .../com/google/devtools/build/lib/packages/RuleClass.java | 6 ------ .../devtools/build/lib/rules/LabelBuildSettings.java | 7 ++++--- .../google/devtools/build/lib/packages/RuleClassTest.java | 5 +++-- 5 files changed, 14 insertions(+), 11 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptionConverters.java b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptionConverters.java index fc76634c3cb7dd..2b2ac09eadd11e 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptionConverters.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptionConverters.java @@ -16,6 +16,7 @@ import static com.google.devtools.build.lib.packages.BuildType.LABEL; import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST; +import static com.google.devtools.build.lib.packages.BuildType.NODEP_LABEL; import static com.google.devtools.build.lib.packages.Type.BOOLEAN; import static com.google.devtools.build.lib.packages.Type.INTEGER; import static com.google.devtools.build.lib.packages.Type.STRING; @@ -57,6 +58,7 @@ public class CoreOptionConverters { .put(STRING_LIST, new CommaSeparatedOptionListConverter()) .put(LABEL, new LabelConverter()) .put(LABEL_LIST, new LabelListConverter()) + .put(NODEP_LABEL, new LabelConverter()) .build(); /** A converter from strings to Starlark int values. */ diff --git a/src/main/java/com/google/devtools/build/lib/packages/BuildSetting.java b/src/main/java/com/google/devtools/build/lib/packages/BuildSetting.java index 3d1360210a2330..c64e81f657e1d0 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/BuildSetting.java +++ b/src/main/java/com/google/devtools/build/lib/packages/BuildSetting.java @@ -14,6 +14,8 @@ package com.google.devtools.build.lib.packages; import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Preconditions; +import com.google.devtools.build.lib.packages.Type.LabelClass; import com.google.devtools.build.lib.starlarkbuildapi.StarlarkConfigApi.BuildSettingApi; import net.starlark.java.eval.Printer; @@ -37,6 +39,9 @@ public static BuildSetting create(boolean isFlag, Type type, boolean allowMul } public static BuildSetting create(boolean isFlag, Type type) { + Preconditions.checkState( + type.getLabelClass() != LabelClass.DEPENDENCY, + "Build settings should not create a dependency with their default attribute"); return new BuildSetting(isFlag, type, /* allowMultiple= */ false); } 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 88b701d4fdce79..28575e92ec2cc9 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 @@ -15,7 +15,6 @@ package com.google.devtools.build.lib.packages; import static com.google.common.collect.Streams.stream; -import static com.google.devtools.build.lib.packages.Attribute.ANY_RULE; import static com.google.devtools.build.lib.packages.Attribute.attr; import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST; import static com.google.devtools.build.lib.packages.ExecGroup.COPY_FROM_RULE_EXEC_GROUP; @@ -58,7 +57,6 @@ import com.google.devtools.build.lib.packages.Type.ConversionException; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec.VisibleForSerialization; -import com.google.devtools.build.lib.util.FileTypeSet; import com.google.devtools.build.lib.util.StringUtil; import com.google.devtools.build.lib.vfs.PathFragment; import java.util.ArrayList; @@ -941,10 +939,6 @@ public RuleClass build(String name, String key) { attr(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME, type) .nonconfigurable(BUILD_SETTING_DEFAULT_NONCONFIGURABLE) .mandatory(); - if (BuildType.isLabelType(type)) { - defaultAttrBuilder.allowedFileTypes(FileTypeSet.ANY_FILE); - defaultAttrBuilder.allowedRuleClasses(ANY_RULE); - } this.add(defaultAttrBuilder); this.add( diff --git a/src/main/java/com/google/devtools/build/lib/rules/LabelBuildSettings.java b/src/main/java/com/google/devtools/build/lib/rules/LabelBuildSettings.java index a3a67fb6d8b63b..da497bb122f2cb 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/LabelBuildSettings.java +++ b/src/main/java/com/google/devtools/build/lib/rules/LabelBuildSettings.java @@ -15,6 +15,7 @@ import static com.google.devtools.build.lib.packages.Attribute.attr; import static com.google.devtools.build.lib.packages.BuildType.LABEL; +import static com.google.devtools.build.lib.packages.BuildType.NODEP_LABEL; import static com.google.devtools.build.lib.packages.RuleClass.Builder.STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME; import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment; @@ -56,7 +57,7 @@ public class LabelBuildSettings { null, (rule, attributes, configuration) -> { if (rule == null || configuration == null) { - return attributes.get(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME, LABEL); + return attributes.get(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME, NODEP_LABEL); } Object commandLineValue = configuration.getOptions().getStarlarkOptions().get(rule.getLabel()); @@ -64,7 +65,7 @@ public class LabelBuildSettings { try { asLabel = commandLineValue == null - ? attributes.get(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME, LABEL) + ? attributes.get(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME, NODEP_LABEL) : LABEL.convert(commandLineValue, "label_flag value resolution"); } catch (ConversionException e) { throw new IllegalStateException( @@ -81,7 +82,7 @@ private static RuleClass buildRuleClass(RuleClass.Builder builder, boolean flag) .removeAttribute("licenses") .removeAttribute("distribs") .add(attr(":alias", LABEL).value(ACTUAL)) - .setBuildSetting(BuildSetting.create(flag, LABEL)) + .setBuildSetting(BuildSetting.create(flag, NODEP_LABEL)) .canHaveAnyProvider() .useToolchainResolution(ToolchainResolutionMode.DISABLED) .build(); 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 60e2a41ca7d257..6d99e3a9cd142a 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 @@ -18,6 +18,7 @@ import static com.google.devtools.build.lib.packages.Attribute.attr; import static com.google.devtools.build.lib.packages.BuildType.LABEL; import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST; +import static com.google.devtools.build.lib.packages.BuildType.NODEP_LABEL; import static com.google.devtools.build.lib.packages.BuildType.OUTPUT_LIST; import static com.google.devtools.build.lib.packages.ImplicitOutputsFunction.substitutePlaceholderIntoTemplate; import static com.google.devtools.build.lib.packages.RuleClass.Builder.STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME; @@ -1097,7 +1098,7 @@ public void testBuildSetting_createsDefaultAttribute() { new RuleClass.Builder("label_flag", RuleClassType.NORMAL, false) .factory(DUMMY_CONFIGURED_TARGET_FACTORY) .add(attr("tags", STRING_LIST)) - .setBuildSetting(BuildSetting.create(true, LABEL)) + .setBuildSetting(BuildSetting.create(true, NODEP_LABEL)) .build(); RuleClass stringSetting = new RuleClass.Builder("string_setting", RuleClassType.NORMAL, false) @@ -1106,7 +1107,7 @@ public void testBuildSetting_createsDefaultAttribute() { .setBuildSetting(BuildSetting.create(false, STRING)) .build(); - assertThat(labelFlag.hasAttr(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME, LABEL)).isTrue(); + assertThat(labelFlag.hasAttr(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME, NODEP_LABEL)).isTrue(); assertThat(stringSetting.hasAttr(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME, STRING)).isTrue(); }