From fb164a00c31e79a53bcd1e034c5a9dc49de1c347 Mon Sep 17 00:00:00 2001 From: Yun Peng Date: Mon, 3 Sep 2018 03:29:02 -0700 Subject: [PATCH] Windows: Make Python binary work with runfiles symlink tree Closes #6036. Change-Id: I1f8ffc6cf675c86b4799682282398dd847babcf0 PiperOrigin-RevId: 211333043 --- .../rules/python/BazelPythonSemantics.java | 20 +++++++++++---- .../rules/python/python_stub_template.txt | 13 +++++++--- .../build/lib/rules/python/PyCommon.java | 9 +++++++ .../shell/bazel/bazel_windows_example_test.sh | 25 +++++++++++++++++++ src/test/shell/integration/runfiles_test.sh | 11 ++++---- src/tools/launcher/python_launcher.cc | 13 +++++++--- 6 files changed, 74 insertions(+), 17 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java index 9a71ae9f553b68..0ecc068a56deb5 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java @@ -144,10 +144,19 @@ public Artifact createExecutable( String pythonBinary = getPythonBinary(ruleContext, config); if (!ruleContext.getFragment(PythonConfiguration.class).buildPythonZip()) { + Artifact stubOutput = executable; + if (OS.getCurrent() == OS.WINDOWS + && ruleContext.getConfiguration().enableWindowsExeLauncher()) { + // On Windows, use a Windows native binary to launch the python launcher script (stub file). + stubOutput = common.getPythonLauncherArtifact(executable); + executable = + createWindowsExeLauncher(ruleContext, pythonBinary, executable, /*useZipFile*/ false); + } + ruleContext.registerAction( new TemplateExpansionAction( ruleContext.getActionOwner(), - executable, + stubOutput, STUB_TEMPLATE, ImmutableList.of( Substitution.of("%main%", main), @@ -155,8 +164,8 @@ public Artifact createExecutable( Substitution.of("%imports%", Joiner.on(":").join(imports)), Substitution.of("%workspace_name%", ruleContext.getWorkspaceName()), Substitution.of("%is_zipfile%", "False"), - Substitution.of("%import_all%", - config.getImportAllRepositories() ? "True" : "False")), + Substitution.of( + "%import_all%", config.getImportAllRepositories() ? "True" : "False")), true)); } else { Artifact zipFile = common.getPythonZipArtifact(executable); @@ -194,7 +203,7 @@ public Artifact createExecutable( .build(ruleContext)); } else { if (ruleContext.getConfiguration().enableWindowsExeLauncher()) { - return createWindowsExeLauncher(ruleContext, pythonBinary, executable); + return createWindowsExeLauncher(ruleContext, pythonBinary, executable, true); } ruleContext.registerAction( @@ -212,13 +221,14 @@ public Artifact createExecutable( } private static Artifact createWindowsExeLauncher( - RuleContext ruleContext, String pythonBinary, Artifact pythonLauncher) + RuleContext ruleContext, String pythonBinary, Artifact pythonLauncher, boolean useZipFile) throws InterruptedException { LaunchInfo launchInfo = LaunchInfo.builder() .addKeyValuePair("binary_type", "Python") .addKeyValuePair("workspace_name", ruleContext.getWorkspaceName()) .addKeyValuePair("python_bin_path", pythonBinary) + .addKeyValuePair("use_zip_file", useZipFile ? "1" : "0") .build(); LauncherFileWriteAction.createAndRegister(ruleContext, pythonLauncher, launchInfo); return pythonLauncher; diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/python_stub_template.txt b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/python_stub_template.txt index d2cdccc6fd1be9..b4ab41f3312f39 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/python_stub_template.txt +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/python_stub_template.txt @@ -55,10 +55,10 @@ def FindPythonBinary(module_space): # Case 1: Path is a label. Not supported yet. raise AssertionError( 'Bazel does not support execution of Python interpreters via labels yet') - elif PYTHON_BINARY.startswith('/'): + elif os.path.isabs(PYTHON_BINARY): # Case 2: Absolute path. return PYTHON_BINARY - elif '/' in PYTHON_BINARY: + elif os.sep in PYTHON_BINARY: # Case 3: Path is relative to the repo root. return os.path.join(module_space, PYTHON_BINARY) else: @@ -72,7 +72,7 @@ def CreatePythonPathEntries(python_imports, module_space): # Find the runfiles tree def FindModuleSpace(): stub_filename = os.path.abspath(sys.argv[0]) - module_space = stub_filename + '.runfiles' + module_space = stub_filename + ('.exe' if IsWindows() else '') + '.runfiles' if os.path.isdir(module_space): return module_space @@ -190,7 +190,12 @@ def Main(): shutil.rmtree(os.path.dirname(module_space), True) exit(retCode) else: - os.execv(args[0], args) + if IsWindows(): + # On Windows, os.execv doesn't handle arguments with spaces correctly, + # and it actually starts a subprocess just like subprocess.call. + exit(subprocess.call(args)) + else: + os.execv(args[0], args) except EnvironmentError: # This works from Python 2.4 all the way to 3.x. e = sys.exc_info()[1] diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java index 4483d7e063a232..be2e5fca0e0fd3 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java @@ -138,6 +138,10 @@ public void initBinary(List srcs) { if (ruleContext.getFragment(PythonConfiguration.class).buildPythonZip()) { filesToBuildBuilder.add(getPythonZipArtifact(executable)); + } else if (OS.getCurrent() == OS.WINDOWS) { + // TODO(bazel-team): Here we should check target platform instead of using OS.getCurrent(). + // On Windows, add the python stub launcher in the set of files to build. + filesToBuildBuilder.add(getPythonLauncherArtifact(executable)); } filesToBuild = filesToBuildBuilder.build(); @@ -154,6 +158,11 @@ public Artifact getPythonZipArtifact(Artifact executable) { return ruleContext.getRelatedArtifact(executable.getRootRelativePath(), ".zip"); } + /** @return An artifact next to the executable file with no suffix, only used on Windows */ + public Artifact getPythonLauncherArtifact(Artifact executable) { + return ruleContext.getRelatedArtifact(executable.getRootRelativePath(), ""); + } + public void addCommonTransitiveInfoProviders(RuleConfiguredTargetBuilder builder, PythonSemantics semantics, NestedSet filesToBuild) { diff --git a/src/test/shell/bazel/bazel_windows_example_test.sh b/src/test/shell/bazel/bazel_windows_example_test.sh index 3779510abcd2a6..12aeeb1a528c55 100755 --- a/src/test/shell/bazel/bazel_windows_example_test.sh +++ b/src/test/shell/bazel/bazel_windows_example_test.sh @@ -196,6 +196,31 @@ function test_native_python() { assert_test_fails //examples/py_native:fail } +function test_native_python_with_runfiles() { + BUILD_FLAGS="--experimental_enable_runfiles --build_python_zip=0" + bazel build -s --verbose_failures $BUILD_FLAGS //examples/py_native:bin \ + || fail "Failed to build //examples/py_native:bin with runfiles support" + ( + # Clear runfiles related envs + unset RUNFILES_MANIFEST_FILE + unset RUNFILES_MANIFEST_ONLY + unset RUNFILES_DIR + # Run the python package directly + ./bazel-bin/examples/py_native/bin >& $TEST_log \ + || fail "//examples/py_native:bin execution failed" + expect_log "Fib(5) == 8" + # Use python to run the python package + python ./bazel-bin/examples/py_native/bin >& $TEST_log \ + || fail "//examples/py_native:bin execution failed" + expect_log "Fib(5) == 8" + ) + bazel test --test_output=errors $BUILD_FLAGS //examples/py_native:test >> $TEST_log 2>&1 \ + || fail "Test //examples/py_native:test failed while expecting success" + bazel test --test_output=errors $BUILD_FLAGS //examples/py_native:fail >> $TEST_log 2>&1 \ + && fail "Test //examples/py_native:fail succeed while expecting failure" \ + || true +} + function test_native_python_with_python3() { PYTHON3_PATH=${PYTHON3_PATH:-/c/Program Files/Anaconda3} if [ ! -x "${PYTHON3_PATH}/python.exe" ]; then diff --git a/src/test/shell/integration/runfiles_test.sh b/src/test/shell/integration/runfiles_test.sh index fd85be9849fd82..2e0d7809828daa 100755 --- a/src/test/shell/integration/runfiles_test.sh +++ b/src/test/shell/integration/runfiles_test.sh @@ -179,14 +179,15 @@ EOF # that accounts for everything cd ../.. - # For shell binary, we build both `bin` and `bin.exe`, but on Linux we only build `bin` - # That's why we have one more symlink on Windows. + # For shell binary and python binary, we build both `bin` and `bin.exe`, + # but on Linux we only build `bin`. + # That's why we have two more symlinks on Windows. if "$is_windows"; then - assert_equals 10 $(find ${WORKSPACE_NAME} -type l | wc -l) + assert_equals 11 $(find ${WORKSPACE_NAME} -type l | wc -l) assert_equals 4 $(find ${WORKSPACE_NAME} -type f | wc -l) assert_equals 9 $(find ${WORKSPACE_NAME} -type d | wc -l) - assert_equals 23 $(find ${WORKSPACE_NAME} | wc -l) - assert_equals 14 $(wc -l < MANIFEST) + assert_equals 24 $(find ${WORKSPACE_NAME} | wc -l) + assert_equals 15 $(wc -l < MANIFEST) else assert_equals 9 $(find ${WORKSPACE_NAME} -type l | wc -l) assert_equals 4 $(find ${WORKSPACE_NAME} -type f | wc -l) diff --git a/src/tools/launcher/python_launcher.cc b/src/tools/launcher/python_launcher.cc index a0f8775923d97c..81d836c35ed261 100644 --- a/src/tools/launcher/python_launcher.cc +++ b/src/tools/launcher/python_launcher.cc @@ -25,6 +25,7 @@ using std::vector; using std::wstring; static constexpr const char* PYTHON_BIN_PATH = "python_bin_path"; +static constexpr const char* USE_ZIP_FILE = "use_zip_file"; ExitCode PythonBinaryLauncher::Launch() { wstring python_binary = this->GetLaunchInfoByKey(PYTHON_BIN_PATH); @@ -35,10 +36,16 @@ ExitCode PythonBinaryLauncher::Launch() { } vector args = this->GetCommandlineArguments(); - wstring python_zip_file = GetBinaryPathWithoutExtension(args[0]) + L".zip"; + wstring use_zip_file = this->GetLaunchInfoByKey(USE_ZIP_FILE); + wstring python_file; + if (use_zip_file == L"1") { + python_file = GetBinaryPathWithoutExtension(args[0]) + L".zip"; + } else { + python_file = GetBinaryPathWithoutExtension(args[0]); + } - // Replace the first argument with python zip file path - args[0] = python_zip_file; + // Replace the first argument with python file path + args[0] = python_file; // Escape arguments that has spaces for (int i = 1; i < args.size(); i++) {