diff options
author | John Cater <jcater@google.com> | 2017-04-21 21:20:45 +0200 |
---|---|---|
committer | Vladimir Moskva <vladmos@google.com> | 2017-04-24 16:51:26 +0200 |
commit | e3d46b7a8edb49415c7a1c23550bf735df35b5da (patch) | |
tree | 3d8d21431c101af517b8d5369ca351b9b563427b /src/main/java/com | |
parent | 410147aed773a73d611635bc421c5b4564aaaac7 (diff) |
Refactor PlatformInfo.Builder to detect and report invalid configurations.
Change-Id: Ib2bbd1b35985c4ec2d1e411aea4b32af7433a426
PiperOrigin-RevId: 153856560
Diffstat (limited to 'src/main/java/com')
-rw-r--r-- | src/main/java/com/google/devtools/build/lib/rules/platform/Platform.java | 71 | ||||
-rw-r--r-- | src/main/java/com/google/devtools/build/lib/rules/platform/PlatformInfo.java | 111 |
2 files changed, 112 insertions, 70 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/rules/platform/Platform.java b/src/main/java/com/google/devtools/build/lib/rules/platform/Platform.java index fde1b73184..c39660585b 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/platform/Platform.java +++ b/src/main/java/com/google/devtools/build/lib/rules/platform/Platform.java @@ -14,10 +14,6 @@ package com.google.devtools.build.lib.rules.platform; -import com.google.common.collect.ArrayListMultimap; -import com.google.common.collect.ImmutableMap; -import com.google.common.collect.Iterables; -import com.google.common.collect.Multimap; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.FileProvider; import com.google.devtools.build.lib.analysis.FilesToRunProvider; @@ -26,6 +22,7 @@ import com.google.devtools.build.lib.analysis.RuleConfiguredTargetBuilder; import com.google.devtools.build.lib.analysis.RuleContext; import com.google.devtools.build.lib.analysis.RunfilesProvider; import com.google.devtools.build.lib.rules.RuleConfiguredTargetFactory; +import com.google.devtools.build.lib.rules.platform.PlatformInfo.DuplicateConstraintException; import com.google.devtools.build.lib.syntax.Type; import java.util.Map; @@ -39,71 +36,27 @@ public class Platform implements RuleConfiguredTargetFactory { ConstraintValueInfo.fromTargets( ruleContext.getPrerequisites(PlatformRule.CONSTRAINT_VALUES_ATTR, Mode.DONT_CHECK)); - // Verify the constraints - no two values for the same setting, and construct the map. - ImmutableMap<ConstraintSettingInfo, ConstraintValueInfo> constraints = - validateConstraints(ruleContext, constraintValues); - if (constraints == null) { - // An error occurred, return null. - return null; - } - PlatformInfo.Builder platformBuilder = PlatformInfo.builder(); - platformBuilder.addConstraints(constraints.values()); + platformBuilder.addConstraints(constraintValues); Map<String, String> remoteExecutionProperties = ruleContext.attributes().get(PlatformRule.REMOTE_EXECUTION_PROPS_ATTR, Type.STRING_DICT); platformBuilder.addRemoteExecutionProperties(remoteExecutionProperties); + PlatformInfo platformInfo; + try { + platformInfo = platformBuilder.build(); + } catch (DuplicateConstraintException e) { + // Report the error and return null. + ruleContext.attributeError(PlatformRule.CONSTRAINT_VALUES_ATTR, e.getMessage()); + return null; + } + return new RuleConfiguredTargetBuilder(ruleContext) .addProvider(RunfilesProvider.class, RunfilesProvider.EMPTY) .addProvider(FileProvider.class, FileProvider.EMPTY) .addProvider(FilesToRunProvider.class, FilesToRunProvider.EMPTY) - .addNativeDeclaredProvider(platformBuilder.build()) + .addNativeDeclaredProvider(platformInfo) .build(); } - - private ImmutableMap<ConstraintSettingInfo, ConstraintValueInfo> validateConstraints( - RuleContext ruleContext, Iterable<ConstraintValueInfo> constraintValues) { - Multimap<ConstraintSettingInfo, ConstraintValueInfo> constraints = ArrayListMultimap.create(); - - for (ConstraintValueInfo constraintValue : constraintValues) { - constraints.put(constraintValue.constraint(), constraintValue); - } - - // Are there any settings with more than one value? - boolean foundError = false; - for (ConstraintSettingInfo constraintSetting : constraints.keySet()) { - if (constraints.get(constraintSetting).size() > 1) { - foundError = true; - // error - StringBuilder constraintValuesDescription = new StringBuilder(); - for (ConstraintValueInfo constraintValue : constraints.get(constraintSetting)) { - if (constraintValuesDescription.length() > 0) { - constraintValuesDescription.append(", "); - } - constraintValuesDescription.append(constraintValue.label()); - } - ruleContext.attributeError( - PlatformRule.CONSTRAINT_VALUES_ATTR, - String.format( - "Duplicate constraint_values for constraint_setting %s: %s", - constraintSetting.label(), constraintValuesDescription.toString())); - } - } - - if (foundError) { - return null; - } - - // Convert to a flat map. - ImmutableMap.Builder<ConstraintSettingInfo, ConstraintValueInfo> builder = - new ImmutableMap.Builder<>(); - for (ConstraintSettingInfo constraintSetting : constraints.keySet()) { - ConstraintValueInfo constraintValue = - Iterables.getOnlyElement(constraints.get(constraintSetting)); - builder.put(constraintSetting, constraintValue); - } - - return builder.build(); - } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/platform/PlatformInfo.java b/src/main/java/com/google/devtools/build/lib/rules/platform/PlatformInfo.java index 7406c1404d..3f980733cf 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/platform/PlatformInfo.java +++ b/src/main/java/com/google/devtools/build/lib/rules/platform/PlatformInfo.java @@ -15,9 +15,12 @@ package com.google.devtools.build.lib.rules.platform; import com.google.common.base.Function; +import com.google.common.collect.ArrayListMultimap; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; +import com.google.common.collect.Multimap; import com.google.devtools.build.lib.analysis.TransitiveInfoCollection; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; import com.google.devtools.build.lib.packages.ClassObjectConstructor; @@ -53,8 +56,8 @@ public class PlatformInfo extends SkylarkClassObject { public static final SkylarkProviderIdentifier SKYLARK_IDENTIFIER = SkylarkProviderIdentifier.forKey(SKYLARK_CONSTRUCTOR.getKey()); - private ImmutableList<ConstraintValueInfo> constraints; - private ImmutableMap<String, String> remoteExecutionProperties; + private final ImmutableList<ConstraintValueInfo> constraints; + private final ImmutableMap<String, String> remoteExecutionProperties; private PlatformInfo( ImmutableList<ConstraintValueInfo> constraints, @@ -101,22 +104,33 @@ public class PlatformInfo extends SkylarkClassObject { }); } + /** Returns a new {@link Builder} for creating a fresh {@link PlatformInfo} instance. */ public static Builder builder() { return new Builder(); } - /** - * Builder class to facilitate creating valid {@link PlatformInfo} instances. - */ + /** Builder class to facilitate creating valid {@link PlatformInfo} instances. */ public static class Builder { - private List<ConstraintValueInfo> constraints = new ArrayList<>(); - private Map<String, String> remoteExecutionProperties = new HashMap<>(); - + private final List<ConstraintValueInfo> constraints = new ArrayList<>(); + private final Map<String, String> remoteExecutionProperties = new HashMap<>(); + + /** + * Adds the given constraint value to the constraints that define this {@link PlatformInfo}. + * + * @param constraint the constraint to add + * @return the {@link Builder} instance for method chaining + */ public Builder addConstraint(ConstraintValueInfo constraint) { this.constraints.add(constraint); return this; } + /** + * Adds the given constraint values to the constraints that define this {@link PlatformInfo}. + * + * @param constraints the constraints to add + * @return the {@link Builder} instance for method chaining + */ public Builder addConstraints(Iterable<ConstraintValueInfo> constraints) { for (ConstraintValueInfo constraint : constraints) { this.addConstraint(constraint); @@ -125,11 +139,26 @@ public class PlatformInfo extends SkylarkClassObject { return this; } + /** + * Adds the given key/value pair to the data being sent to a potential remote executor. If the + * key already exists in the map, the previous value will be overwritten. + * + * @param key the key to be used + * @param value the value to be used + * @return the {@link Builder} instance for method chaining + */ public Builder addRemoteExecutionProperty(String key, String value) { this.remoteExecutionProperties.put(key, value); return this; } + /** + * Adds the given properties to the data being sent to a potential remote executor. If any key + * already exists in the map, the previous value will be overwritten. + * + * @param properties the properties to be added + * @return the {@link Builder} instance for method chaining + */ public Builder addRemoteExecutionProperties(Map<String, String> properties) { for (Map.Entry<String, String> entry : properties.entrySet()) { this.addRemoteExecutionProperty(entry.getKey(), entry.getValue()); @@ -137,9 +166,69 @@ public class PlatformInfo extends SkylarkClassObject { return this; } - public PlatformInfo build() { - return new PlatformInfo( - ImmutableList.copyOf(constraints), ImmutableMap.copyOf(remoteExecutionProperties)); + /** + * Returns the new {@link PlatformInfo} instance. + * + * @throws DuplicateConstraintException if more than one constraint value exists for the same + * constraint setting + */ + public PlatformInfo build() throws DuplicateConstraintException { + ImmutableList<ConstraintValueInfo> validatedConstraints = validateConstraints(constraints); + return new PlatformInfo(validatedConstraints, ImmutableMap.copyOf(remoteExecutionProperties)); + } + + private ImmutableList<ConstraintValueInfo> validateConstraints( + Iterable<ConstraintValueInfo> constraintValues) throws DuplicateConstraintException { + Multimap<ConstraintSettingInfo, ConstraintValueInfo> constraints = ArrayListMultimap.create(); + + for (ConstraintValueInfo constraintValue : constraintValues) { + constraints.put(constraintValue.constraint(), constraintValue); + } + + // Are there any settings with more than one value? + for (ConstraintSettingInfo constraintSetting : constraints.keySet()) { + if (constraints.get(constraintSetting).size() > 1) { + // Only reports the first case of this error. + throw new DuplicateConstraintException( + constraintSetting, constraints.get(constraintSetting)); + } + } + + return ImmutableList.copyOf(constraints.values()); + } + } + + /** + * Exception class used when more than one {@link ConstraintValueInfo} for the same {@link + * ConstraintSettingInfo} is added to a {@link Builder}. + */ + public static class DuplicateConstraintException extends Exception { + private final ImmutableSet<ConstraintValueInfo> duplicateConstraints; + + public DuplicateConstraintException( + ConstraintSettingInfo constraintSetting, + Iterable<ConstraintValueInfo> duplicateConstraintValues) { + super(formatError(constraintSetting, duplicateConstraintValues)); + this.duplicateConstraints = ImmutableSet.copyOf(duplicateConstraintValues); + } + + public ImmutableSet<ConstraintValueInfo> duplicateConstraints() { + return duplicateConstraints; + } + + private static String formatError( + ConstraintSettingInfo constraintSetting, + Iterable<ConstraintValueInfo> duplicateConstraints) { + StringBuilder constraintValuesDescription = new StringBuilder(); + for (ConstraintValueInfo constraintValue : duplicateConstraints) { + if (constraintValuesDescription.length() > 0) { + constraintValuesDescription.append(", "); + } + constraintValuesDescription.append(constraintValue.label()); + } + return String.format( + "Duplicate constraint_values for constraint_setting %s: %s", + constraintSetting.label(), constraintValuesDescription.toString()); } } } |