Skip to content

Commit

Permalink
bazel C++ rules: avoid deprecated EvalException(Location) constructor
Browse files Browse the repository at this point in the history 
Instead, make the auxiliary location a prefix of the error message.

Also, delete dead ArtifactCategory.fromString method.

PiperOrigin-RevId: 350775277
  • Loading branch information
@adonovan @copybara-github
adonovan authored and copybara-github committed Jan 8, 2021
1 parent 63e0392 commit 323cbc7
Show file tree
Hide file tree
Showing 3 changed files with 66 additions and 109 deletions.
27 changes: 0 additions & 27 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/ArtifactCategory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,7 @@
// limitations under the License
package com.google.devtools.build.lib.rules.cpp;

import com.google.common.base.Joiner;
import com.google.common.collect.ImmutableList;
import java.util.Arrays;
import net.starlark.java.eval.EvalException;
import net.starlark.java.syntax.Location;

/**
* A category of artifacts that are candidate input/output to an action, for which the toolchain can
Expand All @@ -44,10 +40,6 @@ public enum ArtifactCategory {
// the options passed to the clif_matcher.
CLIF_OUTPUT_PROTO("", ".opb");

private static final ArtifactCategory[] ALLOWED_FROM_STARLARK = {
STATIC_LIBRARY, ALWAYSLINK_STATIC_LIBRARY, DYNAMIC_LIBRARY, INTERFACE_LIBRARY
};

private final String defaultPrefix;
private final String defaultExtension;
private final String starlarkName;
Expand All @@ -74,25 +66,6 @@ public String getStarlarkName() {
return starlarkName;
}

public static ArtifactCategory fromString(
String starlarkName, Location location, String fieldForError) throws EvalException {
for (ArtifactCategory registerActions : ALLOWED_FROM_STARLARK) {
if (registerActions.getStarlarkName().equals(starlarkName)) {
return registerActions;
}
}
throw new EvalException(
location,
String.format(
"Possible values for %s: %s",
fieldForError,
Joiner.on(", ")
.join(
Arrays.stream(ALLOWED_FROM_STARLARK)
.map(ArtifactCategory::getStarlarkName)
.collect(ImmutableList.toImmutableList()))));
}

/** Returns the name of the category. */
public String getCategoryName() {
return this.toString().toLowerCase();
Expand Down
140 changes: 61 additions & 79 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import com.google.devtools.build.lib.collect.nestedset.Order;
import com.google.devtools.build.lib.packages.BazelModuleContext;
import com.google.devtools.build.lib.packages.BazelStarlarkContext;
import com.google.devtools.build.lib.packages.Info;
import com.google.devtools.build.lib.packages.Provider;
import com.google.devtools.build.lib.packages.RuleClass.ConfiguredTargetFactory.RuleErrorException;
import com.google.devtools.build.lib.packages.StarlarkInfo;
Expand Down Expand Up @@ -70,6 +71,7 @@
import com.google.devtools.build.lib.util.StringUtil;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.CToolchain;
import com.google.errorprone.annotations.FormatMethod;
import java.util.Arrays;
import java.util.HashSet;
import java.util.List;
Expand Down Expand Up @@ -1181,6 +1183,15 @@ private static void checkRightStarlarkInfoProvider(
}
}

@FormatMethod
private static EvalException infoError(Info info, String format, Object... args) {
return Starlark.errorf(
"in %s instantiated at %s: %s",
info.getProvider().getPrintableName(),
info.getCreationLocation(),
String.format(format, args));
}

/** Checks whether the {@link StarlarkInfo} is of the required type. */
private static void checkRightProviderType(StarlarkInfo provider, String type)
throws EvalException {
Expand All @@ -1189,9 +1200,7 @@ private static void checkRightProviderType(StarlarkInfo provider, String type)
providerType = provider.getProvider().getPrintableName();
}
if (!type.equals(provider.getValue("type_name"))) {
throw new EvalException(
provider.getCreationLocation(),
String.format("Expected object of type '%s', received '%s'.", type, providerType));
throw infoError(provider, "Expected object of type '%s', received '%s'.", type, providerType);
}
}

Expand All @@ -1211,18 +1220,16 @@ static Feature featureFromStarlark(StarlarkInfo featureStruct) throws EvalExcept
Boolean enabled =
getMandatoryFieldFromStarlarkProvider(featureStruct, "enabled", Boolean.class);
if (name == null || (name.isEmpty() && !enabled)) {
throw new EvalException(
featureStruct.getCreationLocation(),
"A feature must either have a nonempty 'name' field or be enabled.");
throw infoError(
featureStruct, "A feature must either have a nonempty 'name' field or be enabled.");
}

if (!name.matches("^[_a-z0-9+\\-\\.]*$")) {
throw new EvalException(
featureStruct.getCreationLocation(),
String.format(
"A feature's name must consist solely of lowercase ASCII letters, digits, '.', "
+ "'_', '+', and '-', got '%s'",
name));
throw infoError(
featureStruct,
"A feature's name must consist solely of lowercase ASCII letters, digits, '.', "
+ "'_', '+', and '-', got '%s'",
name);
}

ImmutableList.Builder<FlagSet> flagSetBuilder = ImmutableList.builder();
Expand All @@ -1231,8 +1238,8 @@ static Feature featureFromStarlark(StarlarkInfo featureStruct) throws EvalExcept
for (StarlarkInfo flagSetObject : flagSets) {
FlagSet flagSet = flagSetFromStarlark(flagSetObject, /* actionName= */ null);
if (flagSet.getActions().isEmpty()) {
throw new EvalException(
flagSetObject.getCreationLocation(),
throw infoError(
flagSetObject,
"A flag_set that belongs to a feature must have nonempty 'actions' parameter.");
}
flagSetBuilder.add(flagSet);
Expand All @@ -1251,8 +1258,7 @@ static Feature featureFromStarlark(StarlarkInfo featureStruct) throws EvalExcept
getStarlarkProviderListFromStarlarkField(featureStruct, "requires");
for (StarlarkInfo featureSetStruct : requires) {
if (!"feature_set".equals(featureSetStruct.getValue("type_name"))) { // getValue() may be null
throw new EvalException(
featureStruct.getCreationLocation(), "expected object of type 'feature_set'.");
throw infoError(featureStruct, "expected object of type 'feature_set'.");
}
ImmutableSet<String> featureSet =
getStringSetFromStarlarkProviderField(featureSetStruct, "features");
Expand Down Expand Up @@ -1287,14 +1293,12 @@ static Pair<String, String> makeVariableFromStarlark(StarlarkInfo makeVariableSt
String name = getMandatoryFieldFromStarlarkProvider(makeVariableStruct, "name", String.class);
String value = getMandatoryFieldFromStarlarkProvider(makeVariableStruct, "value", String.class);
if (name == null || name.isEmpty()) {
throw new EvalException(
makeVariableStruct.getCreationLocation(),
"'name' parameter of make_variable must be a nonempty string.");
throw infoError(
makeVariableStruct, "'name' parameter of make_variable must be a nonempty string.");
}
if (value == null || value.isEmpty()) {
throw new EvalException(
makeVariableStruct.getCreationLocation(),
"'value' parameter of make_variable must be a nonempty string.");
throw infoError(
makeVariableStruct, "'value' parameter of make_variable must be a nonempty string.");
}
return Pair.of(name, value);
}
Expand All @@ -1311,14 +1315,10 @@ static Pair<String, String> toolPathFromStarlark(StarlarkInfo toolPathStruct)
String name = getMandatoryFieldFromStarlarkProvider(toolPathStruct, "name", String.class);
String path = getMandatoryFieldFromStarlarkProvider(toolPathStruct, "path", String.class);
if (name == null || name.isEmpty()) {
throw new EvalException(
toolPathStruct.getCreationLocation(),
"'name' parameter of tool_path must be a nonempty string.");
throw infoError(toolPathStruct, "'name' parameter of tool_path must be a nonempty string.");
}
if (path == null || path.isEmpty()) {
throw new EvalException(
toolPathStruct.getCreationLocation(),
"'path' parameter of tool_path must be a nonempty string.");
throw infoError(toolPathStruct, "'path' parameter of tool_path must be a nonempty string.");
}
return Pair.of(name, path);
}
Expand All @@ -1333,13 +1333,13 @@ static VariableWithValue variableWithValueFromStarlark(StarlarkInfo variableWith
String value =
getMandatoryFieldFromStarlarkProvider(variableWithValueStruct, "value", String.class);
if (name == null || name.isEmpty()) {
throw new EvalException(
variableWithValueStruct.getCreationLocation(),
throw infoError(
variableWithValueStruct,
"'name' parameter of variable_with_value must be a nonempty string.");
}
if (value == null || value.isEmpty()) {
throw new EvalException(
variableWithValueStruct.getCreationLocation(),
throw infoError(
variableWithValueStruct,
"'value' parameter of variable_with_value must be a nonempty string.");
}
return new VariableWithValue(name, value);
Expand All @@ -1352,14 +1352,10 @@ static EnvEntry envEntryFromStarlark(StarlarkInfo envEntryStruct) throws EvalExc
String key = getMandatoryFieldFromStarlarkProvider(envEntryStruct, "key", String.class);
String value = getMandatoryFieldFromStarlarkProvider(envEntryStruct, "value", String.class);
if (key == null || key.isEmpty()) {
throw new EvalException(
envEntryStruct.getCreationLocation(),
"'key' parameter of env_entry must be a nonempty string.");
throw infoError(envEntryStruct, "'key' parameter of env_entry must be a nonempty string.");
}
if (value == null || value.isEmpty()) {
throw new EvalException(
envEntryStruct.getCreationLocation(),
"'value' parameter of env_entry must be a nonempty string.");
throw infoError(envEntryStruct, "'value' parameter of env_entry must be a nonempty string.");
}
StringValueParser parser = new StringValueParser(value);
return new EnvEntry(key, parser.getChunks());
Expand All @@ -1383,9 +1379,7 @@ static EnvSet envSetFromStarlark(StarlarkInfo envSetStruct) throws EvalException
checkRightProviderType(envSetStruct, "env_set");
ImmutableSet<String> actions = getStringSetFromStarlarkProviderField(envSetStruct, "actions");
if (actions.isEmpty()) {
throw new EvalException(
envSetStruct.getCreationLocation(),
"actions parameter of env_set must be a nonempty list.");
throw infoError(envSetStruct, "actions parameter of env_set must be a nonempty list.");
}
ImmutableList.Builder<EnvEntry> envEntryBuilder = ImmutableList.builder();
ImmutableList<StarlarkInfo> envEntryStructs =
Expand Down Expand Up @@ -1422,14 +1416,13 @@ static FlagGroup flagGroupFromStarlark(StarlarkInfo flagGroupStruct) throws Eval
}

if (flagGroups.size() > 0 && flags.size() > 0) {
throw new EvalException(
flagGroupStruct.getCreationLocation(),
throw infoError(
flagGroupStruct,
"flag_group must contain either a list of flags or a list of flag_groups.");
}

if (flagGroups.size() == 0 && flags.size() == 0) {
throw new EvalException(
flagGroupStruct.getCreationLocation(), "Both 'flags' and 'flag_groups' are empty.");
throw infoError(flagGroupStruct, "Both 'flags' and 'flag_groups' are empty.");
}

String iterateOver =
Expand Down Expand Up @@ -1469,7 +1462,7 @@ static FlagSet flagSetFromStarlark(StarlarkInfo flagSetStruct, String actionName
// action to its flag_set.action_names
if (actionName != null) {
if (!actions.isEmpty()) {
throw new EvalException(String.format(ActionConfig.FLAG_SET_WITH_ACTION_ERROR, actionName));
throw Starlark.errorf(ActionConfig.FLAG_SET_WITH_ACTION_ERROR, actionName);
}
actions = ImmutableSet.of(actionName);
}
Expand Down Expand Up @@ -1507,16 +1500,12 @@ static CcToolchainFeatures.Tool toolFromStarlark(StarlarkInfo toolStruct) throws
CToolchain.Tool.PathOrigin toolPathOrigin;
if (toolPathString != null) {
if (toolArtifact != null) {
throw new EvalException(
toolStruct.getCreationLocation(),
"\"tool\" and \"path\" cannot be set at the same time.");
throw infoError(toolStruct, "\"tool\" and \"path\" cannot be set at the same time.");
}

toolPath = PathFragment.create(toolPathString);
if (toolPath.isEmpty()) {
throw new EvalException(
toolStruct.getCreationLocation(),
"The 'path' field of tool must be a nonempty string.");
throw infoError(toolStruct, "The 'path' field of tool must be a nonempty string.");
}

if (toolPath.isAbsolute()) {
Expand Down Expand Up @@ -1553,17 +1542,16 @@ static ActionConfig actionConfigFromStarlark(StarlarkInfo actionConfigStruct)
String actionName =
getMandatoryFieldFromStarlarkProvider(actionConfigStruct, "action_name", String.class);
if (actionName == null || actionName.isEmpty()) {
throw new EvalException(
actionConfigStruct.getCreationLocation(),
throw infoError(
actionConfigStruct,
"The 'action_name' field of action_config must be a nonempty string.");
}
if (!actionName.matches("^[_a-z0-9+\\-\\.]*$")) {
throw new EvalException(
actionConfigStruct.getCreationLocation(),
String.format(
"An action_config's name must consist solely of lowercase ASCII letters, digits, "
+ "'.', '_', '+', and '-', got '%s'",
actionName));
throw infoError(
actionConfigStruct,
"An action_config's name must consist solely of lowercase ASCII letters, digits, "
+ "'.', '_', '+', and '-', got '%s'",
actionName);
}

Boolean enabled =
Expand Down Expand Up @@ -1599,8 +1587,8 @@ static ArtifactNamePattern artifactNamePatternFromStarlark(StarlarkInfo artifact
getMandatoryFieldFromStarlarkProvider(
artifactNamePatternStruct, "category_name", String.class);
if (categoryName == null || categoryName.isEmpty()) {
throw new EvalException(
artifactNamePatternStruct.getCreationLocation(),
throw infoError(
artifactNamePatternStruct,
"The 'category_name' field of artifact_name_pattern must be a nonempty string.");
}
ArtifactCategory foundCategory = null;
Expand All @@ -1611,24 +1599,22 @@ static ArtifactNamePattern artifactNamePatternFromStarlark(StarlarkInfo artifact
}

if (foundCategory == null) {
throw new EvalException(
artifactNamePatternStruct.getCreationLocation(),
String.format("Artifact category %s not recognized.", categoryName));
throw infoError(
artifactNamePatternStruct, "Artifact category %s not recognized.", categoryName);
}

String extension =
Strings.nullToEmpty(
getMandatoryFieldFromStarlarkProvider(
artifactNamePatternStruct, "extension", String.class));
if (!foundCategory.getAllowedExtensions().contains(extension)) {
throw new EvalException(
artifactNamePatternStruct.getCreationLocation(),
String.format(
"Unrecognized file extension '%s', allowed extensions are %s,"
+ " please check artifact_name_pattern configuration for %s in your rule.",
extension,
StringUtil.joinEnglishList(foundCategory.getAllowedExtensions(), "or", "'"),
foundCategory.getCategoryName()));
throw infoError(
artifactNamePatternStruct,
"Unrecognized file extension '%s', allowed extensions are %s,"
+ " please check artifact_name_pattern configuration for %s in your rule.",
extension,
StringUtil.joinEnglishList(foundCategory.getAllowedExtensions(), "or", "'"),
foundCategory.getCategoryName());
}

String prefix =
Expand All @@ -1654,9 +1640,7 @@ private static <T> T getFieldFromStarlarkProvider(
Object obj = provider.getValue(fieldName);
if (obj == null) {
if (mandatory) {
throw new EvalException(
provider.getCreationLocation(),
String.format("Missing mandatory field '%s'", fieldName));
throw infoError(provider, "Missing mandatory field '%s'", fieldName);
}
return null;
}
Expand All @@ -1666,9 +1650,7 @@ private static <T> T getFieldFromStarlarkProvider(
if (NoneType.class.isInstance(obj)) {
return null;
}
throw new EvalException(
provider.getCreationLocation(),
String.format("Field '%s' is not of '%s' type.", fieldName, clazz.getName()));
throw infoError(provider, "Field '%s' is not of '%s' type.", fieldName, clazz.getName());
}

/** Returns a list of strings from a field of a {@link StarlarkInfo}. */
Expand Down
8 changes: 5 additions & 3 deletions src/test/java/com/google/devtools/build/lib/rules/cpp/StarlarkCcCommonTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3814,9 +3814,11 @@ public void testFeature_name_mustBeNonempty() throws Exception {
assertThat(featureStruct).isNotNull();
EvalException e =
assertThrows(EvalException.class, () -> CcModule.featureFromStarlark(featureStruct));
assertThat(e)
.hasMessageThat()
.contains("A feature must either have a nonempty 'name' field or be enabled.");
String msg = e.getMessage();
assertThat(msg).contains("A feature must either have a nonempty 'name' field or be enabled.");
assertThat(msg)
.contains(
"in FeatureInfo instantiated at /workspace/tools/cpp/cc_toolchain_config_lib.bzl:");
}

@Test
Expand Down

0 comments on commit 323cbc7

Please sign in to comment.