Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix label_flag and label_setting to not have a dependency on the default #13372

Closed
wants to merge 2 commits into from
Closed

Fix label_flag and label_setting to not have a dependency on the default #13372

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4a44dab
Fix label_flag and label_setting to not have a dependency on the default
katre Apr 19, 2021
422e41c
Fixed option converter failure
katre Apr 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptionConverters.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

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.NODEP_LABEL_LIST;
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 +59,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
3 changes: 2 additions & 1 deletion src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
import com.google.devtools.build.lib.packages.RuleClass.Builder.ThirdPartyLicenseExistencePolicy;
import com.google.devtools.build.lib.packages.RuleFactory.AttributeValues;
import com.google.devtools.build.lib.packages.Type.ConversionException;
import com.google.devtools.build.lib.packages.Type.LabelClass;
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;
Expand Down Expand Up @@ -941,7 +942,7 @@ 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)) {
if (type.getLabelClass() == LabelClass.DEPENDENCY) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When is this true? It doesn't seem to be true at any call site and AFAIU in order to fix the associated bug, the build_setting_default attribute must not create any dependencies, and allowedFilesTypes() / allowedRuleClasses() only make sense if a dependency is created. Assert that it's not true instead? (either here or in BuildSetting)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed this from isLabelType() to == LabelClass.DEPENDENCY specifically because of the change to NODEP_LABEL: with this change, the calls to allowedFileTypes() and allowedRuleClasses() were failing because they only accept real labels, not nodep labels.

Sorry I didn't wait for your review, this is currently internal at cl/369288702.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me rephrase this: when I look at the code, RuleClass.getBuildSetting() never returns a Type where getLabelClass() would return DEPENDENCY. But then why is this branch here?

In addition, if there was any case where that did return DEPENDENCY, that would trigger the bug you are fixing here, so why not assert the opposite?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, now I see your point. This entire check should be removed, since it's not possible to trigger it.

Should I add a check in BuildSetting.create() to ensure that only NODEP_LABEL and NODEP_LABEL_CLASS are used? I will make changes in the internal version and test this.

Copy link
Contributor

@lberki lberki Apr 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a better approach is to assert buildSetting.getType().getLabelClass() != LabelClass.DEPENDENCY beacuse that says exactly what is needed: that the type shouldn't be one that creates a dependency.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But allowedFileTypes can only validly be called when the type is LabelClass.DEPENDENCY (see the definitions). Since it is now not possible to have an attribute with that type, there's no valid way to call this.

defaultAttrBuilder.allowedFileTypes(FileTypeSet.ANY_FILE);
defaultAttrBuilder.allowedRuleClasses(ANY_RULE);
}
Expand Down
7 changes: 4 additions & 3 deletions src/main/java/com/google/devtools/build/lib/rules/LabelBuildSettings.java
View file
Open in desktop
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