From 8e2ba6e9df6f14799a1b91cb15c426a367bc8bd6 Mon Sep 17 00:00:00 2001
From: Geoffrey Martin-Noble <gcmn@google.com>
Date: Tue, 29 Mar 2022 14:41:48 -0700
Subject: [PATCH] Fix runfiles in native_binary/native_test (#339)

The current implementation misses the runfiles from the binary itself
since the attribute for it is called `src` not `srcs` and it makes use
of the discouraged `collect_data` and `collect_default` parameters which
depend on hardcoded attribute names.
---
 rules/native_binary.bzl           | 20 +++++++++++++++-----
 tests/native_binary/BUILD         | 24 +++++++++++++++++++-----
 tests/native_binary/assertarg.cc  |  7 +------
 tests/native_binary/assertdata.cc | 16 +++++++---------
 4 files changed, 42 insertions(+), 25 deletions(-)

diff --git a/rules/native_binary.bzl b/rules/native_binary.bzl
index 9a1725f..8899cd1 100644
--- a/rules/native_binary.bzl
+++ b/rules/native_binary.bzl
@@ -28,14 +28,24 @@ def _impl_rule(ctx, is_windows):
         copy_cmd(ctx, ctx.file.src, out)
     else:
         copy_bash(ctx, ctx.file.src, out)
+    runfiles = ctx.runfiles(files = ctx.files.data)
+
+    # Bazel 4.x LTS does not support `merge_all`.
+    # TODO: remove `merge` branch once we drop support for Bazel 4.x.
+    if hasattr(runfiles, "merge_all"):
+        runfiles = runfiles.merge_all([
+            d[DefaultInfo].default_runfiles
+            for d in ctx.attr.data + [ctx.attr.src]
+        ])
+    else:
+        for d in ctx.attr.data:
+            runfiles = runfiles.merge(d[DefaultInfo].default_runfiles)
+        runfiles = runfiles.merge(ctx.attr.src[DefaultInfo].default_runfiles)
+
     return DefaultInfo(
         executable = out,
         files = depset([out]),
-        runfiles = ctx.runfiles(
-            files = [out],
-            collect_data = True,
-            collect_default = True,
-        ),
+        runfiles = runfiles,
     )
 
 def _impl(ctx):
diff --git a/tests/native_binary/BUILD b/tests/native_binary/BUILD
index 852d607..0272770 100644
--- a/tests/native_binary/BUILD
+++ b/tests/native_binary/BUILD
@@ -21,6 +21,15 @@ cc_binary(
     # rule.
 )
 
+cc_binary(
+    name = "assertdata_with_runfiles",
+    srcs = ["assertdata.cc"],
+    # This version depends on runfiles directly, to ensure runfiles from the
+    # binary are picked up by native_test/native_binary
+    data = ["testdata.txt"],
+    deps = ["@bazel_tools//tools/cpp/runfiles"],
+)
+
 # A rule that copies "assertarg"'s output as an opaque executable, simulating a
 # binary that's not built from source and needs to be wrapped in native_binary.
 copy_file(
@@ -51,11 +60,7 @@ _ARGS = [
     "$(location testdata.txt) $$(location testdata.txt) $(location testdata.txt)",
     "'$(location testdata.txt) $$(location testdata.txt) $(location testdata.txt)'",
     "$$TEST_SRCDIR",
-
-    # TODO(laszlocsomor): uncomment this (and its counterpart in assertarg.cc)
-    # after https://github.com/bazelbuild/bazel/issues/6622 is resolved and the
-    # Bash-less test wrapper is the default on Windows.
-    # "$${TEST_SRCDIR}",
+    "$${TEST_SRCDIR}",
 ]
 
 native_binary(
@@ -101,3 +106,12 @@ native_test(
     out = "data_test.exe",
     data = ["testdata.txt"],
 )
+
+native_test(
+    name = "data_from_binary_test",
+    src = ":assertdata_with_runfiles",
+    # On Windows we need the ".exe" extension.
+    # On other platforms the extension doesn't matter.
+    # Therefore we can use ".exe" on every platform.
+    out = "data_from_binary_test.exe",
+)
diff --git a/tests/native_binary/assertarg.cc b/tests/native_binary/assertarg.cc
index 02cb0fd..6506635 100644
--- a/tests/native_binary/assertarg.cc
+++ b/tests/native_binary/assertarg.cc
@@ -25,12 +25,7 @@ int main(int argc, char** argv) {
     "tests/native_binary/testdata.txt",
     "tests/native_binary/testdata.txt $(location testdata.txt) tests/native_binary/testdata.txt",
     "$TEST_SRCDIR",
-
-    // TODO(laszlocsomor): uncomment this (and its counterpart in the BUILD
-    // file) after https://github.com/bazelbuild/bazel/issues/6622 is resolved
-    // and the Bash-less test wrapper is the default on Windows.
-    // "${TEST_SRCDIR}",
-
+    "${TEST_SRCDIR}",
     "",
   };
 
diff --git a/tests/native_binary/assertdata.cc b/tests/native_binary/assertdata.cc
index 111aaa2..6c49c68 100644
--- a/tests/native_binary/assertdata.cc
+++ b/tests/native_binary/assertdata.cc
@@ -21,7 +21,7 @@
 
 using bazel::tools::cpp::runfiles::Runfiles;
 
-int main(int argc, char** argv) {
+int main(int argc, char **argv) {
   std::string error;
   std::unique_ptr<Runfiles> runfiles(Runfiles::Create(argv[0], &error));
   if (runfiles == nullptr) {
@@ -30,17 +30,16 @@ int main(int argc, char** argv) {
     return 1;
   }
 
-  // Even though this cc_binary rule has no data-dependencies, the
-  // native_binary that wraps a copy of this binary does, so we have some
-  // runfiles.
+  // This should have runfiles, either from the binary itself or from the
+  // native_test.
   std::string path =
       runfiles->Rlocation("bazel_skylib/tests/native_binary/testdata.txt");
-  if (path.empty()) {
-    fprintf(stderr, "ERROR(" __FILE__ ":%d): Could not find runfile\n",
-            __LINE__);
+  FILE *f = fopen(path.c_str(), "rt");
+  if (!f) {
+    fprintf(stderr, "ERROR(" __FILE__ ":%d): Could not find runfile '%s'\n",
+            __LINE__, path.c_str());
   }
 
-  FILE* f = fopen(path.c_str(), "rt");
   char buf[6];
   size_t s = fread(buf, 1, 5, f);
   fclose(f);
@@ -53,4 +52,3 @@ int main(int argc, char** argv) {
 
   return 0;
 }
-