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 @copybara-github
katre authored and copybara-github committed Apr 20, 2021
1 parent 660e8a5 commit c9f9eed
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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 @@ -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);
}

Expand Down
6 changes: 0 additions & 6 deletions src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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(
Expand Down
7 changes: 4 additions & 3 deletions src/main/java/com/google/devtools/build/lib/rules/LabelBuildSettings.java
View file
Original file line number Diff line number Diff line change
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 @@ -56,15 +57,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 @@ -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();
Expand Down
5 changes: 3 additions & 2 deletions src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java
View file
Original file line number Diff line number Diff line change
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 @@ -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)
Expand All @@ -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();
}

Expand Down

0 comments on commit c9f9eed

Please sign in to comment.