Make Window launchers relocatable

When the Windows launcher binary is copied to become the `executable` of another target, it can't expect to find the main file right next to it. Instead, look it up via `Rlocation`, which doesn't require colocation to find the file. This helps with a common pattern in wrapper rules where `ctx.symlink` is used to work around the limitation that the `executable` of a target has to be produced by that target.

For the Python rules, for which the source of truth is not the Bazel repo anymore, the new launch key is checked for in a backwards compatible way.

Closes #23076.

PiperOrigin-RevId: 657116461
Change-Id: I3f966b48e0812668cfa7bd394f2eaf23d66889b6

Author: fmeum

src/main/java/com/google/devtools/build/lib/bazel/rules/sh/ShBinary.java

@@ -102,7 +102,7 @@ private static boolean isWindowsExecutable(Artifact artifact) {
   }
 
   private static Artifact createWindowsExeLauncher(
-      RuleContext ruleContext, PathFragment shExecutable) throws RuleErrorException {
+      RuleContext ruleContext, PathFragment shExecutable, Artifact primaryOutput) {
     Artifact bashLauncher =
         ruleContext.getImplicitOutputArtifact(ruleContext.getTarget().getName() + ".exe");
 
@@ -114,6 +114,11 @@ private static Artifact createWindowsExeLauncher(
                 "symlink_runfiles_enabled",
                 ruleContext.getConfiguration().runfilesEnabled() ? "1" : "0")
             .addKeyValuePair("bash_bin_path", shExecutable.getPathString())
+            .addKeyValuePair(
+                "bash_file_rlocationpath",
+                PathFragment.create(ruleContext.getWorkspaceName())
+                    .getRelative(primaryOutput.getRunfilesPathString())
+                    .getPathString())
             .build();
 
     LauncherFileWriteAction.createAndRegister(ruleContext, bashLauncher, launchInfo);
@@ -139,6 +144,6 @@ private static Artifact launcherForWindows(
     PathFragment shExecutable =
         ShToolchain.getPathForPlatform(
             ruleContext.getConfiguration(), ruleContext.getExecutionPlatform());
-    return createWindowsExeLauncher(ruleContext, shExecutable);
+    return createWindowsExeLauncher(ruleContext, shExecutable, primaryOutput);
   }
 }

src/main/starlark/builtins_bzl/common/python/py_executable_bazel.bzl

@@ -209,8 +209,8 @@ def _create_executable(
     # 2. (non-Windows) A self-executable zip file of a bootstrap template based program.
     # 3. (Windows) A native Windows executable that finds and launches
     #    the actual underlying Bazel program (one of the above). Note that
-    #    it implicitly assumes one of the above is located next to it, and
-    #    that --build_python_zip defaults to true for Windows.
+    #    it implicitly assumes that --build_python_zip defaults to true for
+    #    Windows.
 
     should_create_executable_zip = False
     bootstrap_output = None
@@ -220,20 +220,24 @@ def _create_executable(
         else:
             bootstrap_output = executable
     else:
-        _create_windows_exe_launcher(
-            ctx,
-            output = executable,
-            use_zip_file = build_zip_enabled,
-            python_binary_path = runtime_details.executable_interpreter_path,
-        )
-        if not build_zip_enabled:
+        if build_zip_enabled:
+            python_file = zip_file
+        else:
             # On Windows, the main executable has an "exe" extension, so
             # here we re-use the un-extensioned name for the bootstrap output.
             bootstrap_output = ctx.actions.declare_file(base_executable_name)
+            python_file = bootstrap_output
 
             # The launcher looks for the non-zip executable next to
             # itself, so add it to the default outputs.
             extra_files_to_build.append(bootstrap_output)
+        _create_windows_exe_launcher(
+            ctx,
+            output = executable,
+            use_zip_file = build_zip_enabled,
+            python_binary_path = runtime_details.executable_interpreter_path,
+            python_file = python_file,
+        )
 
     if should_create_executable_zip:
         if bootstrap_output != None:
@@ -316,7 +320,8 @@ def _create_windows_exe_launcher(
         *,
         output,
         python_binary_path,
-        use_zip_file):
+        use_zip_file,
+        python_file):
     launch_info = ctx.actions.args()
     launch_info.use_param_file("%s", use_always = True)
     launch_info.set_param_file_format("multiline")
@@ -328,6 +333,7 @@ def _create_windows_exe_launcher(
     )
     launch_info.add(python_binary_path, format = "python_bin_path=%s")
     launch_info.add("1" if use_zip_file else "0", format = "use_zip_file=%s")
+    launch_info.add(python_file.short_path, format = "python_file_short_path=%s")
 
     ctx.actions.run(
         executable = ctx.executable._windows_launcher_maker,

src/test/java/com/google/devtools/build/lib/blackbox/framework/ProcessRunner.java

@@ -15,6 +15,7 @@
 package com.google.devtools.build.lib.blackbox.framework;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Lists;
 import com.google.common.flogger.GoogleLogger;
 import com.google.common.flogger.LazyArgs;
@@ -77,6 +78,18 @@ public ProcessResult runSynchronously() throws Exception {
     ProcessBuilder processBuilder = new ProcessBuilder(commandParts);
     processBuilder.directory(parameters.workingDirectory());
     parameters.environment().ifPresent(map -> processBuilder.environment().putAll(map));
+    // Always clear the variables used for runfiles discovery so that the process doesn't inherit
+    // them from the Bazel test environment.
+    processBuilder
+        .environment()
+        .keySet()
+        .removeAll(
+            ImmutableSet.of(
+                "RUNFILES_DIR",
+                "RUNFILES_MANIFEST_FILE",
+                "RUNFILES_MANIFEST_ONLY",
+                "JAVA_RUNFILES",
+                "PYTHON_RUNFILES"));
 
     parameters.redirectOutput().ifPresent(path -> processBuilder.redirectOutput(path.toFile()));
     parameters.redirectError().ifPresent(path -> processBuilder.redirectError(path.toFile()));

src/test/py/bazel/runfiles_test.py

@@ -480,6 +480,138 @@ def testTestsRunWithNoBuildRunfileLinksAndNoEnableRunfiles(self):
         ["test", ":test", "--nobuild_runfile_links", "--noenable_runfiles"]
     )
 
+  def testWrappedShBinary(self):
+    self.writeWrapperRule()
+    self.ScratchFile("MODULE.bazel")
+    self.ScratchFile(
+        "BUILD",
+        [
+            "sh_binary(",
+            "  name = 'binary',",
+            "  srcs = ['binary.sh'],",
+            "  visibility = ['//visibility:public'],",
+            ")",
+        ],
+    )
+    self.ScratchFile(
+        "binary.sh",
+        [
+            "echo Hello, World!",
+        ],
+        executable=True,
+    )
+
+    _, stdout, _ = self.RunBazel(["run", "//wrapped"])
+    self.assertEqual(stdout, ["Hello, World!"])
+
+  def testWrappedPyBinary(self):
+    self.writeWrapperRule()
+    self.ScratchFile("MODULE.bazel")
+    self.ScratchFile(
+        "BUILD",
+        [
+            "py_binary(",
+            "  name = 'binary',",
+            "  srcs = ['binary.py'],",
+            "  visibility = ['//visibility:public'],",
+            ")",
+        ],
+    )
+    self.ScratchFile(
+        "binary.py",
+        [
+            "print('Hello, World!')",
+        ],
+    )
+
+    _, stdout, _ = self.RunBazel(["run", "//wrapped"])
+    self.assertEqual(stdout, ["Hello, World!"])
+
+  def testWrappedJavaBinary(self):
+    self.writeWrapperRule()
+    self.ScratchFile("MODULE.bazel")
+    self.ScratchFile(
+        "BUILD",
+        [
+            "java_binary(",
+            "  name = 'binary',",
+            "  srcs = ['Binary.java'],",
+            "  main_class = 'Binary',",
+            "  visibility = ['//visibility:public'],",
+            ")",
+        ],
+    )
+    self.ScratchFile(
+        "Binary.java",
+        [
+            "public class Binary {",
+            "  public static void main(String[] args) {",
+            '    System.out.println("Hello, World!");',
+            "  }",
+            "}",
+        ],
+    )
+
+    _, stdout, _ = self.RunBazel(["run", "//wrapped"])
+    self.assertEqual(stdout, ["Hello, World!"])
+
+  def writeWrapperRule(self):
+    self.ScratchFile("rules/BUILD")
+    self.ScratchFile(
+        "rules/wrapper.bzl",
+        [
+            "def _wrapper_impl(ctx):",
+            "    target = ctx.attr.target",
+            (
+                "    original_executable ="
+                " target[DefaultInfo].files_to_run.executable"
+            ),
+            (
+                "    executable ="
+                " ctx.actions.declare_file(original_executable.basename)"
+            ),
+            (
+                "    ctx.actions.symlink(output = executable, target_file ="
+                " original_executable)"
+            ),
+            (
+                "    data_runfiles ="
+                " ctx.runfiles([executable]).merge(target[DefaultInfo].data_runfiles)"
+            ),
+            (
+                "    default_runfiles ="
+                " ctx.runfiles([executable]).merge(target[DefaultInfo].default_runfiles)"
+            ),
+            "    return [",
+            "        DefaultInfo(",
+            "            executable = executable,",
+            "            files = target[DefaultInfo].files,",
+            "            data_runfiles = data_runfiles,",
+            "            default_runfiles = default_runfiles,",
+            "        ),    ]",
+            "wrapper = rule(",
+            "    implementation = _wrapper_impl,",
+            "    attrs = {",
+            "        'target': attr.label(",
+            "            cfg = 'target',",
+            "            executable = True,",
+            "        ),",
+            "    },",
+            "    executable = True,",
+            ")",
+        ],
+    )
+    self.ScratchFile(
+        "wrapped/BUILD",
+        [
+            "load('//rules:wrapper.bzl', 'wrapper')",
+            "wrapper(",
+            "  name = 'wrapped',",
+            "  target = '//:binary',",
+            ")",
+        ],
+    )
+
 
 if __name__ == "__main__":
   absltest.main()

src/test/shell/bazel/bazel_windows_example_test.sh

@@ -293,8 +293,11 @@ function test_java_test() {
 function test_native_python() {
   # On windows, we build a python executable zip as the python binary
   assert_build //examples/py_native:bin
-  # run the python package directly
-  ./bazel-bin/examples/py_native/bin >& $TEST_log \
+  # run the python package directly, clearing out runfiles variables to
+  # ensure that the binary finds its own runfiles correctly
+  env -u RUNFILES_DIR -u RUNFILES_MANIFEST_FILE -u RUNFILES_MANIFEST_ONLY \
+    -u JAVA_RUNFILES -u PYTHON_RUNFILES \
+    ./bazel-bin/examples/py_native/bin >& $TEST_log \
     || fail "//examples/py_native:bin execution failed"
   expect_log "Fib(5) == 8"
   # Using python <zipfile> to run the python package

src/test/shell/bazel/python_version_test.sh

@@ -266,7 +266,11 @@ print(__file__)
 EOF
 
   bazel build //test:pybin --build_python_zip &> $TEST_log || fail "bazel build failed"
-  pybin_location=$(bazel-bin/test/pybin)
+  # Clear out runfiles variables to ensure that the binary finds its own
+  # runfiles correctly.
+  pybin_location=$(env -u RUNFILES_DIR -u RUNFILES_MANIFEST_FILE \
+      -u RUNFILES_MANIFEST_ONLY -u JAVA_RUNFILES -u PYTHON_RUNFILES \
+      bazel-bin/test/pybin)
 
   # The pybin location is "<ms root>/runfiles/<workspace>/test/pybin.py",
   # so we have to go up 4 directories to get to the module space root

src/test/shell/integration/run_test.sh

@@ -594,15 +594,18 @@ echo "goodbye $@"
 EOF
   chmod +x "$pkg/farewell.sh"
 
-  bazel run --run_under="//$pkg:greetings friend &&" -- "//$pkg:farewell" buddy \
+  bazel run --run_under="//$pkg:greetings friend && unset RUNFILES_MANIFEST_FILE &&" -- "//$pkg:farewell" buddy \
       >$TEST_log || fail "expected test to pass"
   # TODO(https://github.com/bazelbuild/bazel/issues/22148): bazel-team - This is
   # just demonstrating how things are, it's probably not how we want them to be.
+  # "unset RUNFILES_MANIFEST_FILE" is necessary because the environment
+  # variables set by //pkg:greetings are otherwise passed to //pkg:farewell and
+  # break its runfiles discovery.
   if "$is_windows"; then
     expect_log "hello there friend"
     expect_log "goodbye buddy"
   else
-    expect_log "hello there friend && .*bin/$pkg/farewell buddy"
+    expect_log "hello there friend && unset RUNFILES_MANIFEST_FILE && .*bin/$pkg/farewell buddy"
     expect_not_log "goodbye"
   fi
 }

src/tools/launcher/bash_launcher.cc

@@ -27,6 +27,8 @@ using std::wostringstream;
 using std::wstring;
 
 static constexpr const char* BASH_BIN_PATH = "bash_bin_path";
+static constexpr const char* BASH_FILE_RLOCATIONPATH =
+    "bash_file_rlocationpath";
 
 ExitCode BashBinaryLauncher::Launch() {
   wstring bash_binary = this->GetLaunchInfoByKey(BASH_BIN_PATH);
@@ -52,8 +54,10 @@ ExitCode BashBinaryLauncher::Launch() {
 
   vector<wstring> origin_args = this->GetCommandlineArguments();
   wostringstream bash_command;
-  wstring bash_main_file = GetBinaryPathWithoutExtension(GetLauncherPath());
-  bash_command << BashEscapeArg(bash_main_file);
+  wstring bash_file_rlocationpath =
+      this->GetLaunchInfoByKey(BASH_FILE_RLOCATIONPATH);
+  wstring bash_file = Rlocation(bash_file_rlocationpath, true);
+  bash_command << BashEscapeArg(bash_file);
   for (int i = 1; i < origin_args.size(); i++) {
     bash_command << L' ';
     bash_command << BashEscapeArg(origin_args[i]);

src/tools/launcher/launcher.cc

@@ -197,6 +197,14 @@ wstring BinaryLauncherBase::GetLaunchInfoByKey(const string& key) {
   return item->second;
 }
 
+wstring BinaryLauncherBase::GetLaunchInfoByKeyOrEmpty(const std::string& key) {
+  auto item = launch_info.find(key);
+  if (item == launch_info.end()) {
+    return L"";
+  }
+  return item->second;
+}
+
 const vector<wstring>& BinaryLauncherBase::GetCommandlineArguments() const {
   return this->commandline_arguments;
 }

src/tools/launcher/launcher.h

@@ -49,6 +49,7 @@ class BinaryLauncherBase {
 
   // Get launch information based on a launch info key.
   std::wstring GetLaunchInfoByKey(const std::string& key);
+  std::wstring GetLaunchInfoByKeyOrEmpty(const std::string& key);
 
   // Get the original command line arguments passed to this binary.
   const std::vector<std::wstring>& GetCommandlineArguments() const;