Skip to content

Commit

Permalink
Do not add dummy py version transitions
Browse files Browse the repository at this point in the history
These transitions were intended to bring --force_python in sync with the actual Python version to make select() less broken. This ended up causing more problems than it fixed (#7655). It's not really needed anyway since the select() issue becomes moot once --incompatible_remove_old_python_version_api is enabled.

RELNOTES: None
PiperOrigin-RevId: 238281325
  • Loading branch information
brandjon authored and copybara-github committed Mar 13, 2019
1 parent 8753eca commit 09c6b7a
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 13 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -305,26 +305,31 @@ public PythonVersion getPythonVersion() {
*
* <p>Under the new semantics ({@link #incompatibleAllowPythonVersionTransitions} is true),
* version transitions are always allowed, so this essentially returns whether the new version is
* different from the existing one. However, to improve compatibility for unmigrated {@code
* select()}s that depend on {@code "force_python"}, if the old API is still enabled then
* transitioning is still done whenever {@link #forcePython} is not in agreement with the
* requested version, even if {@link #getPythonVersion}'s value would be unaffected.
* different from the existing one.
*
* <p>Under the old semantics ({@link #incompatibleAllowPythonVersionTransitions} is false),
* version transitions are not allowed once the version has already been set ({@link #forcePython}
* or {@link #pythonVersion} is non-null). Due to a historical bug, it is also not allowed to
* 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.
*
* <p>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) {
Preconditions.checkArgument(version.isTargetValue());
if (incompatibleAllowPythonVersionTransitions) {
boolean currentVersionNeedsUpdating = !version.equals(getPythonVersion());
boolean forcePythonNeedsUpdating =
!incompatibleRemoveOldPythonVersionApi && !version.equals(forcePython);
return currentVersionNeedsUpdating || forcePythonNeedsUpdating;
return !version.equals(getPythonVersion());
} else {
boolean currentlyUnset = forcePython == null && pythonVersion == null;
boolean transitioningToNonDefault = !version.equals(getDefaultPythonVersion());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -177,17 +177,16 @@ public void canTransitionPythonVersion_NewSemantics_NoBecauseSameAsCurrent() thr
}

@Test
public void canTransitionPythonVersion_NewApi_YesBecauseForcePythonDisagrees() throws Exception {
public void canTransitionPythonVersion_NewApi_NoEvenWhenForcePythonDisagrees() throws Exception {
PythonOptions opts =
parsePythonOptions(
"--incompatible_allow_python_version_transitions=true",
"--incompatible_remove_old_python_version_api=false",
// Test that even though getPythonVersion() would not be affected by a transition (it is
// PY3 before and after), the transition is still considered necessary because
// --force_python's value needs to be brought in sync.
// Test that even though --force_python's value isn't in sync, we don't transition
// because getPythonVersion() would be unaffected by the transition.
"--force_python=PY2",
"--python_version=PY3");
assertThat(opts.canTransitionPythonVersion(PythonVersion.PY3)).isTrue();
assertThat(opts.canTransitionPythonVersion(PythonVersion.PY3)).isFalse();
}

@Test
Expand Down

0 comments on commit 09c6b7a

Please sign in to comment.