Skip to content

Commit ee738da

Browse files
katrekatre
committed
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
1 parent 6080c1e commit ee738da

5 files changed

Lines changed: 14 additions & 11 deletions

File tree

‎src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptionConverters.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
import static com.google.devtools.build.lib.packages.BuildType.LABEL;
1818
import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST;
19+
import static com.google.devtools.build.lib.packages.BuildType.NODEP_LABEL;
1920
import static com.google.devtools.build.lib.packages.Type.BOOLEAN;
2021
import static com.google.devtools.build.lib.packages.Type.INTEGER;
2122
import static com.google.devtools.build.lib.packages.Type.STRING;
@@ -57,6 +58,7 @@ public class CoreOptionConverters {
5758
.put(STRING_LIST, new CommaSeparatedOptionListConverter())
5859
.put(LABEL, new LabelConverter())
5960
.put(LABEL_LIST, new LabelListConverter())
61+
.put(NODEP_LABEL, new LabelConverter())
6062
.build();
6163

6264
/** A converter from strings to Starlark int values. */

‎src/main/java/com/google/devtools/build/lib/packages/BuildSetting.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414
package com.google.devtools.build.lib.packages;
1515

1616
import com.google.common.annotations.VisibleForTesting;
17+
import com.google.common.base.Preconditions;
18+
import com.google.devtools.build.lib.packages.Type.LabelClass;
1719
import com.google.devtools.build.lib.starlarkbuildapi.StarlarkConfigApi.BuildSettingApi;
1820
import net.starlark.java.eval.Printer;
1921

@@ -28,6 +30,9 @@ public class BuildSetting implements BuildSettingApi {
2830
public BuildSetting(boolean isFlag, Type<?> type) {
2931
this.isFlag = isFlag;
3032
this.type = type;
33+
Preconditions.checkState(
34+
type.getLabelClass() != LabelClass.DEPENDENCY,
35+
"Build settings should not create a dependency with their default attribute");
3136
}
3237

3338
public Type<?> getType() {

‎src/main/java/com/google/devtools/build/lib/packages/RuleClass.java‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@
1414

1515
package com.google.devtools.build.lib.packages;
1616

17-
import static com.google.devtools.build.lib.packages.Attribute.ANY_RULE;
1817
import static com.google.devtools.build.lib.packages.Attribute.attr;
1918
import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST;
2019
import static com.google.devtools.build.lib.packages.ExecGroup.COPY_FROM_RULE_EXEC_GROUP;
@@ -56,7 +55,6 @@
5655
import com.google.devtools.build.lib.packages.Type.ConversionException;
5756
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
5857
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec.VisibleForSerialization;
59-
import com.google.devtools.build.lib.util.FileTypeSet;
6058
import com.google.devtools.build.lib.util.StringUtil;
6159
import com.google.devtools.build.lib.vfs.PathFragment;
6260
import java.util.ArrayList;
@@ -858,10 +856,6 @@ public RuleClass build(String name, String key) {
858856
attr(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME, type)
859857
.nonconfigurable(BUILD_SETTING_DEFAULT_NONCONFIGURABLE)
860858
.mandatory();
861-
if (BuildType.isLabelType(type)) {
862-
attrBuilder.allowedFileTypes(FileTypeSet.ANY_FILE);
863-
attrBuilder.allowedRuleClasses(ANY_RULE);
864-
}
865859
this.add(attrBuilder);
866860

867861
// Build setting rules should opt out of toolchain resolution, since they form part of the

‎src/main/java/com/google/devtools/build/lib/rules/LabelBuildSettings.java‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515

1616
import static com.google.devtools.build.lib.packages.Attribute.attr;
1717
import static com.google.devtools.build.lib.packages.BuildType.LABEL;
18+
import static com.google.devtools.build.lib.packages.BuildType.NODEP_LABEL;
1819
import static com.google.devtools.build.lib.packages.RuleClass.Builder.STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME;
1920

2021
import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment;
@@ -55,15 +56,15 @@ public class LabelBuildSettings {
5556
null,
5657
(rule, attributes, configuration) -> {
5758
if (rule == null || configuration == null) {
58-
return attributes.get(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME, LABEL);
59+
return attributes.get(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME, NODEP_LABEL);
5960
}
6061
Object commandLineValue =
6162
configuration.getOptions().getStarlarkOptions().get(rule.getLabel());
6263
Label asLabel;
6364
try {
6465
asLabel =
6566
commandLineValue == null
66-
? attributes.get(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME, LABEL)
67+
? attributes.get(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME, NODEP_LABEL)
6768
: LABEL.convert(commandLineValue, "label_flag value resolution");
6869
} catch (ConversionException e) {
6970
throw new IllegalStateException(
@@ -80,7 +81,7 @@ private static RuleClass buildRuleClass(RuleClass.Builder builder, boolean flag)
8081
.removeAttribute("licenses")
8182
.removeAttribute("distribs")
8283
.add(attr(":alias", LABEL).value(ACTUAL))
83-
.setBuildSetting(new BuildSetting(flag, LABEL))
84+
.setBuildSetting(new BuildSetting(flag, NODEP_LABEL))
8485
.canHaveAnyProvider()
8586
.useToolchainResolution(false)
8687
.build();

‎src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import static com.google.devtools.build.lib.packages.Attribute.attr;
1919
import static com.google.devtools.build.lib.packages.BuildType.LABEL;
2020
import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST;
21+
import static com.google.devtools.build.lib.packages.BuildType.NODEP_LABEL;
2122
import static com.google.devtools.build.lib.packages.BuildType.OUTPUT_LIST;
2223
import static com.google.devtools.build.lib.packages.ImplicitOutputsFunction.substitutePlaceholderIntoTemplate;
2324
import static com.google.devtools.build.lib.packages.RuleClass.Builder.STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME;
@@ -1127,7 +1128,7 @@ public void testBuildSetting_createsDefaultAttribute() {
11271128
new RuleClass.Builder("label_flag", RuleClassType.NORMAL, false)
11281129
.factory(DUMMY_CONFIGURED_TARGET_FACTORY)
11291130
.add(attr("tags", STRING_LIST))
1130-
.setBuildSetting(new BuildSetting(true, LABEL))
1131+
.setBuildSetting(new BuildSetting(true, NODEP_LABEL))
11311132
.build();
11321133
RuleClass stringSetting =
11331134
new RuleClass.Builder("string_setting", RuleClassType.NORMAL, false)
@@ -1136,7 +1137,7 @@ public void testBuildSetting_createsDefaultAttribute() {
11361137
.setBuildSetting(new BuildSetting(false, STRING))
11371138
.build();
11381139

1139-
assertThat(labelFlag.hasAttr(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME, LABEL)).isTrue();
1140+
assertThat(labelFlag.hasAttr(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME, NODEP_LABEL)).isTrue();
11401141
assertThat(stringSetting.hasAttr(STARLARK_BUILD_SETTING_DEFAULT_ATTR_NAME, STRING)).isTrue();
11411142
}
11421143

0 commit comments

Comments
 (0)