diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPyBinaryRule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPyBinaryRule.java index ff74a30a753468..cfa10dd068ea45 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPyBinaryRule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPyBinaryRule.java @@ -41,7 +41,7 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env) */ return builder .requiresConfigurationFragments(PythonConfiguration.class, BazelPythonConfiguration.class) - .cfg(PyRuleClasses.PYTHON_VERSION_TRANSITION) + .cfg(PyRuleClasses.VERSION_TRANSITION) .add( attr("$zipper", LABEL) .cfg(HostTransition.INSTANCE) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPyRuleClasses.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPyRuleClasses.java index 99184bbe8d7bec..355b3d378b4c83 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPyRuleClasses.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPyRuleClasses.java @@ -111,7 +111,7 @@ A string specifying the Python major version(s) that the .py source .add( attr("srcs_version", STRING) .value(PythonVersion.DEFAULT_SRCS_VALUE.toString()) - .allowedValues(new AllowedValueSet(PythonVersion.ALL_STRINGS))) + .allowedValues(new AllowedValueSet(PythonVersion.SRCS_STRINGS))) // TODO(brandjon): Consider adding to py_interpreter a .mandatoryNativeProviders() of // BazelPyRuntimeProvider. (Add a test case to PythonConfigurationTest for violations // of this requirement.) @@ -170,8 +170,8 @@ public RuleClass build(RuleClass.Builder builder, final RuleDefinitionEnvironmen */ .add( attr(PyCommon.DEFAULT_PYTHON_VERSION_ATTRIBUTE, STRING) - .value(PythonVersion.DEFAULT_TARGET_VALUE.toString()) - .allowedValues(new AllowedValueSet(PythonVersion.TARGET_STRINGS)) + .value(PythonVersion._INTERNAL_SENTINEL.toString()) + .allowedValues(PyRuleClasses.TARGET_PYTHON_ATTR_VALUE_SET) .nonconfigurable( "read by PyRuleClasses.PYTHON_VERSION_TRANSITION, which doesn't have access" + " to the configuration")) @@ -181,8 +181,8 @@ public RuleClass build(RuleClass.Builder builder, final RuleDefinitionEnvironmen */ .add( attr(PyCommon.PYTHON_VERSION_ATTRIBUTE, STRING) - .value(PythonVersion.DEFAULT_TARGET_VALUE.toString()) - .allowedValues(new AllowedValueSet(PythonVersion.TARGET_STRINGS)) + .value(PythonVersion._INTERNAL_SENTINEL.toString()) + .allowedValues(PyRuleClasses.TARGET_PYTHON_ATTR_VALUE_SET) .nonconfigurable( "read by PyRuleClasses.PYTHON_VERSION_TRANSITION, which doesn't have access" + " to the configuration")) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPyTestRule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPyTestRule.java index 4723f58d7a83c4..4c07e5a060123e 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPyTestRule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPyTestRule.java @@ -39,7 +39,7 @@ public final class BazelPyTestRule implements RuleDefinition { public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env) { return builder .requiresConfigurationFragments(PythonConfiguration.class, BazelPythonConfiguration.class) - .cfg(PyRuleClasses.PYTHON_VERSION_TRANSITION) + .cfg(PyRuleClasses.VERSION_TRANSITION) .add( attr("$zipper", LABEL) .cfg(HostTransition.INSTANCE) diff --git a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java index 8beeb0a38e362c..83f491417cb247 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java +++ b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java @@ -329,7 +329,7 @@ public static class AllowedValueSet implements PredicateWithMessage { private final Set allowedValues; - public AllowedValueSet(T... values) { + public AllowedValueSet(Object... values) { this(Arrays.asList(values)); } diff --git a/src/main/java/com/google/devtools/build/lib/packages/AttributeMap.java b/src/main/java/com/google/devtools/build/lib/packages/AttributeMap.java index faf3d9ea4516af..18886695a43869 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/AttributeMap.java +++ b/src/main/java/com/google/devtools/build/lib/packages/AttributeMap.java @@ -93,13 +93,20 @@ public interface AttributeMap { @Nullable Attribute getAttributeDefinition(String attrName); /** - * Returns true iff the value of the specified attribute is explicitly set in the BUILD file (as - * opposed to its default value). This also returns true if the value from the BUILD file is the - * same as the default value. + * Returns true iff the specified attribute is explicitly set in the target's definition (as + * opposed to being omitted and taking on its default value from the rule definition). * - *

It is probably a good idea to avoid this method in default value and implicit outputs - * computation, because it is confusing that setting an attribute to an empty list (for example) - * is different from not setting it at all. + *

Note that this returns true in the case where the attribute is explicitly set to the same + * value as its default. Therefore, this method breaks encapsulation in the sense that it + * describes *how* a target is defined rather than just *what* its attribute values are. + * + *

CAUTION: It is a good idea to avoid relying on this method if possible. It's confusing to + * users that setting an attribute to (for example) an empty list is different from not setting it + * at all. It also breaks some use cases, such as programmatically copying a target definition via + * {@code native.existing_rules}. Specifically, the Starlark code doing the copying will observe + * the attribute on the existing target whether or not it was set explicitly, and then set that + * value explicitly on the new target. This can cause the two targets to behave differently, and + * can be a difficult bug to track down. (See #7071, b/122596733). */ boolean isAttributeValueExplicitlySpecified(String attributeName); diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java index 05a5a417d6ad88..15213c1d634eee 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java @@ -65,6 +65,37 @@ public final class PyCommon { public static final String DEFAULT_PYTHON_VERSION_ATTRIBUTE = "default_python_version"; public static final String PYTHON_VERSION_ATTRIBUTE = "python_version"; + /** + * Returns the Python version based on the {@code python_version} and {@code + * default_python_version} attributes of the given {@code AttributeMap}. + * + *

It is expected that both attributes are defined, string-typed, and default to {@link + * PythonVersion#_INTERNAL_SENTINEL}. The returned version is the value of {@code python_version} + * if it is not the sentinel, then {@code default_python_version} if it is not the sentinel, then + * finally {@code default}. In all cases the return value is a target version value (either {@code + * PY2} or {@code PY3}). + * + * @throws IllegalArgumentException if the attributes are not present, not string-typed, or not + * parsable as target {@link PythonVersion} values or as the sentinel value; or if {@code + * default} is not a target version value + */ + public static PythonVersion readPythonVersionFromAttributes( + AttributeMap attrs, PythonVersion defaultVersion) { + Preconditions.checkArgument(defaultVersion.isTargetValue()); + PythonVersion pythonVersionAttr = + PythonVersion.parseTargetOrSentinelValue(attrs.get(PYTHON_VERSION_ATTRIBUTE, Type.STRING)); + PythonVersion defaultPythonVersionAttr = + PythonVersion.parseTargetOrSentinelValue( + attrs.get(DEFAULT_PYTHON_VERSION_ATTRIBUTE, Type.STRING)); + if (pythonVersionAttr != PythonVersion._INTERNAL_SENTINEL) { + return pythonVersionAttr; + } else if (defaultPythonVersionAttr != PythonVersion._INTERNAL_SENTINEL) { + return defaultPythonVersionAttr; + } else { + return defaultVersion; + } + } + private static final LocalMetadataCollector METADATA_COLLECTOR = new LocalMetadataCollector() { @Override public void collectMetadataArtifacts(Iterable artifacts, @@ -153,7 +184,9 @@ public PyCommon(RuleContext ruleContext, PythonSemantics semantics) { this.hasPy3OnlySources = initHasPy3OnlySources(ruleContext, this.sourcesVersion); this.convertedFiles = makeAndInitConvertedFiles(ruleContext, version, this.sourcesVersion); maybeValidateVersionCompatibleWithOwnSourcesAttr(); - validatePythonVersionAttr(); + validateTargetPythonVersionAttr(DEFAULT_PYTHON_VERSION_ATTRIBUTE); + validateTargetPythonVersionAttr(PYTHON_VERSION_ATTRIBUTE); + validateOldVersionAttrNotUsedIfDisabled(); } /** Returns the parsed value of the "srcs_version" attribute. */ @@ -167,7 +200,7 @@ private static PythonVersion initSrcsVersionAttr(RuleContext ruleContext) { "srcs_version", String.format( "'%s' is not a valid value. Expected one of: %s", - attrValue, Joiner.on(", ").join(PythonVersion.ALL_STRINGS))); + attrValue, Joiner.on(", ").join(PythonVersion.SRCS_STRINGS))); return PythonVersion.DEFAULT_SRCS_VALUE; } } @@ -386,16 +419,49 @@ private void maybeValidateVersionCompatibleWithOwnSourcesAttr() { } } + /** + * Reports an attribute error if the given target Python version attribute ({@code + * default_python_version} or {@code python_version}) cannot be parsed as {@code PY2}, {@code + * PY3}, or the sentinel value. + * + *

This *should* be enforced by rule attribute validation ({@link + * Attribute.Builder.allowedValues}), but this check is here to fail-fast just in case. + */ + private void validateTargetPythonVersionAttr(String attr) { + AttributeMap attrs = ruleContext.attributes(); + if (!attrs.has(attr, Type.STRING)) { + return; + } + String attrValue = attrs.get(attr, Type.STRING); + try { + PythonVersion.parseTargetOrSentinelValue(attrValue); + } catch (IllegalArgumentException ex) { + ruleContext.attributeError( + attr, + String.format("'%s' is not a valid value. Expected either 'PY2' or 'PY3'", attrValue)); + } + } + /** * Reports an attribute error if the {@code default_python_version} attribute is set but * disallowed by the configuration. */ - private void validatePythonVersionAttr() { + private void validateOldVersionAttrNotUsedIfDisabled() { AttributeMap attrs = ruleContext.attributes(); + if (!attrs.has(DEFAULT_PYTHON_VERSION_ATTRIBUTE, Type.STRING)) { + return; + } + PythonVersion value; + try { + value = + PythonVersion.parseTargetOrSentinelValue( + attrs.get(DEFAULT_PYTHON_VERSION_ATTRIBUTE, Type.STRING)); + } catch (IllegalArgumentException e) { + // Should be reported by validateTargetPythonVersionAttr(); no action required here. + return; + } PythonConfiguration config = ruleContext.getFragment(PythonConfiguration.class); - if (attrs.has(DEFAULT_PYTHON_VERSION_ATTRIBUTE, Type.STRING) - && attrs.isAttributeValueExplicitlySpecified(DEFAULT_PYTHON_VERSION_ATTRIBUTE) - && !config.oldPyVersionApiAllowed()) { + if (value != PythonVersion._INTERNAL_SENTINEL && !config.oldPyVersionApiAllowed()) { ruleContext.attributeError( DEFAULT_PYTHON_VERSION_ATTRIBUTE, "the 'default_python_version' attribute is disabled by the " diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyRuleClasses.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyRuleClasses.java index 16548fa8afe5eb..ac16325becd631 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PyRuleClasses.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PyRuleClasses.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.rules.python; import com.google.common.base.Preconditions; +import com.google.devtools.build.lib.packages.Attribute.AllowedValueSet; import com.google.devtools.build.lib.packages.AttributeMap; import com.google.devtools.build.lib.packages.RawAttributeMapper; import com.google.devtools.build.lib.packages.RuleClass; @@ -27,43 +28,61 @@ public class PyRuleClasses { public static final FileType PYTHON_SOURCE = FileType.of(".py", ".py3"); /** - * A rule transition factory for Python binary rules and other rules that may change the Python - * version. + * A value set of the target and sentinel values that doesn't mention the sentinel in error + * messages. + */ + public static final AllowedValueSet TARGET_PYTHON_ATTR_VALUE_SET = + new AllowedValueSet(PythonVersion.TARGET_AND_SENTINEL_STRINGS) { + @Override + public String getErrorReason(Object value) { + return String.format("has to be one of 'PY2' or 'PY3' instead of '%s'", value); + } + }; + + /** + * Returns a rule transition factory for Python binary rules and other rules that may change the + * Python version. * - *

This sets the Python version to the value specified by {@code python_version} if it is given - * explicitly, or the (possibly default) value of {@code default_python_version} otherwise. + *

The factory makes a transition to set the Python version to the value specified by the + * rule's {@code python_version} attribute if it is given, or otherwise the {@code + * default_python_version} attribute if it is given, or otherwise the default value passed into + * this function. * - *

The transition throws {@link IllegalArgumentException} if used on a rule whose {@link - * RuleClass} does not define both attributes. If both are defined, but the one whose value is to - * be read cannot be parsed as a Python version, {@link PythonVersion#DEFAULT_TARGET_VALUE} is - * used instead; in this case it is up to the rule's analysis phase (in {@link PyCommon}) to - * report an attribute error to the user. + *

The factory throws {@link IllegalArgumentException} if used on a rule whose {@link + * RuleClass} does not define both attributes. If both are defined, but one of their values cannot + * be parsed as a Python version, the given default value is used as a fallback instead; in this + * case it is up to the rule's analysis phase ({@link PyCommon#validateTargetPythonVersionAttr}) + * to report an attribute error to the user. This case should be prevented by attribute validation + * if the rule is defined correctly. */ - public static final RuleTransitionFactory PYTHON_VERSION_TRANSITION = - (rule) -> { - AttributeMap attrs = RawAttributeMapper.of(rule); - Preconditions.checkArgument( - attrs.has(PyCommon.PYTHON_VERSION_ATTRIBUTE, Type.STRING) - && attrs.has(PyCommon.DEFAULT_PYTHON_VERSION_ATTRIBUTE, Type.STRING), - "python version transitions require that the RuleClass define both " - + "'default_python_version' and 'python_version'"); + public static RuleTransitionFactory makeVersionTransition(PythonVersion defaultVersion) { + return (rule) -> { + AttributeMap attrs = RawAttributeMapper.of(rule); + // Fail fast if we're used on an ill-defined rule class. + Preconditions.checkArgument( + attrs.has(PyCommon.PYTHON_VERSION_ATTRIBUTE, Type.STRING) + && attrs.has(PyCommon.DEFAULT_PYTHON_VERSION_ATTRIBUTE, Type.STRING), + "python version transitions require that the RuleClass define both " + + "'default_python_version' and 'python_version'"); + // Attribute validation should enforce that the attribute string value is either a target + // value ("PY2" or "PY3") or the sentinel value ("_INTERNAL_SENTINEL"). But just in case, + // we'll, treat an invalid value as the default value rather than propagate an unchecked + // exception in this context. That way the user can at least get a clean error message + // instead of a crash. + PythonVersion version; + try { + version = PyCommon.readPythonVersionFromAttributes(attrs, defaultVersion); + } catch (IllegalArgumentException ex) { + version = defaultVersion; + } + return new PythonVersionTransition(version); + }; + } - String versionString = - attrs.isAttributeValueExplicitlySpecified(PyCommon.PYTHON_VERSION_ATTRIBUTE) - ? attrs.get(PyCommon.PYTHON_VERSION_ATTRIBUTE, Type.STRING) - : attrs.get(PyCommon.DEFAULT_PYTHON_VERSION_ATTRIBUTE, Type.STRING); - - // It should be a target value ("PY2" or "PY3"), and if not that should be caught by - // attribute validation. But just in case, we'll treat an invalid value as null (which means - // "use the hard-coded default version") rather than propagate an unchecked exception in - // this context. That way the user can at least get a clean error message instead of a - // crash. - PythonVersion version; - try { - version = PythonVersion.parseTargetValue(versionString); - } catch (IllegalArgumentException ex) { - version = null; - } - return new PythonVersionTransition(version); - }; + /** + * A Python version transition that sets the version as specified by the target's attributes, with + * a default of {@link PythonVersion#DEFAULT_TARGET_VALUE}. + */ + public static final RuleTransitionFactory VERSION_TRANSITION = + makeVersionTransition(PythonVersion.DEFAULT_TARGET_VALUE); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PythonVersion.java b/src/main/java/com/google/devtools/build/lib/rules/python/PythonVersion.java index 3e906dc1f2bc7c..efa633c94a9806 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PythonVersion.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PythonVersion.java @@ -67,7 +67,19 @@ public enum PythonVersion { * *

Deprecated meaning: Indicates code that cannot be processed by 3to2. */ - PY3ONLY; + PY3ONLY, + + /** + * Internal sentinel value used as the default value of the {@code python_version} and {@code + * default_python_version} attributes. + * + *

This should not be referenced by the user. But since we can't actually hide it from Starlark + * ({@code native.existing_rules()}) or bazel query, we give it the scary "_internal" prefix + * instead. + * + *

The logical meaning of this value is the same as {@link #DEFAULT_TARGET_VALUE}. + */ + _INTERNAL_SENTINEL; private static ImmutableList convertToStrings(List values) { return values.stream() @@ -75,19 +87,27 @@ private static ImmutableList convertToStrings(List values .collect(ImmutableList.toImmutableList()); } - /** All enum values. */ - public static final ImmutableList ALL_VALUES = - ImmutableList.of(PY2, PY3, PY2AND3, PY2ONLY, PY3ONLY); - - /** String names of all enum values. */ - public static final ImmutableList ALL_STRINGS = convertToStrings(ALL_VALUES); - /** Enum values representing a distinct Python version. */ public static final ImmutableList TARGET_VALUES = ImmutableList.of(PY2, PY3); /** String names of enum values representing a distinct Python version. */ public static final ImmutableList TARGET_STRINGS = convertToStrings(TARGET_VALUES); + /** Target values plus the sentinel value. */ + public static final ImmutableList TARGET_AND_SENTINEL_VALUES = + ImmutableList.of(PY2, PY3, _INTERNAL_SENTINEL); + + /** String names of target values plus the sentinel value. */ + public static final ImmutableList TARGET_AND_SENTINEL_STRINGS = + convertToStrings(TARGET_AND_SENTINEL_VALUES); + + /** All values not including the sentinel. */ + public static final ImmutableList SRCS_VALUES = + ImmutableList.of(PY2, PY3, PY2AND3, PY2ONLY, PY3ONLY); + + /** String names of all enum values not including the sentinel. */ + public static final ImmutableList SRCS_STRINGS = convertToStrings(SRCS_VALUES); + /** Enum values that do not imply running a transpiler to convert between versions. */ public static final ImmutableList NON_CONVERSION_VALUES = ImmutableList.of(PY2AND3, PY2ONLY, PY3ONLY); @@ -120,12 +140,31 @@ public static PythonVersion parseTargetValue(String str) { return PythonVersion.valueOf(str); } + /** + * Converts the string to a target or sentinel {@code PythonVersion} value (case-sensitive). + * + * @throws IllegalArgumentException if the string is not "PY2", "PY3", or "_INTERNAL_SENTINEL". + */ + public static PythonVersion parseTargetOrSentinelValue(String str) { + if (!TARGET_AND_SENTINEL_STRINGS.contains(str)) { + // Use the same error message as for parseTargetValue, because the user shouldn't be aware of + // the sentinel value. + throw new IllegalArgumentException( + String.format("'%s' is not a valid Python major version. Expected 'PY2' or 'PY3'.", str)); + } + return PythonVersion.valueOf(str); + } + /** * Converts the string to a sources {@code PythonVersion} value (case-sensitive). * - * @throws IllegalArgumentException if the string is not an enum name. + * @throws IllegalArgumentException if the string is not an enum name or is the sentinel value. */ public static PythonVersion parseSrcsValue(String str) { + if (!SRCS_STRINGS.contains(str)) { + throw new IllegalArgumentException( + String.format("'%s' is not a valid Python srcs_version value.", str)); + } return PythonVersion.valueOf(str); } } diff --git a/src/test/java/com/google/devtools/build/lib/rules/python/PyExecutableConfiguredTargetTestBase.java b/src/test/java/com/google/devtools/build/lib/rules/python/PyExecutableConfiguredTargetTestBase.java index 9c8b03257b48b7..f548df9ff52c45 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/python/PyExecutableConfiguredTargetTestBase.java +++ b/src/test/java/com/google/devtools/build/lib/rules/python/PyExecutableConfiguredTargetTestBase.java @@ -207,6 +207,41 @@ public void cannotUseOldVersionAttrWithRemovalFlag() throws Exception { ruleDeclWithDefaultPyVersionAttr("foo", "PY2")); } + /** + * Regression test for #7071: Don't let prohibiting the old attribute get in the way of cloning a + * target using {@code native.existing_rules()}. + * + *

The use case of cloning a target is pretty dubious and brittle. But as long as it's possible + * and not proscribed, we won't let version attribute validation get in the way. + */ + @Test + public void canCopyTargetWhenOldAttrDisallowed() throws Exception { + useConfiguration("--experimental_remove_old_python_version_api=true"); + scratch.file( + "pkg/rules.bzl", + "def copy_target(rulefunc, src, dest):", + " t = native.existing_rule(src)", + " t.pop('kind')", + " t.pop('name')", + " # Also remove these because they get in the way of creating the new target but aren't", + " # related to the attribute under test.", + " t.pop('restricted_to')", + " t.pop('shard_count', default=None)", + " rulefunc(name = dest, **t)"); + scratch.file( + "pkg/BUILD", + "load(':rules.bzl', 'copy_target')", + ruleName + "(", + " name = 'foo',", + " srcs = ['foo.py'],", + " main = 'foo.py',", + " python_version = 'PY2',", + ")", + "copy_target(" + ruleName + ", 'foo', 'bar')"); + ConfiguredTarget target = getConfiguredTarget("//pkg:bar"); + assertThat(target).isNotNull(); + } + @Test public void newVersionAttrTakesPrecedenceOverOld() throws Exception { scratch.file( diff --git a/src/test/java/com/google/devtools/build/lib/rules/python/PythonVersionTest.java b/src/test/java/com/google/devtools/build/lib/rules/python/PythonVersionTest.java index e49d94a7353fe8..e9ea763ced8ea6 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/python/PythonVersionTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/python/PythonVersionTest.java @@ -25,40 +25,71 @@ @RunWith(JUnit4.class) public class PythonVersionTest { + private static void assertIsInvalidForParseTargetValue(String value) { + assertThat( + assertThrows( + IllegalArgumentException.class, () -> PythonVersion.parseTargetValue(value))) + .hasMessageThat() + .contains("not a valid Python major version"); + } + + private static void assertIsInvalidForParseTargetOrSentinelValue(String value) { + assertThat( + assertThrows( + IllegalArgumentException.class, + () -> PythonVersion.parseTargetOrSentinelValue(value))) + .hasMessageThat() + .contains("not a valid Python major version"); + } + + private static void assertIsInvalidForParseSrcsValue(String value) { + assertThat( + assertThrows(IllegalArgumentException.class, () -> PythonVersion.parseSrcsValue(value))) + .hasMessageThat() + .contains("not a valid Python srcs_version value"); + } + + @Test + public void isTargetValue() { + assertThat(PythonVersion.PY2.isTargetValue()).isTrue(); + assertThat(PythonVersion.PY3.isTargetValue()).isTrue(); + assertThat(PythonVersion.PY2AND3.isTargetValue()).isFalse(); + assertThat(PythonVersion.PY2ONLY.isTargetValue()).isFalse(); + assertThat(PythonVersion.PY3ONLY.isTargetValue()).isFalse(); + assertThat(PythonVersion._INTERNAL_SENTINEL.isTargetValue()).isFalse(); + } + @Test public void parseTargetValue() { assertThat(PythonVersion.parseTargetValue("PY2")).isEqualTo(PythonVersion.PY2); + assertThat(PythonVersion.parseTargetValue("PY3")).isEqualTo(PythonVersion.PY3); + assertIsInvalidForParseTargetValue("PY2AND3"); + assertIsInvalidForParseTargetValue("PY2ONLY"); + assertIsInvalidForParseTargetValue("PY3ONLY"); + assertIsInvalidForParseTargetValue("_INTERNAL_SENTINEL"); + assertIsInvalidForParseTargetValue("not an enum value"); + } - IllegalArgumentException expected = - assertThrows( - IllegalArgumentException.class, () -> PythonVersion.parseTargetValue("PY2AND3")); - assertThat(expected).hasMessageThat().contains("not a valid Python major version"); - - expected = - assertThrows( - IllegalArgumentException.class, - () -> PythonVersion.parseTargetValue("not an enum value")); - assertThat(expected).hasMessageThat().contains("not a valid Python major version"); - - expected = - assertThrows(IllegalArgumentException.class, () -> PythonVersion.parseTargetValue("py2")); - assertThat(expected).hasMessageThat().contains("not a valid Python major version"); + @Test + public void parseTargetOrSentinelValue() { + assertThat(PythonVersion.parseTargetOrSentinelValue("PY2")).isEqualTo(PythonVersion.PY2); + assertThat(PythonVersion.parseTargetOrSentinelValue("PY3")).isEqualTo(PythonVersion.PY3); + assertIsInvalidForParseTargetOrSentinelValue("PY2AND3"); + assertIsInvalidForParseTargetOrSentinelValue("PY2ONLY"); + assertIsInvalidForParseTargetOrSentinelValue("PY3ONLY"); + assertThat(PythonVersion.parseTargetOrSentinelValue("_INTERNAL_SENTINEL")) + .isEqualTo(PythonVersion._INTERNAL_SENTINEL); + assertIsInvalidForParseTargetOrSentinelValue("not an enum value"); } @Test public void parseSrcsValue() { assertThat(PythonVersion.parseSrcsValue("PY2")).isEqualTo(PythonVersion.PY2); - + assertThat(PythonVersion.parseSrcsValue("PY3")).isEqualTo(PythonVersion.PY3); assertThat(PythonVersion.parseSrcsValue("PY2AND3")).isEqualTo(PythonVersion.PY2AND3); - - IllegalArgumentException expected = - assertThrows( - IllegalArgumentException.class, - () -> PythonVersion.parseSrcsValue("not an enum value")); - assertThat(expected).hasMessageThat().contains("No enum constant"); - - expected = - assertThrows(IllegalArgumentException.class, () -> PythonVersion.parseSrcsValue("py2")); - assertThat(expected).hasMessageThat().contains("No enum constant"); + assertThat(PythonVersion.parseSrcsValue("PY2ONLY")).isEqualTo(PythonVersion.PY2ONLY); + assertThat(PythonVersion.parseSrcsValue("PY3ONLY")).isEqualTo(PythonVersion.PY3ONLY); + assertIsInvalidForParseSrcsValue("_INTERNAL_SENTINEL"); + assertIsInvalidForParseSrcsValue("not an enum value"); } }