Skip to content

Commit

Permalink
Fix label_flag and label_setting to not have a dependency on the default
Browse files Browse the repository at this point in the history 
value.

This prevents an extra analysis, since the dependency should only be on
the value being used.

Fixes bazelbuild#11291.

Closes bazelbuild#13372.

PiperOrigin-RevId: 369445041
  • Loading branch information
@katre
katre committed Jul 13, 2021
1 parent 6080c1e commit ee738da
Show file tree
Hide file tree
Showing 5 changed files with 14 additions and 11 deletions.
2 changes: 2 additions & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptionConverters.java
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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. */
Expand Down
5 changes: 5 additions & 0 deletions src/main/java/com/google/devtools/build/lib/packages/BuildSetting.java
View file
Expand Up @@ -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;

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

public Type<?> getType() {
Expand Down
6 changes: 0 additions & 6 deletions src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
View file
Expand Up @@ -14,7 +14,6 @@

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

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;
Expand Down Expand Up @@ -56,7 +55,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;
Expand Down Expand Up @@ -858,10 +856,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)) {
attrBuilder.allowedFileTypes(FileTypeSet.ANY_FILE);
attrBuilder.allowedRuleClasses(ANY_RULE);
}
this.add(attrBuilder);

// Build setting rules should opt out of toolchain resolution, since they form part of the
Expand Down
7 changes: 4 additions & 3 deletions src/main/java/com/google/devtools/build/lib/rules/LabelBuildSettings.java
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -55,15 +56,15 @@ 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());
Label asLabel;
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(
Expand All @@ -80,7 +81,7 @@ private static RuleClass buildRuleClass(RuleClass.Builder builder, boolean flag)
.removeAttribute("licenses")
.removeAttribute("distribs")
.add(attr(":alias", LABEL).value(ACTUAL))
.setBuildSetting(new BuildSetting(flag, LABEL))
.setBuildSetting(new BuildSetting(flag, NODEP_LABEL))
.canHaveAnyProvider()
.useToolchainResolution(false)
.build();
Expand Down
5 changes: 3 additions & 2 deletions src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -1127,7 +1128,7 @@ public void testBuildSetting_createsDefaultAttribute() {
new RuleClass.Builder("label_flag", RuleClassType.NORMAL, false)
.factory(DUMMY_CONFIGURED_TARGET_FACTORY)
.add(attr("tags", STRING_LIST))
.setBuildSetting(new BuildSetting(true, LABEL))
.setBuildSetting(new BuildSetting(true, NODEP_LABEL))
.build();
RuleClass stringSetting =
new RuleClass.Builder("string_setting", RuleClassType.NORMAL, false)
Expand All @@ -1136,7 +1137,7 @@ public void testBuildSetting_createsDefaultAttribute() {
.setBuildSetting(new BuildSetting(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();
}

Expand Down

0 comments on commit ee738da

Please sign in to comment.