From af77a3005e594c7263158f5b83614b2deee6c64b Mon Sep 17 00:00:00 2001 From: brandjon Date: Tue, 22 Jan 2019 13:28:57 -0800 Subject: [PATCH] Make --experimental_remove_old_python_version_api not breaking existing_rules() The current way of gating access to the deprecated attribute `default_python_version` is to have the rule check whether or not the attribute was set explicitly (AttributeMap#isAttributeValueExplicitlySpecified), and match that against whether or not the attribute is allowed. However, this breaks the use case of copying a rule definition programmatically via `native.existing_rules()`, because when the definition is copied all the attributes of the new target appear to be explicitly specified. The solution is to use a sentinel value as the physical default value in the attribute definition. Then the rule just checks for equality with that sentinel. If the configuration permits use of that attribute, then the sentinel is interpreted the same as what the logical default would be. This requires adding an item to the PythonVersion enum. Ugly, but necessary. We can remove it once the incompatible flag is flipped to remove the old API. Work toward #6583. Fixes #7071. RELNOTES: None PiperOrigin-RevId: 230399577 --- .../bazel/rules/python/BazelPyBinaryRule.java | 2 +- .../rules/python/BazelPyRuleClasses.java | 10 +-- .../bazel/rules/python/BazelPyTestRule.java | 2 +- .../build/lib/packages/Attribute.java | 2 +- .../build/lib/packages/AttributeMap.java | 19 ++-- .../build/lib/rules/python/PyCommon.java | 78 ++++++++++++++-- .../build/lib/rules/python/PyRuleClasses.java | 89 +++++++++++-------- .../build/lib/rules/python/PythonVersion.java | 57 ++++++++++-- .../PyExecutableConfiguredTargetTestBase.java | 35 ++++++++ .../lib/rules/python/PythonVersionTest.java | 81 +++++++++++------ 10 files changed, 286 insertions(+), 89 deletions(-) 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"); } }