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 c4faad733e8bd9..ff74a30a753468 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.DEFAULT_PYTHON_VERSION_TRANSITION)
+ .cfg(PyRuleClasses.PYTHON_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 4a70c1599c53da..9bbec8674faf9d 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
@@ -164,14 +164,28 @@ public RuleClass build(RuleClass.Builder builder, final RuleDefinitionEnvironmen
all of its deps
.
Valid values are "PY2"
(default) or "PY3"
.
Python 3 support is experimental.
+
If both this attribute and Since this is a configuration transition, this propagates to the rules' transitive deps.
+ * 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 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.
*/
- public static final RuleTransitionFactory DEFAULT_PYTHON_VERSION_TRANSITION =
+ public static final RuleTransitionFactory PYTHON_VERSION_TRANSITION =
(rule) -> {
- String attrDefault = RawAttributeMapper.of(rule).get("default_python_version", Type.STRING);
+ 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'");
+
+ 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.
- PythonVersion version = null;
- // Should be non-null because this transition shouldn't be used on rules without the attr.
- if (attrDefault != null) {
- try {
- version = PythonVersion.parseTargetValue(attrDefault);
- } catch (IllegalArgumentException ex) {
- // Parsing error.
- }
+ // 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);
};
diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyTest.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyTest.java
index 4554321abda429..b398976c5bce7a 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/python/PyTest.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/python/PyTest.java
@@ -34,7 +34,6 @@ public ConfiguredTarget create(RuleContext ruleContext)
throws InterruptedException, RuleErrorException, ActionConflictException {
PythonSemantics semantics = createSemantics();
PyCommon common = new PyCommon(ruleContext);
- common.initCommon(common.getDefaultPythonVersion());
RuleConfiguredTargetBuilder builder = PyBinary.init(ruleContext, semantics, common);
if (builder == null) {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfiguration.java
index 621afa58c4dd3d..c7934941a3a578 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfiguration.java
@@ -41,14 +41,18 @@ public class PythonConfiguration extends BuildConfiguration.Fragment {
private final PythonVersion version;
private final TriState buildPythonZip;
private final boolean buildTransitiveRunfilesTrees;
+ // TODO(brandjon): Remove once migration to the new API is complete (#6583).
+ private final boolean newPyVersionApiEnabled;
PythonConfiguration(
PythonVersion defaultPythonVersion,
TriState buildPythonZip,
- boolean buildTransitiveRunfilesTrees) {
+ boolean buildTransitiveRunfilesTrees,
+ boolean newPyVersionApiEnabled) {
this.version = defaultPythonVersion;
this.buildPythonZip = buildPythonZip;
this.buildTransitiveRunfilesTrees = buildTransitiveRunfilesTrees;
+ this.newPyVersionApiEnabled = newPyVersionApiEnabled;
}
/**
@@ -113,4 +117,12 @@ public boolean buildPythonZip() {
public boolean buildTransitiveRunfilesTrees() {
return buildTransitiveRunfilesTrees;
}
+
+ /**
+ * Returns whether use of {@code --python_version} flag and {@code python_version} attribute is
+ * allowed.
+ */
+ public boolean newPyVersionApiEnabled() {
+ return newPyVersionApiEnabled;
+ }
}
diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfigurationLoader.java b/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfigurationLoader.java
index 93ab29a2fbcab3..42898baf3abd53 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfigurationLoader.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfigurationLoader.java
@@ -37,7 +37,8 @@ public PythonConfiguration create(BuildOptions buildOptions)
return new PythonConfiguration(
pythonVersion,
pythonOptions.buildPythonZip,
- pythonOptions.buildTransitiveRunfilesTrees);
+ pythonOptions.buildTransitiveRunfilesTrees,
+ /*newPyVersionApiEnabled=*/ pythonOptions.experimentalBetterPythonVersionMixing);
}
@Override
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 efe33762c27cc5..37bb5a4d789802 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
@@ -59,45 +59,124 @@ protected void assertPythonVersionIs_UnderNewConfigs(
}
}
- private String ruleDeclWithVersionAttr(String name, String version) {
- return ruleName + "(\n"
- + " name = '" + name + "',\n"
- + " srcs = ['" + name + ".py'],\n"
- + " default_python_version = '" + version + "'\n"
- + ")\n";
+ private String join(String... lines) {
+ return String.join("\n", lines);
+ }
+
+ private String ruleDeclWithDefaultPyVersionAttr(String name, String version) {
+ return join(
+ ruleName + "(",
+ " name = '" + name + "',",
+ " srcs = ['" + name + ".py'],",
+ " default_python_version = '" + version + "',",
+ ")");
+ }
+
+ private String ruleDeclWithPyVersionAttr(String name, String version) {
+ return join(
+ ruleName + "(",
+ " name = '" + name + "',",
+ " srcs = ['" + name + ".py'],",
+ " python_version = '" + version + "'",
+ ")");
}
@Test
- public void versionAttr_UnknownValue() throws Exception {
- checkError("pkg", "foo",
+ public void oldVersionAttr_UnknownValue() throws Exception {
+ checkError(
+ "pkg",
+ "foo",
// error:
"invalid value in 'default_python_version' attribute: "
+ "has to be one of 'PY2' or 'PY3' instead of 'doesnotexist'",
// build file:
- ruleDeclWithVersionAttr("foo", "doesnotexist"));
+ ruleDeclWithDefaultPyVersionAttr("foo", "doesnotexist"));
}
@Test
- public void versionAttr_BadValue() throws Exception {
- checkError("pkg", "foo",
+ public void newVersionAttr_UnknownValue() throws Exception {
+ useConfiguration("--experimental_better_python_version_mixing=true");
+ checkError(
+ "pkg",
+ "foo",
+ // error:
+ "invalid value in 'python_version' attribute: "
+ + "has to be one of 'PY2' or 'PY3' instead of 'doesnotexist'",
+ // build file:
+ ruleDeclWithPyVersionAttr("foo", "doesnotexist"));
+ }
+
+ @Test
+ public void oldVersionAttr_BadValue() throws Exception {
+ checkError(
+ "pkg",
+ "foo",
// error:
"invalid value in 'default_python_version' attribute: "
+ "has to be one of 'PY2' or 'PY3' instead of 'PY2AND3'",
// build file:
- ruleDeclWithVersionAttr("foo", "PY2AND3"));
+ ruleDeclWithDefaultPyVersionAttr("foo", "PY2AND3"));
+ }
+
+ @Test
+ public void newVersionAttr_BadValue() throws Exception {
+ useConfiguration("--experimental_better_python_version_mixing=true");
+ checkError(
+ "pkg",
+ "foo",
+ // error:
+ "invalid value in 'python_version' attribute: "
+ + "has to be one of 'PY2' or 'PY3' instead of 'PY2AND3'",
+ // build file:
+ ruleDeclWithPyVersionAttr("foo", "PY2AND3"));
+ }
+
+ @Test
+ public void oldVersionAttr_GoodValue() throws Exception {
+ scratch.file("pkg/BUILD", ruleDeclWithDefaultPyVersionAttr("foo", "PY2"));
+ getConfiguredTarget("//pkg:foo");
+ assertNoEvents();
}
@Test
- public void versionAttr_GoodValue() throws Exception {
- scratch.file("pkg/BUILD", ruleDeclWithVersionAttr("foo", "PY2"));
+ public void newVersionAttr_GoodValue() throws Exception {
+ useConfiguration("--experimental_better_python_version_mixing=true");
+ scratch.file("pkg/BUILD", ruleDeclWithPyVersionAttr("foo", "PY2"));
getConfiguredTarget("//pkg:foo");
assertNoEvents();
}
+ @Test
+ public void cannotUseNewVersionAttrWithoutFlag() throws Exception {
+ useConfiguration("--experimental_better_python_version_mixing=false");
+ checkError(
+ "pkg",
+ "foo",
+ // error:
+ "using the 'python_version' attribute requires the "
+ + "'--experimental_better_python_version_mixing' flag",
+ // build file:
+ ruleDeclWithPyVersionAttr("foo", "PY2"));
+ }
+
+ @Test
+ public void newVersionAttrTakesPrecedenceOverOld() throws Exception {
+ scratch.file(
+ "pkg/BUILD",
+ ruleName + "(",
+ " name = 'foo',",
+ " srcs = ['foo.py'],",
+ " default_python_version = 'PY2',",
+ " python_version = 'PY3',",
+ ")");
+ assertPythonVersionIs_UnderNewConfig(
+ "//pkg:foo", PythonVersion.PY3, "--experimental_better_python_version_mixing=true");
+ }
+
@Test
public void versionAttrWorksUnderOldAndNewSemantics_WhenNotDefaultValue() throws Exception {
ensureDefaultIsPY2();
- scratch.file("pkg/BUILD", ruleDeclWithVersionAttr("foo", "PY3"));
+ scratch.file("pkg/BUILD", ruleDeclWithDefaultPyVersionAttr("foo", "PY3"));
assertPythonVersionIs_UnderNewConfigs(
"//pkg:foo",
@@ -109,7 +188,7 @@ public void versionAttrWorksUnderOldAndNewSemantics_WhenNotDefaultValue() throws
@Test
public void versionAttrWorksUnderOldAndNewSemantics_WhenSameAsDefaultValue() throws Exception {
ensureDefaultIsPY2();
- scratch.file("pkg/BUILD", ruleDeclWithVersionAttr("foo", "PY2"));
+ scratch.file("pkg/BUILD", ruleDeclWithDefaultPyVersionAttr("foo", "PY2"));
assertPythonVersionIs_UnderNewConfigs(
"//pkg:foo",
@@ -121,7 +200,7 @@ public void versionAttrWorksUnderOldAndNewSemantics_WhenSameAsDefaultValue() thr
@Test
public void flagTakesPrecedenceUnderOldSemantics_NonDefaultValue() throws Exception {
ensureDefaultIsPY2();
- scratch.file("pkg/BUILD", ruleDeclWithVersionAttr("foo", "PY2"));
+ scratch.file("pkg/BUILD", ruleDeclWithDefaultPyVersionAttr("foo", "PY2"));
assertPythonVersionIs_UnderNewConfig(
"//pkg:foo",
PythonVersion.PY3,
@@ -132,7 +211,7 @@ public void flagTakesPrecedenceUnderOldSemantics_NonDefaultValue() throws Except
@Test
public void flagTakesPrecedenceUnderOldSemantics_DefaultValue() throws Exception {
ensureDefaultIsPY2();
- scratch.file("pkg/BUILD", ruleDeclWithVersionAttr("foo", "PY3"));
+ scratch.file("pkg/BUILD", ruleDeclWithDefaultPyVersionAttr("foo", "PY3"));
assertPythonVersionIs_UnderNewConfig(
"//pkg:foo",
PythonVersion.PY2,
@@ -143,7 +222,7 @@ public void flagTakesPrecedenceUnderOldSemantics_DefaultValue() throws Exception
@Test
public void versionAttrTakesPrecedenceUnderNewSemantics_NonDefaultValue() throws Exception {
ensureDefaultIsPY2();
- scratch.file("pkg/BUILD", ruleDeclWithVersionAttr("foo", "PY3"));
+ scratch.file("pkg/BUILD", ruleDeclWithDefaultPyVersionAttr("foo", "PY3"));
// Test against both flags.
assertPythonVersionIs_UnderNewConfigs(
@@ -156,7 +235,7 @@ public void versionAttrTakesPrecedenceUnderNewSemantics_NonDefaultValue() throws
@Test
public void versionAttrTakesPrecedenceUnderNewSemantics_DefaultValue() throws Exception {
ensureDefaultIsPY2();
- scratch.file("pkg/BUILD", ruleDeclWithVersionAttr("foo", "PY2"));
+ scratch.file("pkg/BUILD", ruleDeclWithDefaultPyVersionAttr("foo", "PY2"));
// Test against both flags.
assertPythonVersionIs_UnderNewConfigs(
@@ -170,8 +249,8 @@ public void versionAttrTakesPrecedenceUnderNewSemantics_DefaultValue() throws Ex
public void canBuildWithDifferentVersionAttrs_UnderOldAndNewSemantics() throws Exception {
scratch.file(
"pkg/BUILD",
- ruleDeclWithVersionAttr("foo_v2", "PY2"),
- ruleDeclWithVersionAttr("foo_v3", "PY3"));
+ ruleDeclWithDefaultPyVersionAttr("foo_v2", "PY2"),
+ ruleDeclWithDefaultPyVersionAttr("foo_v3", "PY3"));
assertPythonVersionIs_UnderNewConfigs(
"//pkg:foo_v2",
@@ -191,8 +270,8 @@ public void canBuildWithDifferentVersionAttrs_UnderOldSemantics_FlagSetToDefault
ensureDefaultIsPY2();
scratch.file(
"pkg/BUILD",
- ruleDeclWithVersionAttr("foo_v2", "PY2"),
- ruleDeclWithVersionAttr("foo_v3", "PY3"));
+ ruleDeclWithDefaultPyVersionAttr("foo_v2", "PY2"),
+ ruleDeclWithDefaultPyVersionAttr("foo_v3", "PY3"));
assertPythonVersionIs_UnderNewConfig("//pkg:foo_v2", PythonVersion.PY2, "--force_python=PY2");
assertPythonVersionIs_UnderNewConfig("//pkg:foo_v3", PythonVersion.PY2, "--force_python=PY2");
@@ -204,8 +283,8 @@ public void canBuildWithDifferentVersionAttrs_UnderOldSemantics_FlagSetToNonDefa
ensureDefaultIsPY2();
scratch.file(
"pkg/BUILD",
- ruleDeclWithVersionAttr("foo_v2", "PY2"),
- ruleDeclWithVersionAttr("foo_v3", "PY3"));
+ ruleDeclWithDefaultPyVersionAttr("foo_v2", "PY2"),
+ ruleDeclWithDefaultPyVersionAttr("foo_v3", "PY3"));
assertPythonVersionIs_UnderNewConfig("//pkg:foo_v2", PythonVersion.PY3, "--force_python=PY3");
assertPythonVersionIs_UnderNewConfig("//pkg:foo_v3", PythonVersion.PY3, "--force_python=PY3");
python_version
are supplied,
+ default_python_version
will be ignored.
*/
.add(
- attr("default_python_version", STRING)
+ attr(PyCommon.DEFAULT_PYTHON_VERSION_ATTRIBUTE, STRING)
.value(PythonVersion.DEFAULT_TARGET_VALUE.toString())
.allowedValues(new AllowedValueSet(PythonVersion.TARGET_STRINGS))
.nonconfigurable(
- "read by PythonUtils.getNewPythonVersion, which doesn't have access"
- + " to configuration keys"))
+ "read by PyRuleClasses.PYTHON_VERSION_TRANSITION, which doesn't have access"
+ + " to the configuration"))
+ /*
+ An experimental replacement for default_python_version
. Only available when
+ --experimental_better_python_version_mixing
is enabled. If both this and
+ default_python_version
are supplied, the latter will be ignored.
+ */
+ .add(
+ attr(PyCommon.PYTHON_VERSION_ATTRIBUTE, STRING)
+ .value(PythonVersion.DEFAULT_TARGET_VALUE.toString())
+ .allowedValues(new AllowedValueSet(PythonVersion.TARGET_STRINGS))
+ .nonconfigurable(
+ "read by PyRuleClasses.PYTHON_VERSION_TRANSITION, which doesn't have access"
+ + " to the configuration"))
/*
The list of source files that are processed to create the target.
This includes all your checked-in code and any
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 96183ea8c8d15b..4723f58d7a83c4 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.DEFAULT_PYTHON_VERSION_TRANSITION)
+ .cfg(PyRuleClasses.PYTHON_VERSION_TRANSITION)
.add(
attr("$zipper", LABEL)
.cfg(HostTransition.INSTANCE)
diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyBinary.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyBinary.java
index d3a7cecea1f99a..eb6b8138aac7c7 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/python/PyBinary.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/python/PyBinary.java
@@ -44,7 +44,6 @@ public abstract class PyBinary implements RuleConfiguredTargetFactory {
public ConfiguredTarget create(RuleContext ruleContext)
throws InterruptedException, RuleErrorException, ActionConflictException {
PyCommon common = new PyCommon(ruleContext);
- common.initCommon(common.getDefaultPythonVersion());
RuleConfiguredTargetBuilder builder = init(ruleContext, createSemantics(), common);
if (builder == null) {
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 ea30b597d7566b..5e5301d18190f3 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
@@ -39,6 +39,7 @@
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.collect.nestedset.Order;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
+import com.google.devtools.build.lib.packages.AttributeMap;
import com.google.devtools.build.lib.packages.BuildType;
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.packages.StructImpl;
@@ -70,6 +71,9 @@ public final class PyCommon {
public static final String IS_USING_SHARED_LIBRARY = "uses_shared_libraries";
public static final String IMPORTS = "imports";
+ public static final String DEFAULT_PYTHON_VERSION_ATTRIBUTE = "default_python_version";
+ public static final String PYTHON_VERSION_ATTRIBUTE = "python_version";
+
private static final LocalMetadataCollector METADATA_COLLECTOR = new LocalMetadataCollector() {
@Override
public void collectMetadataArtifacts(Iterable