Skip to content

Commit

Permalink
Add a mechanism to prohibit select()-ing on some native flags
Browse files Browse the repository at this point in the history
The options fragment gets to decide whether its flags are selectable. This way, selectability can be guarded by an experimental or incompatible change flag, provided that it's declared in the same options fragment as the flag it's guarding.

Work toward #6583 and #6501.

RELNOTES: None
PiperOrigin-RevId: 227040456
  • Loading branch information
brandjon authored and Copybara-Service committed Dec 27, 2018
1 parent f4ddd6b commit 328b7bb
Show file tree
Hide file tree
Showing 12 changed files with 457 additions and 101 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1262,7 +1262,7 @@ public BuildConfiguration(
this.testEnv = setupTestEnvironment();

this.transitiveOptionDetails =
TransitiveOptionDetails.forOptionsWithDefaults(buildOptions.getNativeOptions());
TransitiveOptionDetails.forOptions(buildOptions.getNativeOptions());

ImmutableMap.Builder<String, String> globalMakeEnvBuilder = ImmutableMap.builder();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,13 @@

import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.common.options.OptionDefinition;
import com.google.devtools.common.options.Options;
import com.google.devtools.common.options.OptionsBase;
import java.io.Serializable;
import java.util.Map;
import java.util.Set;
import javax.annotation.Nullable;

/** Command-line build options for a Blaze module. */
public abstract class FragmentOptions extends OptionsBase implements Cloneable, Serializable {
Expand Down Expand Up @@ -59,4 +61,63 @@ public FragmentOptions getDefault() {
public FragmentOptions getHost() {
return getDefault();
}

/** Tracks limitations on referring to an option in a {@code config_setting}. */
// TODO(bazel-team): There will likely also be a need to customize whether or not an option is
// visible to users for setting on the command line (or perhaps even in a test of a Starlark
// rule). This class may be a good place to add this functionality.
public static final class SelectRestriction {

private final boolean visibleWithinToolsPackage;

@Nullable private final String errorMessage;

public SelectRestriction(boolean visibleWithinToolsPackage, @Nullable String errorMessage) {
this.visibleWithinToolsPackage = visibleWithinToolsPackage;
this.errorMessage = errorMessage;
}

/**
* Whether the option can still be seen by {@code config_setting}s that are defined by packages
* underneath the tools repository's "tools" package, e.g. {@code @bazel_tools//tools/...}.
*/
public boolean isVisibleWithinToolsPackage() {
return visibleWithinToolsPackage;
}

/**
* An additional explanation to append to the generic error message when a user attempts to use
* this option. Should explain why this option is unavailable.
*
* <p>If null, no content will be appended to the generic error message.
*/
@Nullable
public String getErrorMessage() {
return errorMessage;
}
}

/**
* Returns a map from options defined by this fragment to restrictions on whether the option may
* appear in a {@code config_setting}. If an option defined by this fragment is not a key of this
* map, then it has no restriction.
*
* <p>In addition to making options unconditionally non-selectable, this can also be used to gate
* selectability based on the value of other flags in the same fragment -- for instance,
* experimental or incompatible change flags.
*
* <p>The intended usage pattern is to define, for each flag {@code foo} to have a restriction, a
* field
*
* <pre>{@code
* private static final OptionDefinition FOO_DEFINITION =
* OptionsParser.getOptionDefinitionByName(ThisClass.class, "foo");
* }</pre>
*
* This way, if the option is ever renamed (especially common for an experimental flag), if the
* definition is not updated at the same time it will fail-fast during static initialization.
*/
public Map<OptionDefinition, SelectRestriction> getSelectRestrictions() {
return ImmutableMap.of();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,47 +16,59 @@

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.analysis.config.FragmentOptions.SelectRestriction;
import com.google.devtools.common.options.OptionDefinition;
import com.google.devtools.common.options.OptionMetadataTag;
import com.google.devtools.common.options.OptionsBase;
import com.google.devtools.common.options.OptionsParser;
import java.io.Serializable;
import java.util.Map;
import javax.annotation.Nullable;

/**
* Introspector for option details - what OptionsBase class the option is defined in, the option's
* current value, and whether the option allows multiple values to be specified.
* Introspector for option details, to be used when {@code select()}-ing over the options.
*
* <p>This tracks:
*
* <ul>
* <li>what {@link FragmentOptions} class the option is defined in
* <li>the option's current value
* <li>whether it allows multiple values to be specified ({@link Option#allowMultiple}
* <li>whether it is selectable, i.e., allowed to appear in a {@code config_setting}
* </ul>
*
* <p>This is "transitive" in that it includes *all* options recognizable by a given configuration,
* including those defined in child fragments.
*/
public final class TransitiveOptionDetails implements Serializable {

/**
* Computes and returns the transitive optionName -> "option info" map for the given set of
* options sets, using the given map as defaults for options which would otherwise be null.
*/
static TransitiveOptionDetails forOptionsWithDefaults(
Iterable<? extends OptionsBase> buildOptions) {
/** Builds a {@code TransitiveOptionDetails} for the given set of options. */
static TransitiveOptionDetails forOptions(Iterable<? extends FragmentOptions> buildOptions) {
ImmutableMap.Builder<String, OptionDetails> map = ImmutableMap.builder();
try {
for (OptionsBase options : buildOptions) {
for (FragmentOptions options : buildOptions) {
ImmutableList<OptionDefinition> optionDefinitions =
OptionsParser.getOptionDefinitions(options.getClass());
Map<OptionDefinition, SelectRestriction> selectRestrictions =
options.getSelectRestrictions();

for (OptionDefinition optionDefinition : optionDefinitions) {
if (ImmutableList.copyOf(optionDefinition.getOptionMetadataTags())
.contains(OptionMetadataTag.INTERNAL)) {
// ignore internal options
continue;
}
// ignore internal options
continue;
}
Object value = optionDefinition.getField().get(options);
if (value == null && !optionDefinition.isSpecialNullDefault()) {
// See {@link Option#defaultValue} for an explanation of default "null" strings.
value = optionDefinition.getUnparsedDefaultValue();
}
map.put(
optionDefinition.getOptionName(),
new OptionDetails(options.getClass(), value, optionDefinition.allowsMultiple()));
new OptionDetails(
options.getClass(),
value,
optionDefinition.allowsMultiple(),
selectRestrictions.get(optionDefinition)));
}
}
} catch (IllegalAccessException e) {
Expand All @@ -67,21 +79,32 @@ static TransitiveOptionDetails forOptionsWithDefaults(
}

private static final class OptionDetails implements Serializable {
private OptionDetails(Class<? extends OptionsBase> optionsClass, Object value,
boolean allowsMultiple) {

private OptionDetails(
Class<? extends FragmentOptions> optionsClass,
Object value,
boolean allowsMultiple,
@Nullable SelectRestriction selectRestriction) {
this.optionsClass = optionsClass;
this.value = value;
this.allowsMultiple = allowsMultiple;
this.selectRestriction = selectRestriction;
}

/** The {@link FragmentOptions} class that defines this option. */
private final Class<? extends OptionsBase> optionsClass;
private final Class<? extends FragmentOptions> optionsClass;

/** The value of the given option (either explicitly defined or default). May be null. */
@Nullable private final Object value;

/** Whether or not this option supports multiple values. */
private final boolean allowsMultiple;

/**
* Information on whether this option is permitted to appear in {@code config_setting}s. Null if
* there is no such restriction.
*/
@Nullable private final SelectRestriction selectRestriction;
}

/**
Expand All @@ -101,13 +124,14 @@ private TransitiveOptionDetails(ImmutableMap<String, OptionDetails> transitiveOp
}

/**
* Returns the {@link OptionsBase} class the defines the given option, null if the option isn't
* recognized.
* Returns the {@link FragmentOptions} class the defines the given option, null if the option
* isn't recognized.
*
* <p>optionName is the name of the option as it appears on the command line e.g. {@link
* OptionDefinition#getOptionName()}).
*/
public Class<? extends OptionsBase> getOptionClass(String optionName) {
@Nullable
public Class<? extends FragmentOptions> getOptionClass(String optionName) {
OptionDetails optionDetails = transitiveOptionsMap.get(optionName);
return optionDetails == null ? null : optionDetails.optionsClass;
}
Expand All @@ -120,6 +144,7 @@ public Class<? extends OptionsBase> getOptionClass(String optionName) {
* <p>optionName is the name of the option as it appears on the command line e.g. {@link
* OptionDefinition#getOptionName()}).
*/
@Nullable
public Object getOptionValue(String optionName) {
OptionDetails optionDetails = transitiveOptionsMap.get(optionName);
return (optionDetails == null) ? null : optionDetails.value;
Expand All @@ -137,4 +162,15 @@ public boolean allowsMultipleValues(String optionName) {
OptionDetails optionDetails = transitiveOptionsMap.get(optionName);
return optionDetails != null && optionDetails.allowsMultiple;
}

/**
* Returns information about whether an option may appear in a {@code config_setting}.
*
* <p>Returns null for unrecognized options or options that have no restriction.
*/
@Nullable
public SelectRestriction getSelectRestriction(String optionName) {
OptionDetails optionDetails = transitiveOptionsMap.get(optionName);
return optionDetails == null ? null : optionDetails.selectRestriction;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -452,12 +452,7 @@ public String getUnambiguousCanonicalForm() {
* conflated.
*/
public String getDefaultCanonicalForm() {
String repository;
if (packageIdentifier.getRepository().isMain()) {
repository = "";
} else {
repository = packageIdentifier.getRepository().getName();
}
String repository = packageIdentifier.getRepository().getDefaultCanonicalForm();
return repository + "//" + packageIdentifier.getPackageFragment() + ":" + name;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,14 @@ public String getName() {
return name;
}

/**
* Returns the repository name, except that the main repo is conflated with the default repo
* ({@code "@"} becomes the empty string).
*/
public String getDefaultCanonicalForm() {
return isMain() ? "" : getName();
}

/**
* Returns the relative path to the repository source. Returns "" for the main repository and
* external/[repository name] for external repositories.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization",
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec",
"//src/main/java/com/google/devtools/build/lib/skylarkbuildapi/config",
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
"//src/main/java/com/google/devtools/common/options",
"//third_party:auto_value",
"//third_party:guava",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@
import com.google.devtools.build.lib.analysis.RuleContext;
import com.google.devtools.build.lib.analysis.RuleDefinition;
import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment;
import com.google.devtools.build.lib.packages.Attribute.ComputedDefault;
import com.google.devtools.build.lib.packages.AttributeMap;
import com.google.devtools.build.lib.packages.NonconfigurableAttributeMapper;
import com.google.devtools.build.lib.packages.RuleClass;
import com.google.devtools.build.lib.syntax.Type;
Expand Down Expand Up @@ -131,10 +133,22 @@ public static final class ConfigSettingRule implements RuleDefinition {
/** The name of the attribute that declares constraint_values. */
public static final String CONSTRAINT_VALUES_ATTRIBUTE = "constraint_values";

/** The name of the tools repository. */
public static final String TOOLS_REPOSITORY_ATTRIBUTE = "$tools_repository";

@Override
public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env) {
return builder
.requiresConfigurationFragments(PlatformConfiguration.class)
.add(
attr(TOOLS_REPOSITORY_ATTRIBUTE, STRING)
.value(
new ComputedDefault() {
@Override
public Object getDefault(AttributeMap rule) {
return env.getToolsRepository();
}
}))

/* <!-- #BLAZE_RULE(config_setting).ATTRIBUTE(values) -->
The set of configuration values that match this rule (expressed as Bazel flags)
Expand Down
Loading

0 comments on commit 328b7bb

Please sign in to comment.