Overrides previous instances of the exact same subclass of {@code FragmentOptions}. + * + *
The options get preprocessed with {@link FragmentOptions#getNormalized}.
*/
public Motivation: Sometimes a fragment's physical option values, as set by the options parser, do
+ * not correspond to their logical interpretation. For example, an option may need custom code to
+ * determine its logical default value at runtime, but it's limited to a single hard-coded
+ * physical default value in the {@link Option#defaultValue} annotation field. If two instances of
+ * the fragment have the same logical value but different physical values, a redundant
+ * configuration can be created, which results in an action conflict (particularly for unshareable
+ * actions; see #7808).
+ *
+ * To solve this, we can distinguish between "normalized" and "non-normalized" instances of a
+ * fragment type, and preserve the invariant that configured targets only ever see normalized
+ * instances. This requires that 1) the top-level configuration is normalized, and 2) all
+ * transitions preserve normalization. Step 1) is ensured by {@link BuildOptions} calling this
+ * method. Step 2) is the responsibility of each transition implementation.
+ */
+ public FragmentOptions getNormalized() {
+ return clone();
+ }
+
/** Tracks limitations on referring to an option in a {@code config_setting}. */
// TODO(bazel-team): There will likely also be a need to customize whether or not an option is
// visible to users for setting on the command line (or perhaps even in a test of a Starlark
diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java b/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java
index e2c5ef4fb2b8bb..481eac422837f0 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java
@@ -313,17 +313,6 @@ public PythonVersion getPythonVersion() {
* transition the version to the hard-coded default value. Under these constraints, there is only
* one transition possible, from null to the non-default value, and it is never a no-op.
*
- * Previously this method also allowed transitioning under the new semantics in cases where the
- * transition would have no impact on {@link #getPythonVersion} but would bring {@link
- * #forcePython} into agreement with the actual version. The benefit of doing this was supposed to
- * be that {@code select()}ing on {@code "force_python"} would give the correct result more often,
- * even though it's still incorrect in general. However, this ended up causing more harm than
- * good, because this type of transition does not change the output root and therefore caused
- * action conflicts for unshareable actions (mainly C++ actions); see #7655. The resolution is
- * that users should just migrate their {@code select()}s to the {@code
- * //tools/python:python_version} target in the tools repository, as is required when {@code
- * --incompatible_remove_old_python_version_api} is enabled.
- *
* @throws IllegalArgumentException if {@code version} is not {@code PY2} or {@code PY3}
*/
public boolean canTransitionPythonVersion(PythonVersion version) {
@@ -357,9 +346,13 @@ public boolean canTransitionPythonVersion(PythonVersion version) {
public void setPythonVersion(PythonVersion version) {
Preconditions.checkArgument(version.isTargetValue());
this.pythonVersion = version;
- // Update forcePython, but if the flag to remove the old API is enabled, no one will be able
- // to tell anyway.
- this.forcePython = version;
+ // If the old version API is enabled, update forcePython for consistency. If the old API is
+ // disabled, don't update it because 1) no one can read it anyway, and 2) updating it during
+ // normalization would cause analysis-time validation of the flag to spuriously fail (it'd think
+ // the user set the flag).
+ if (!incompatibleRemoveOldPythonVersionApi) {
+ this.forcePython = version;
+ }
}
@Override
@@ -378,4 +371,16 @@ public FragmentOptions getHost() {
hostPythonOptions.incompatibleUsePythonToolchains = incompatibleUsePythonToolchains;
return hostPythonOptions;
}
+
+ @Override
+ public FragmentOptions getNormalized() {
+ // Under the new version semantics, we want to ensure that options with "null" physical default
+ // values are normalized, to avoid #7808. We don't want to normalize with the old version
+ // semantics because that breaks backwards compatibility (--force_python would always be on).
+ PythonOptions newOptions = (PythonOptions) clone();
+ if (incompatibleAllowPythonVersionTransitions) {
+ newOptions.setPythonVersion(newOptions.getPythonVersion());
+ }
+ return newOptions;
+ }
}
diff --git a/src/test/java/com/google/devtools/build/lib/rules/python/PythonConfigurationTest.java b/src/test/java/com/google/devtools/build/lib/rules/python/PythonConfigurationTest.java
index ecb94703bb9e03..ab93932a536804 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/python/PythonConfigurationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/python/PythonConfigurationTest.java
@@ -190,7 +190,7 @@ public void canTransitionPythonVersion_NewApi_NoEvenWhenForcePythonDisagrees() t
}
@Test
- public void setPythonVersion() throws Exception {
+ public void setPythonVersion_OldApiEnabled() throws Exception {
PythonOptions opts =
parsePythonOptions(
"--incompatible_remove_old_python_version_api=false",
@@ -201,6 +201,16 @@ public void setPythonVersion() throws Exception {
assertThat(opts.pythonVersion).isEqualTo(PythonVersion.PY3);
}
+ @Test
+ public void setPythonVersion_OldApiDisabled() throws Exception {
+ PythonOptions opts =
+ parsePythonOptions(
+ "--incompatible_remove_old_python_version_api=true", "--python_version=PY2");
+ opts.setPythonVersion(PythonVersion.PY3);
+ assertThat(opts.forcePython).isNull();
+ assertThat(opts.pythonVersion).isEqualTo(PythonVersion.PY3);
+ }
+
@Test
public void getHost_CopiesMostValues() throws Exception {
PythonOptions opts =
@@ -260,4 +270,20 @@ public void getHost_Py3IsDefaultFlagChangesHost() throws Exception {
PythonOptions hostOpts = (PythonOptions) opts.getHost();
assertThat(hostOpts.getPythonVersion()).isEqualTo(PythonVersion.PY3);
}
+
+ @Test
+ public void getNormalized_OldSemantics() throws Exception {
+ PythonOptions opts =
+ parsePythonOptions("--incompatible_allow_python_version_transitions=false");
+ PythonOptions normalizedOpts = (PythonOptions) opts.getNormalized();
+ assertThat(normalizedOpts.pythonVersion).isNull();
+ }
+
+ @Test
+ public void getNormalized_NewSemantics() throws Exception {
+ ensureDefaultIsPY2();
+ PythonOptions opts = parsePythonOptions("--incompatible_allow_python_version_transitions=true");
+ PythonOptions normalizedOpts = (PythonOptions) opts.getNormalized();
+ assertThat(normalizedOpts.pythonVersion).isEqualTo(PythonVersion.PY2);
+ }
}
diff --git a/src/test/shell/integration/python_stub_test.sh b/src/test/shell/integration/python_stub_test.sh
index f1688d71b202b4..7ecf25432dbcec 100755
--- a/src/test/shell/integration/python_stub_test.sh
+++ b/src/test/shell/integration/python_stub_test.sh
@@ -136,4 +136,72 @@ EOF
&> $TEST_log || fail "bazel build failed"
}
+# Regression test for #7808. We want to ensure that changing the Python version
+# to a value different from the top-level configuration, and then changing it
+# back again, is able to reuse the top-level configuration.
+function test_no_action_conflicts_from_version_transition() {
+ mkdir -p test
+
+ # To repro, we need to build a C++ target in two different ways in the same
+ # build:
+ #
+ # 1) At the top-level, and without any explicit flags passed to control the
+ # Python version, because the behavior under test involves the internal
+ # null default value of said flags.
+ #
+ # 2) As a dependency of a target that transitions the Python version to the
+ # same value as in the top-level configuration.
+ #
+ # We need to use two different Python targets, to transition the version
+ # *away* from the top-level default and then *back* again. Furthermore,
+ # because (as of the writing of this test) the default Python version is in
+ # the process of being migrated from PY2 to PY3, we'll future-proof this test
+ # by using two separate paths that have the versions inverted.
+ #
+ # We use C++ for the repro because it has unshareable actions, so we'll know
+ # if the top-level config isn't being reused.
+
+ cat > test/BUILD << EOF
+cc_binary(
+ name = "cc",
+ srcs = ["cc.cc"],
+)
+
+py_binary(
+ name = "path_A_inner",
+ srcs = ["path_A_inner.py"],
+ data = [":cc"],
+ python_version = "PY2",
+)
+
+py_binary(
+ name = "path_A_outer",
+ srcs = [":path_A_outer.py"],
+ data = [":path_A_inner"],
+ python_version = "PY3",
+)
+
+py_binary(
+ name = "path_B_inner",
+ srcs = [":path_B_inner.py"],
+ data = [":cc"],
+ python_version = "PY3",
+)
+
+py_binary(
+ name = "path_B_outer",
+ srcs = [":path_B_outer.py"],
+ data = [":path_B_inner"],
+ python_version = "PY2",
+)
+EOF
+
+ # Build cc at the top level, along with the outer halves of both paths to cc.
+ # Make sure to use the new version transition semantics.
+ bazel build --nobuild //test:cc //test:path_A_outer //test:path_B_outer \
+ --incompatible_remove_old_python_version_api=true \
+ --incompatible_allow_python_version_transitions=true \
+ &> $TEST_log || fail "bazel run failed"
+}
+
run_suite "Tests for the Python rules without Python execution"