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 c5e8d936cce61a..9acc4cf49ceea0 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 @@ -90,8 +90,7 @@ public class PythonConfiguration extends BuildConfiguration.Fragment implements * Returns the Python version to use. * *

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; @@ -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( @@ -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; } 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 aca45d8490ed53..44b835c4712557 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 @@ -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`.") @@ -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. - * - *

This flag is not accessible to the user when {@link #incompatibleRemoveOldPythonVersionApi} - * is true. + * Deprecated machinery for setting the Python version; will be removed soon. * - *

Native rule logic should call {@link #getPythonVersion} / {@link #setPythonVersion} instead - * of accessing this option directly. BUILD/.bzl code should {@code select()} on {@code //tools/python:python_version} rather than on this option directly. + *

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 = @@ -322,14 +314,12 @@ public PythonVersion getDefaultPythonVersion() { /** * Returns the Python major version ({@code PY2} or {@code PY3}) that targets should be built for. * - *

The version is taken as the value of {@code --python_version} if not null, otherwise {@code - * --force_python} if not null, otherwise {@link #getDefaultPythonVersion}. + *

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(); } @@ -343,10 +333,10 @@ public PythonVersion getPythonVersion() { * different from the existing one. * *

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} */ @@ -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; } @@ -372,22 +362,11 @@ public boolean canTransitionPythonVersion(PythonVersion version) { *

If the old semantics are in effect ({@link #incompatibleAllowPythonVersionTransitions} is * false), after this method is called {@link #canTransitionPythonVersion} will return false. * - *

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 @@ -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()); diff --git a/src/test/java/com/google/devtools/build/lib/rules/python/PyBaseConfiguredTargetTestBase.java b/src/test/java/com/google/devtools/build/lib/rules/python/PyBaseConfiguredTargetTestBase.java index c7ba4da139e132..d3767f6cba1441 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/python/PyBaseConfiguredTargetTestBase.java +++ b/src/test/java/com/google/devtools/build/lib/rules/python/PyBaseConfiguredTargetTestBase.java @@ -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 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 a5d30925d170d5..5cd8e8b9ca8e53 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 @@ -359,18 +359,11 @@ 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 @@ -378,18 +371,11 @@ public void versionAttrTakesPrecedenceUnderNewSemantics_DefaultValue() throws Ex 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 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 3632960c3e0401..998fc0c2aa116b 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 @@ -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( @@ -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(); @@ -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 @@ -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(); } @@ -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); } @@ -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); } @@ -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 = @@ -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); } diff --git a/src/test/java/com/google/devtools/build/lib/rules/python/PythonVersionSelectTest.java b/src/test/java/com/google/devtools/build/lib/rules/python/PythonVersionSelectTest.java index 413b48327f9982..6b390586e87a1b 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/python/PythonVersionSelectTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/python/PythonVersionSelectTest.java @@ -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")); @@ -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, @@ -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)