Skip to content

Commit

Permalink
Turn deprecated --force_python into a no-op
Browse files Browse the repository at this point in the history
The flag now behaves as if it is always unset. It will be removed entirely in a separate CL. In the meantime, it is still disallowed to select() on the flag.

Work toward #7797.

RELNOTES: None
PiperOrigin-RevId: 302665836
  • Loading branch information
brandjon authored and copybara-github committed Mar 24, 2020
1 parent 0a60a1b commit f237219
Show file tree
Hide file tree
Showing 6 changed files with 30 additions and 145 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,7 @@ public class PythonConfiguration extends BuildConfiguration.Fragment implements
* Returns the Python version to use.
*
* <p>Specified using either the {@code --python_version} flag and {@code python_version} rule
* attribute (new API), or the {@code --force_python} flag and {@code default_python_version} rule
* attribute (old API).
* attribute (new API), or the {@code default_python_version} rule attribute (old API).
*/
public PythonVersion getPythonVersion() {
return version;
Expand Down Expand Up @@ -135,11 +134,6 @@ public String getOutputDirectoryName() {
@Override
public void reportInvalidOptions(EventHandler reporter, BuildOptions buildOptions) {
PythonOptions opts = buildOptions.get(PythonOptions.class);
if (opts.forcePython != null && opts.incompatibleRemoveOldPythonVersionApi) {
reporter.handle(
Event.error(
"`--force_python` is disabled by `--incompatible_remove_old_python_version_api`"));
}
if (opts.incompatiblePy3IsDefault && !opts.incompatibleAllowPythonVersionTransitions) {
reporter.handle(
Event.error(
Expand Down Expand Up @@ -168,10 +162,7 @@ public boolean buildTransitiveRunfilesTrees() {
return buildTransitiveRunfilesTrees;
}

/**
* Returns whether use of {@code --force_python} flag and {@code default_python_version} attribute
* is allowed.
*/
/** Returns whether use of the {@code default_python_version} attribute is allowed. */
public boolean oldPyVersionApiAllowed() {
return oldPyVersionApiAllowed;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ public String getTypeDescription() {
OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES
},
help =
"If true, disables use of the `--force_python` flag and the `default_python_version` "
"If true, disables use of the `default_python_version` "
+ "attribute for `py_binary` and `py_test`. Use the `--python_version` flag and "
+ "`python_version` attribute instead, which have exactly the same meaning. This "
+ "flag also disables `select()`-ing over `--host_force_python`.")
Expand Down Expand Up @@ -169,25 +169,17 @@ public String getTypeDescription() {
OptionsParser.getOptionDefinitionByName(PythonOptions.class, "python_version");

/**
* This field should be either null (unset), {@code PY2}, or {@code PY3}. Other {@code
* PythonVersion} values do not represent distinct Python versions and are not allowed.
*
* <p>This flag is not accessible to the user when {@link #incompatibleRemoveOldPythonVersionApi}
* is true.
* Deprecated machinery for setting the Python version; will be removed soon.
*
* <p>Native rule logic should call {@link #getPythonVersion} / {@link #setPythonVersion} instead
* of accessing this option directly. BUILD/.bzl code should {@code select()} on {@code <tools
* repo>//tools/python:python_version} rather than on this option directly.
* <p>Not in GraveyardOptions because we still want to prohibit users from select()ing on it.
*/
@Option(
name = "force_python",
defaultValue = "null",
converter = TargetPythonVersionConverter.class,
documentationCategory = OptionDocumentationCategory.OUTPUT_PARAMETERS,
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS, OptionEffectTag.AFFECTS_OUTPUTS},
help =
"Deprecated alias for `--python_version`. Disabled by "
+ "`--incompatible_remove_old_python_version_api`.")
help = "No-op, will be removed soon.")
public PythonVersion forcePython;

private static final OptionDefinition FORCE_PYTHON_DEFINITION =
Expand Down Expand Up @@ -322,14 +314,12 @@ public PythonVersion getDefaultPythonVersion() {
/**
* Returns the Python major version ({@code PY2} or {@code PY3}) that targets should be built for.
*
* <p>The version is taken as the value of {@code --python_version} if not null, otherwise {@code
* --force_python} if not null, otherwise {@link #getDefaultPythonVersion}.
* <p>The version is taken as the value of {@code --python_version} if not null, otherwise {@link
* #getDefaultPythonVersion}.
*/
public PythonVersion getPythonVersion() {
if (pythonVersion != null) {
return pythonVersion;
} else if (forcePython != null) {
return forcePython;
} else {
return getDefaultPythonVersion();
}
Expand All @@ -343,10 +333,10 @@ public PythonVersion getPythonVersion() {
* 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.
* version transitions are not allowed once the version has already been set ({@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.
*
* @throws IllegalArgumentException if {@code version} is not {@code PY2} or {@code PY3}
*/
Expand All @@ -355,7 +345,7 @@ public boolean canTransitionPythonVersion(PythonVersion version) {
if (incompatibleAllowPythonVersionTransitions) {
return !version.equals(getPythonVersion());
} else {
boolean currentlyUnset = forcePython == null && pythonVersion == null;
boolean currentlyUnset = pythonVersion == null;
boolean transitioningToNonDefault = !version.equals(getDefaultPythonVersion());
return currentlyUnset && transitioningToNonDefault;
}
Expand All @@ -372,22 +362,11 @@ public boolean canTransitionPythonVersion(PythonVersion version) {
* <p>If the old semantics are in effect ({@link #incompatibleAllowPythonVersionTransitions} is
* false), after this method is called {@link #canTransitionPythonVersion} will return false.
*
* <p>To help avoid breaking old-API {@code select()} expressions that check the value of {@code
* "force_python"}, both the old and new flags are updated even though {@code --python_version}
* takes precedence over {@code --force_python}.
*
* @throws IllegalArgumentException if {@code version} is not {@code PY2} or {@code PY3}
*/
public void setPythonVersion(PythonVersion version) {
Preconditions.checkArgument(version.isTargetValue());
this.pythonVersion = 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
Expand Down Expand Up @@ -416,7 +395,7 @@ public FragmentOptions getHost() {
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).
// semantics because that breaks backwards compatibility.
PythonOptions newOptions = (PythonOptions) clone();
if (incompatibleAllowPythonVersionTransitions) {
newOptions.setPythonVersion(newOptions.getPythonVersion());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ public void versionIs2IfUnspecified() throws Exception {

@Test
public void versionIs3IfForcedByFlagUnderOldSemantics() throws Exception {
// Under the old version semantics, --force_python takes precedence over the rule's own
// Under the old version semantics, --python_version takes precedence over the rule's own
// default_python_version attribute, so this test case applies equally well to py_library,
// py_binary, and py_test. Under the new semantics the rule attribute takes precedence, so this
// would only make sense for py_library; see PyLibraryConfiguredTargetTest for the analogous
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -359,37 +359,23 @@ public void versionAttrTakesPrecedenceUnderNewSemantics_NonDefaultValue() throws
assumesDefaultIsPY2();
scratch.file("pkg/BUILD", ruleDeclWithPyVersionAttr("foo", "PY3"));

// Test against both flags.
assertPythonVersionIs_UnderNewConfigs(
assertPythonVersionIs_UnderNewConfig(
"//pkg:foo",
PythonVersion.PY3,
new String[] {
"--incompatible_allow_python_version_transitions=true",
"--incompatible_remove_old_python_version_api=false",
"--force_python=PY2"
},
new String[] {
"--incompatible_allow_python_version_transitions=true", "--python_version=PY2"
});
"--incompatible_allow_python_version_transitions=true",
"--python_version=PY2");
}

@Test
public void versionAttrTakesPrecedenceUnderNewSemantics_DefaultValue() throws Exception {
assumesDefaultIsPY2();
scratch.file("pkg/BUILD", ruleDeclWithPyVersionAttr("foo", "PY2"));

// Test against both flags.
assertPythonVersionIs_UnderNewConfigs(
assertPythonVersionIs_UnderNewConfig(
"//pkg:foo",
PythonVersion.PY2,
new String[] {
"--incompatible_allow_python_version_transitions=true",
"--incompatible_remove_old_python_version_api=false",
"--force_python=PY3"
},
new String[] {
"--incompatible_allow_python_version_transitions=true", "--python_version=PY3"
});
"--incompatible_allow_python_version_transitions=true",
"--python_version=PY3");
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,28 +38,17 @@ private PythonOptions parsePythonOptions(String... cmdline) throws Exception {
@Test
public void invalidTargetPythonValue_NotATargetValue() {
OptionsParsingException expected =
assertThrows(OptionsParsingException.class, () -> create("--force_python=PY2AND3"));
assertThrows(OptionsParsingException.class, () -> create("--python_version=PY2AND3"));
assertThat(expected).hasMessageThat().contains("Not a valid Python major version");
}

@Test
public void invalidTargetPythonValue_UnknownValue() {
OptionsParsingException expected =
assertThrows(
OptionsParsingException.class,
() -> create("--force_python=BEETLEJUICE"));
assertThrows(OptionsParsingException.class, () -> create("--python_version=BEETLEJUICE"));
assertThat(expected).hasMessageThat().contains("Not a valid Python major version");
}

@Test
public void oldVersionFlagGatedByIncompatibleFlag() throws Exception {
create("--incompatible_remove_old_python_version_api=false", "--force_python=PY2");
checkError(
"`--force_python` is disabled by `--incompatible_remove_old_python_version_api`",
"--incompatible_remove_old_python_version_api=true",
"--force_python=PY2");
}

@Test
public void py3IsDefaultFlagRequiresNewSemanticsFlag() throws Exception {
checkError(
Expand Down Expand Up @@ -101,28 +90,6 @@ public void getPythonVersion_FallBackOnDefaultPythonVersion() throws Exception {
assertThat(py3Opts.getPythonVersion()).isEqualTo(PythonVersion.PY3);
}

@Test
public void getPythonVersion_NewFlagTakesPrecedence() throws Exception {
assumesDefaultIsPY2();
// --force_python is superseded by --python_version.
PythonOptions opts =
parsePythonOptions(
"--incompatible_remove_old_python_version_api=false",
"--force_python=PY2",
"--python_version=PY3");
assertThat(opts.getPythonVersion()).isEqualTo(PythonVersion.PY3);
}

@Test
public void getPythonVersion_FallBackOnOldFlag() throws Exception {
assumesDefaultIsPY2();
// --force_python is used because --python_version is absent.
PythonOptions opts =
parsePythonOptions(
"--incompatible_remove_old_python_version_api=false", "--force_python=PY3");
assertThat(opts.getPythonVersion()).isEqualTo(PythonVersion.PY3);
}

@Test
public void canTransitionPythonVersion_OldSemantics_Yes() throws Exception {
assumesDefaultIsPY2();
Expand All @@ -134,16 +101,10 @@ public void canTransitionPythonVersion_OldSemantics_Yes() throws Exception {
@Test
public void canTransitionPythonVersion_OldSemantics_NoBecauseAlreadySet() throws Exception {
assumesDefaultIsPY2();
PythonOptions optsWithOldFlag =
parsePythonOptions(
"--incompatible_allow_python_version_transitions=false",
"--incompatible_remove_old_python_version_api=false",
"--force_python=PY2");
PythonOptions optsWithNewFlag =
PythonOptions opts =
parsePythonOptions(
"--incompatible_allow_python_version_transitions=false", "--python_version=PY2");
assertThat(optsWithOldFlag.canTransitionPythonVersion(PythonVersion.PY3)).isFalse();
assertThat(optsWithNewFlag.canTransitionPythonVersion(PythonVersion.PY3)).isFalse();
assertThat(opts.canTransitionPythonVersion(PythonVersion.PY3)).isFalse();
}

@Test
Expand All @@ -165,26 +126,10 @@ public void canTransitionPythonVersion_NewSemantics_Yes() throws Exception {

@Test
public void canTransitionPythonVersion_NewSemantics_NoBecauseSameAsCurrent() throws Exception {
PythonOptions opts =
parsePythonOptions(
"--incompatible_allow_python_version_transitions=true",
// Set --force_python too, or else we fall into the "make --force_python consistent"
// case.
"--incompatible_remove_old_python_version_api=false",
"--force_python=PY3",
"--python_version=PY3");
assertThat(opts.canTransitionPythonVersion(PythonVersion.PY3)).isFalse();
}

@Test
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 --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)).isFalse();
}
Expand All @@ -194,10 +139,8 @@ public void setPythonVersion_OldApiEnabled() throws Exception {
PythonOptions opts =
parsePythonOptions(
"--incompatible_remove_old_python_version_api=false",
"--force_python=PY2",
"--python_version=PY2");
opts.setPythonVersion(PythonVersion.PY3);
assertThat(opts.forcePython).isEqualTo(PythonVersion.PY3);
assertThat(opts.pythonVersion).isEqualTo(PythonVersion.PY3);
}

Expand All @@ -207,7 +150,6 @@ public void setPythonVersion_OldApiDisabled() throws Exception {
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);
}

Expand Down Expand Up @@ -235,11 +177,6 @@ public void getHost_CopiesMostValues() throws Exception {
@Test
public void getHost_AppliesHostForcePython() throws Exception {
assumesDefaultIsPY2();
PythonOptions optsWithForcePythonFlag =
parsePythonOptions(
"--incompatible_remove_old_python_version_api=false",
"--force_python=PY2",
"--host_force_python=PY3");
PythonOptions optsWithPythonVersionFlag =
parsePythonOptions("--python_version=PY2", "--host_force_python=PY3");
PythonOptions optsWithPy3IsDefaultFlag =
Expand All @@ -250,11 +187,9 @@ public void getHost_AppliesHostForcePython() throws Exception {
// It's more interesting to set the incompatible flag true and force host to PY2, than
// it is to set the flag false and force host to PY3.
"--host_force_python=PY2");
PythonOptions hostOptsWithForcePythonFlag = (PythonOptions) optsWithForcePythonFlag.getHost();
PythonOptions hostOptsWithPythonVersionFlag =
(PythonOptions) optsWithPythonVersionFlag.getHost();
PythonOptions hostOptsWithPy3IsDefaultFlag = (PythonOptions) optsWithPy3IsDefaultFlag.getHost();
assertThat(hostOptsWithForcePythonFlag.getPythonVersion()).isEqualTo(PythonVersion.PY3);
assertThat(hostOptsWithPythonVersionFlag.getPythonVersion()).isEqualTo(PythonVersion.PY3);
assertThat(hostOptsWithPy3IsDefaultFlag.getPythonVersion()).isEqualTo(PythonVersion.PY2);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,8 @@ public void cannotSelectOnNativePythonVersionFlag() throws Exception {
public void canSelectOnForcePythonFlagsUnderOldApi() throws Exception {
// For backwards compatibility purposes, select()-ing on --force_python and --host_force_python
// is allowed while the old API is still enabled.
// TODO(brandjon): Although --force_python is a no-op, this test is still present because the
// behavior belongs to the remove-old-api flag. Delete the test when we no-op that flag too.
useConfiguration("--incompatible_remove_old_python_version_api=false");
scratch.file("fp/BUILD", makeFooThatSelectsOnFlag("force_python", "PY2"));
scratch.file("hfp/BUILD", makeFooThatSelectsOnFlag("host_force_python", "PY2"));
Expand Down Expand Up @@ -119,7 +121,7 @@ public void selectOnPythonVersionTarget() throws Exception {
" }),",
")");

// Neither --python_version nor --force_python, use default value.
// No --python_version, use default value.
doTestSelectOnPythonVersionTarget(py2, "--incompatible_py3_is_default=false");
doTestSelectOnPythonVersionTarget(
py3,
Expand All @@ -128,17 +130,9 @@ public void selectOnPythonVersionTarget() throws Exception {
// enabled before we're allowed to set the default to PY3.
"--incompatible_allow_python_version_transitions=true");

// No --python_version, trust --force_python.
doTestSelectOnPythonVersionTarget(py2, "--force_python=PY2");
doTestSelectOnPythonVersionTarget(py3, "--force_python=PY3");

// --python_version overrides --force_python.
// --python_version is given, use it.
doTestSelectOnPythonVersionTarget(py2, "--python_version=PY2");
doTestSelectOnPythonVersionTarget(py2, "--python_version=PY2", "--force_python=PY2");
doTestSelectOnPythonVersionTarget(py2, "--python_version=PY2", "--force_python=PY3");
doTestSelectOnPythonVersionTarget(py3, "--python_version=PY3");
doTestSelectOnPythonVersionTarget(py3, "--python_version=PY3", "--force_python=PY2");
doTestSelectOnPythonVersionTarget(py3, "--python_version=PY3", "--force_python=PY3");
}

private void doTestSelectOnPythonVersionTarget(Artifact expected, String... flags)
Expand Down

0 comments on commit f237219

Please sign in to comment.