diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonConfiguration.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonConfiguration.java index d19e56b37cd53e..fb5caf6401b78c 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonConfiguration.java @@ -24,9 +24,10 @@ import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; -import com.google.devtools.build.lib.util.OS; +import com.google.devtools.build.lib.events.Event; +import com.google.devtools.build.lib.events.EventHandler; +import com.google.devtools.build.lib.rules.python.PythonOptions; import com.google.devtools.build.lib.vfs.PathFragment; -import com.google.devtools.common.options.Converter; import com.google.devtools.common.options.Option; import com.google.devtools.common.options.OptionDocumentationCategory; import com.google.devtools.common.options.OptionEffectTag; @@ -35,77 +36,61 @@ /** Bazel-specific Python configuration. */ @Immutable public class BazelPythonConfiguration extends BuildConfiguration.Fragment { - /** - * A path converter for python3 path - */ - public static class Python3PathConverter implements Converter { - @Override - public String convert(String input) { - if (input.equals("auto")) { - return OS.getCurrent() == OS.WINDOWS ? "python" : "python3"; - } - return input; - } - - @Override - public String getTypeDescription() { - return "An option for python3 path"; - } - } /** Bazel-specific Python configuration options. */ public static final class Options extends FragmentOptions { @Option( - name = "python2_path", - defaultValue = "python", - documentationCategory = OptionDocumentationCategory.TOOLCHAIN, - effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS, OptionEffectTag.AFFECTS_OUTPUTS}, - metadataTags = {OptionMetadataTag.DEPRECATED}, - help = - "Local path to the Python2 executable. " - + "Deprecated, please use python_path or python_top instead." - ) + name = "python2_path", + defaultValue = "null", + documentationCategory = OptionDocumentationCategory.TOOLCHAIN, + effectTags = {OptionEffectTag.NO_OP}, + metadataTags = {OptionMetadataTag.DEPRECATED}, + help = "Deprecated, no-op. Disabled by `--incompatible_use_python_toolchains`.") public String python2Path; @Option( - name = "python3_path", - converter = Python3PathConverter.class, - defaultValue = "auto", - documentationCategory = OptionDocumentationCategory.TOOLCHAIN, - effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS, OptionEffectTag.AFFECTS_OUTPUTS}, - metadataTags = {OptionMetadataTag.DEPRECATED}, - help = - "Local path to the Python3 executable. " - + "Deprecated, please use python_path or python_top instead." - ) + name = "python3_path", + defaultValue = "null", + documentationCategory = OptionDocumentationCategory.TOOLCHAIN, + effectTags = {OptionEffectTag.NO_OP}, + metadataTags = {OptionMetadataTag.DEPRECATED}, + help = "Deprecated, no-op. Disabled by `--incompatible_use_python_toolchains`.") public String python3Path; @Option( - name = "python_top", - converter = LabelConverter.class, - defaultValue = "null", - documentationCategory = OptionDocumentationCategory.TOOLCHAIN, - effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS, OptionEffectTag.AFFECTS_OUTPUTS}, - help = "The label of py_runtime rule used for the Python interpreter invoked by Bazel." - ) + name = "python_top", + converter = LabelConverter.class, + defaultValue = "null", + documentationCategory = OptionDocumentationCategory.TOOLCHAIN, + effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS, OptionEffectTag.AFFECTS_OUTPUTS}, + help = + "The label of a py_runtime representing the Python interpreter invoked to run Python " + + "targets on the target platform. Deprecated; disabled by " + + "--incompatible_use_python_toolchains.") public Label pythonTop; @Option( - name = "python_path", - defaultValue = "python", - documentationCategory = OptionDocumentationCategory.TOOLCHAIN, - effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS, OptionEffectTag.AFFECTS_OUTPUTS}, - help = "The absolute path of the Python interpreter invoked by Bazel." - ) + name = "python_path", + defaultValue = "null", + documentationCategory = OptionDocumentationCategory.TOOLCHAIN, + effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS, OptionEffectTag.AFFECTS_OUTPUTS}, + help = + "The absolute path of the Python interpreter invoked to run Python targets on the " + + "target platform. Deprecated; disabled by --incompatible_use_python_toolchains.") public String pythonPath; @Option( - name = "experimental_python_import_all_repositories", - defaultValue = "true", - documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, - effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS}, - help = "Do not use." - ) + name = "experimental_python_import_all_repositories", + defaultValue = "true", + documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, + effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS}, + help = + "If true, the roots of repositories in the runfiles tree are added to PYTHONPATH, so " + + "that imports like `import mytoplevelpackage.package.module` are valid." + + " Regardless of whether this flag is true, the runfiles root itself is also" + + " added to the PYTHONPATH, so " + + "`import myreponame.mytoplevelpackage.package.module` is valid. The latter form " + + "is less likely to experience import name collisions.") public boolean experimentalPythonImportAllRepositories; /** @@ -143,7 +128,7 @@ public Class creates() { @Override public ImmutableSet> requiredOptions() { - return ImmutableSet.>of(Options.class); + return ImmutableSet.of(Options.class, PythonOptions.class); } } @@ -153,12 +138,35 @@ private BazelPythonConfiguration(Options options) { this.options = options; } - public String getPython2Path() { - return options.python2Path; - } - - public String getPython3Path() { - return options.python3Path; + @Override + public void reportInvalidOptions(EventHandler reporter, BuildOptions buildOptions) { + PythonOptions pythonOpts = buildOptions.get(PythonOptions.class); + Options opts = buildOptions.get(Options.class); + if (pythonOpts.incompatibleUsePythonToolchains) { + // Forbid deprecated flags. + if (opts.python2Path != null) { + reporter.handle( + Event.error( + "`--python2_path` is disabled by `--incompatible_use_python_toolchains`. Since " + + "`--python2_path` is a deprecated no-op, there is no need to pass it.")); + } + if (opts.python3Path != null) { + reporter.handle( + Event.error( + "`--python3_path` is disabled by `--incompatible_use_python_toolchains`. Since " + + "`--python3_path` is a deprecated no-op, there is no need to pass it.")); + } + if (opts.pythonTop != null) { + reporter.handle( + Event.error( + "`--python_top` is disabled by `--incompatible_use_python_toolchains`. Instead of " + + "configuring the Python runtime directly, register a Python toolchain. See " + + "https://github.com/bazelbuild/bazel/issues/7899. You can temporarily revert " + + "to the legacy flag-based way of specifying toolchains by setting " + + "`--incompatible_use_python_toolchains=false`.")); + } + // TODO(#7901): Also prohibit --python_path here. + } } public Label getPythonTop() { @@ -166,7 +174,7 @@ public Label getPythonTop() { } public String getPythonPath() { - return options.pythonPath; + return options.pythonPath == null ? "python" : options.pythonPath; } public boolean getImportAllRepositories() { diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyRuntime.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyRuntime.java index 4459f39500f9fb..3b145b6660bb51 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PyRuntime.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PyRuntime.java @@ -33,6 +33,8 @@ public final class PyRuntime implements RuleConfiguredTargetFactory { @Override public ConfiguredTarget create(RuleContext ruleContext) throws ActionConflictException { + PythonConfiguration pyConfig = ruleContext.getFragment(PythonConfiguration.class); + NestedSet files = PrerequisiteArtifacts.nestedSet(ruleContext, "files", Mode.TARGET); Artifact interpreter = ruleContext.getPrerequisiteArtifact("interpreter", Mode.TARGET); @@ -58,15 +60,24 @@ public ConfiguredTarget create(RuleContext ruleContext) throws ActionConflictExc } if (pythonVersion == PythonVersion._INTERNAL_SENTINEL) { - // Use the same default as py_binary/py_test would use for their python_version attribute. - // (Of course, in our case there's no configuration transition involved.) - pythonVersion = ruleContext.getFragment(PythonConfiguration.class).getDefaultPythonVersion(); + if (pyConfig.useToolchains()) { + ruleContext.attributeError( + "python_version", + "When using Python toolchains, this attribute must be set explicitly to either 'PY2' " + + "or 'PY3'. See https://github.com/bazelbuild/bazel/issues/7899 for more " + + "information. You can temporarily avoid this error by reverting to the legacy " + + "Python runtime mechanism (`--incompatible_use_python_toolchains=false`)."); + } else { + // Use the same default as py_binary/py_test would use for their python_version attribute. + // (Of course, in our case there's no configuration transition involved.) + pythonVersion = pyConfig.getDefaultPythonVersion(); + } } - Preconditions.checkState(pythonVersion.isTargetValue()); if (ruleContext.hasErrors()) { return null; } + Preconditions.checkState(pythonVersion.isTargetValue()); PyRuntimeInfo provider = hermetic 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 0468469cdd70e2..fa99438f3b4222 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 @@ -230,11 +230,14 @@ public String getTypeDescription() { public boolean incompatibleDisallowLegacyPyProvider; @Option( - // TODO(brandjon): Rename to --incompatible_use_python_toolchains when ready to make available - name = "experimental_use_python_toolchains", + name = "incompatible_use_python_toolchains", defaultValue = "false", documentationCategory = OptionDocumentationCategory.GENERIC_INPUTS, effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS}, + metadataTags = { + OptionMetadataTag.INCOMPATIBLE_CHANGE, + OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES + }, help = "If set to true, executable native Python rules will use the Python runtime specified by " + "the Python toolchain, rather than the runtime given by legacy flags like " diff --git a/src/test/java/com/google/devtools/build/lib/bazel/rules/python/BazelPyBinaryConfiguredTargetTest.java b/src/test/java/com/google/devtools/build/lib/bazel/rules/python/BazelPyBinaryConfiguredTargetTest.java index 4e96207bd2d4e8..a878000b0a28bb 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/rules/python/BazelPyBinaryConfiguredTargetTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/rules/python/BazelPyBinaryConfiguredTargetTest.java @@ -94,7 +94,7 @@ public void runtimeSetByPythonTop() throws Exception { ")"); String pythonTop = analysisMock.pySupport().createPythonTopEntryPoint(mockToolsConfig, "//pkg:my_py_runtime"); - useConfiguration("--experimental_use_python_toolchains=false", "--python_top=" + pythonTop); + useConfiguration("--incompatible_use_python_toolchains=false", "--python_top=" + pythonTop); String path = getInterpreterPathFromStub(getConfiguredTarget("//pkg:pybin")); assertThat(path).isEqualTo("/system/python2"); } @@ -107,7 +107,7 @@ public void runtimeSetByPythonPath() throws Exception { " name = 'pybin',", " srcs = ['pybin.py'],", ")"); - useConfiguration("--experimental_use_python_toolchains=false", "--python_path=/system/python2"); + useConfiguration("--incompatible_use_python_toolchains=false", "--python_path=/system/python2"); String path = getInterpreterPathFromStub(getConfiguredTarget("//pkg:pybin")); assertThat(path).isEqualTo("/system/python2"); } @@ -120,7 +120,7 @@ public void runtimeDefaultsToPythonSystemCommand() throws Exception { " name = 'pybin',", " srcs = ['pybin.py'],", ")"); - useConfiguration("--experimental_use_python_toolchains=false"); + useConfiguration("--incompatible_use_python_toolchains=false"); String path = getInterpreterPathFromStub(getConfiguredTarget("//pkg:pybin")); assertThat(path).isEqualTo("python"); } @@ -141,7 +141,7 @@ public void pythonTopTakesPrecedenceOverPythonPath() throws Exception { String pythonTop = analysisMock.pySupport().createPythonTopEntryPoint(mockToolsConfig, "//pkg:my_py_runtime"); useConfiguration( - "--experimental_use_python_toolchains=false", + "--incompatible_use_python_toolchains=false", "--python_top=" + pythonTop, "--python_path=/better/not/be/this/one"); String path = getInterpreterPathFromStub(getConfiguredTarget("//pkg:pybin")); @@ -205,7 +205,7 @@ public void runtimeObtainedFromToolchain() throws Exception { " python_version = 'PY3',", ")"); useConfiguration( - "--experimental_use_python_toolchains=true", + "--incompatible_use_python_toolchains=true", "--extra_toolchains=//toolchains:py_toolchain"); String py2Path = getInterpreterPathFromStub(getConfiguredTarget("//pkg:py2_bin")); @@ -225,7 +225,7 @@ public void toolchainCanOmitUnusedRuntimeVersion() throws Exception { " python_version = 'PY2',", ")"); useConfiguration( - "--experimental_use_python_toolchains=true", + "--incompatible_use_python_toolchains=true", "--extra_toolchains=//toolchains:py_toolchain_for_py2_only"); String path = getInterpreterPathFromStub(getConfiguredTarget("//pkg:py2_bin")); @@ -237,24 +237,14 @@ public void toolchainTakesPrecedenceOverLegacyFlags() throws Exception { defineToolchains(); scratch.file( "pkg/BUILD", - "py_runtime(", - " name = 'dont_use_this_runtime',", - " interpreter_path = '/dont/use/me',", - " python_version = 'PY2',", - ")", "py_binary(", " name = 'py2_bin',", " srcs = ['py2_bin.py'],", " python_version = 'PY2',", ")"); - String pythonTop = - analysisMock - .pySupport() - .createPythonTopEntryPoint(mockToolsConfig, "//pkg:dont_use_this_runtime"); useConfiguration( - "--experimental_use_python_toolchains=true", + "--incompatible_use_python_toolchains=true", "--extra_toolchains=//toolchains:py_toolchain", - "--python_top=" + pythonTop, "--python_path=/better/not/be/this/one"); String path = getInterpreterPathFromStub(getConfiguredTarget("//pkg:py2_bin")); @@ -273,7 +263,7 @@ public void toolchainIsMissingNeededRuntime() throws Exception { " python_version = 'PY3',", ")"); useConfiguration( - "--experimental_use_python_toolchains=true", + "--incompatible_use_python_toolchains=true", "--extra_toolchains=//toolchains:py_toolchain_for_py2_only"); getConfiguredTarget("//pkg:py3_bin"); @@ -324,7 +314,7 @@ private void analyzePy2BinaryTargetUsingCustomToolchain() throws Exception { " python_version = 'PY2',", ")"); useConfiguration( - "--experimental_use_python_toolchains=true", + "--incompatible_use_python_toolchains=true", "--extra_toolchains=//toolchains:custom_toolchain"); getConfiguredTarget("//pkg:pybin"); } diff --git a/src/test/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonConfigurationTest.java b/src/test/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonConfigurationTest.java index cdb3223df05008..5b5eb2dfa72651 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonConfigurationTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonConfigurationTest.java @@ -61,4 +61,23 @@ public void pythonPathMustBeAbsolute() { () -> create("--python_path=pajama")); assertThat(expected).hasMessageThat().contains("python_path must be an absolute path"); } + + @Test + public void legacyFlagsDeprecatedByPythonToolchains() throws Exception { + checkError( + "`--python2_path` is disabled by `--incompatible_use_python_toolchains`", + "--incompatible_use_python_toolchains=true", + "--python2_path=/system/python2"); + checkError( + "`--python3_path` is disabled by `--incompatible_use_python_toolchains`", + "--incompatible_use_python_toolchains=true", + "--python3_path=/system/python3"); + checkError( + "`--python_top` is disabled by `--incompatible_use_python_toolchains`", + "--incompatible_use_python_toolchains=true", + "--python_top=//mypkg:my_py_runtime"); + // TODO(#7901): Also test that --python_path is disallowed (once implemented). Currently we + // still need it to communicate the location of python.exe on Windows from the client to the + // server. + } } diff --git a/src/test/java/com/google/devtools/build/lib/rules/python/PyRuntimeConfiguredTargetTest.java b/src/test/java/com/google/devtools/build/lib/rules/python/PyRuntimeConfiguredTargetTest.java index 8569131d032b0c..75e498fcea5cf1 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/python/PyRuntimeConfiguredTargetTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/python/PyRuntimeConfiguredTargetTest.java @@ -73,6 +73,8 @@ public void nonhermeticRuntime() throws Exception { @Test public void pythonVersionDefault() throws Exception { ensureDefaultIsPY2(); + // When using toolchains, the python_version attribute is mandatory. + useConfiguration("--incompatible_use_python_toolchains=false"); scratch.file( "pkg/BUILD", "py_runtime(", @@ -170,4 +172,19 @@ public void badPythonVersionAttribute() throws Exception { assertContainsEvent("invalid value in 'python_version' attribute"); } + + @Test + public void versionAttributeMandatoryWhenUsingToolchains() throws Exception { + reporter.removeHandler(failFastHandler); + useConfiguration("--incompatible_use_python_toolchains=true"); + scratch.file( + "pkg/BUILD", + "py_runtime(", + " name = 'myruntime',", + " interpreter_path = '/system/interpreter',", + ")"); + getConfiguredTarget("//pkg:myruntime"); + + assertContainsEvent("must be set explicitly to either 'PY2' or 'PY3'"); + } } 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 ab93932a536804..b2b5f12374c9c7 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 @@ -221,7 +221,7 @@ public void getHost_CopiesMostValues() throws Exception { "--incompatible_py2_outputs_are_suffixed=true", "--build_python_zip=true", "--incompatible_disallow_legacy_py_provider=true", - "--experimental_use_python_toolchains=true"); + "--incompatible_use_python_toolchains=true"); PythonOptions hostOpts = (PythonOptions) opts.getHost(); assertThat(hostOpts.incompatibleAllowPythonVersionTransitions).isTrue(); assertThat(hostOpts.incompatibleRemoveOldPythonVersionApi).isTrue(); diff --git a/src/test/shell/bazel/bazel_example_test.sh b/src/test/shell/bazel/bazel_example_test.sh index 168d90c358aff4..663dfefd0ba074 100755 --- a/src/test/shell/bazel/bazel_example_test.sh +++ b/src/test/shell/bazel/bazel_example_test.sh @@ -97,13 +97,13 @@ function test_genrule_and_genquery() { } function test_native_python() { - assert_build //examples/py_native:bin --python2_path=python - assert_test_ok //examples/py_native:test --python2_path=python - assert_test_fails //examples/py_native:fail --python2_path=python + assert_build //examples/py_native:bin + assert_test_ok //examples/py_native:test + assert_test_fails //examples/py_native:fail } function test_native_python_with_zip() { - assert_build //examples/py_native:bin --python2_path=python --build_python_zip + assert_build //examples/py_native:bin --build_python_zip # run the python package directly ./bazel-bin/examples/py_native/bin >& $TEST_log \ || fail "//examples/py_native:bin execution failed" @@ -112,8 +112,8 @@ function test_native_python_with_zip() { python ./bazel-bin/examples/py_native/bin >& $TEST_log \ || fail "//examples/py_native:bin execution failed" expect_log "Fib(5) == 8" - assert_test_ok //examples/py_native:test --python2_path=python --build_python_zip - assert_test_fails //examples/py_native:fail --python2_path=python --build_python_zip + assert_test_ok //examples/py_native:test --build_python_zip + assert_test_fails //examples/py_native:fail --build_python_zip } function test_shell() { diff --git a/tools/python/pywrapper_template.txt b/tools/python/pywrapper_template.txt index c0a67d13ccbbb9..c2f5b1882cda9c 100644 --- a/tools/python/pywrapper_template.txt +++ b/tools/python/pywrapper_template.txt @@ -1,4 +1,4 @@ -#!/bin/sh +#!/bin/bash # TODO(#7843): integration tests for failure to find python / wrong version # found / error while trying to print version