Bring tar runfiles up to feature parity with pkg_files.runfiles. (#754)

* Bring tar runfiles up to feature parity with pkg_files.runfiles. The bulk of the change is to have pkg_tar use pkg_files%add_label_list to process `srcs`. That brings it in conformance with pkg_files and pkg_zip. This changes the behavior of runfiles to be useful, but it is a breaking change. Fixes: #153 Fixes: #579 Other bits: - do not fail verify_archive_test without a min_size or max_size - change `data` file in the sample executable from BUILD to foo.cc so that it is easier to disambiguate from other files. - fix bug in runfiles support where files were not added to file_deps list. - fix bug in runfiles support where the runfiles base was not computed correctly. - some buildifier fixes to make the linters happy.
bazelbuild · Nov 28, 2023 · 1788b97 · bcsgh · Dec 31, 2023 · aiuto
1 parent 8b85c37
commit 1788b97
Show file tree

Hide file tree

Showing 9 changed files with 123 additions and 100 deletions.
diff --git a/docs/latest.md b/docs/latest.md
diff --git a/pkg/private/pkg_files.bzl b/pkg/private/pkg_files.bzl
@@ -75,6 +75,8 @@ _MappingContext = provider(
         "include_runfiles": "bool: include runfiles",
         "strip_prefix": "strip_prefix",
 
+        "path_mapper": "function to map destination paths",
+
         # Defaults
         "default_mode": "Default mode to apply to file without a mode setting",
         "default_user": "Default user name to apply to file without a user",
@@ -91,7 +93,9 @@ def create_mapping_context_from_ctx(
         allow_duplicates_with_different_content = None,
         strip_prefix = None,
         include_runfiles = None,
-        default_mode = None):
+        default_mode = None,
+        path_mapper = None
+    ):
     """Construct a MappingContext.
 
     Args: See the provider definition.
@@ -116,6 +120,7 @@ def create_mapping_context_from_ctx(
         strip_prefix = strip_prefix,
         include_runfiles = include_runfiles,
         default_mode = default_mode,
+        path_mapper = path_mapper or (lambda x: x),
         # TODO(aiuto): allow these to be passed in as needed. But, before doing
         # that, explore defauilt_uid/gid as 0 rather than None
         default_user = "",
@@ -340,7 +345,7 @@ def add_label_list(mapping_context, srcs):
 
     Args:
       mapping_context: (r/w) a MappingContext
-      srcs: List of source objects.
+      srcs: List of source objects
     """
 
     # Compute the relative path
@@ -390,12 +395,13 @@ def add_from_default_info(
     the_executable = get_my_executable(src)
     all_files = src[DefaultInfo].files.to_list()
     for f in all_files:
-        d_path = dest_path(f, data_path, data_path_without_prefix)
+        d_path = mapping_context.path_mapper(
+            dest_path(f, data_path, data_path_without_prefix))
         if f.is_directory:
             add_tree_artifact(
                 mapping_context.content_map,
-                d_path,
-                f,
+                dest_path = d_path,
+                src = f,
                 origin = src.label,
                 mode = mapping_context.default_mode,
                 user = mapping_context.default_user,
@@ -415,7 +421,16 @@ def add_from_default_info(
     if include_runfiles:
         runfiles = src[DefaultInfo].default_runfiles
         if runfiles:
-            base_path = d_path + ".runfiles"
+            mapping_context.file_deps.append(runfiles.files)
+
+            # Computing the runfiles root is subtle. It should be based off of
+            # the executable, but that is not always obvious. When in doubt,
+            # the first file of DefaultInfo.files should be the right target.
+            base_file = the_executable or all_files[0]
+            base_file_path = mapping_context.path_mapper(
+                dest_path(base_file, data_path, data_path_without_prefix))
+            base_path = base_file_path + ".runfiles"
+
             for rf in runfiles.files.to_list():
                 d_path = base_path + "/" + rf.short_path
                 fmode = "0755" if rf == the_executable else mapping_context.default_mode

diff --git a/pkg/private/tar/tar.bzl b/pkg/private/tar/tar.bzl
@@ -13,17 +13,15 @@
 # limitations under the License.
 """Rules for making .tar files."""
 
-load("//pkg:path.bzl", "compute_data_path", "dest_path")
 load("//pkg:providers.bzl", "PackageVariablesInfo")
 load(
     "//pkg/private:pkg_files.bzl",
     "add_directory",
     "add_empty_file",
+    "add_label_list",
     "add_single_file",
     "add_symlink",
-    "add_tree_artifact",
     "create_mapping_context_from_ctx",
-    "process_src",
     "write_manifest",
 )
 load("//pkg/private:util.bzl", "setup_output_files", "substitute_package_variables")
@@ -58,16 +56,8 @@ def _pkg_tar_impl(ctx):
 
     # Files needed by rule implementation at runtime
     files = []
-
     outputs, output_file, _ = setup_output_files(ctx)
 
-    # Compute the relative path
-    data_path = compute_data_path(ctx.label, ctx.attr.strip_prefix)
-    data_path_without_prefix = compute_data_path(ctx.label, ".")
-
-    # Find a list of path remappings to apply.
-    remap_paths = ctx.attr.remap_paths
-
     # Start building the arguments.
     args = ctx.actions.args()
     args.add("--output", output_file.path)
@@ -112,53 +102,36 @@ def _pkg_tar_impl(ctx):
         args.add("--mtime", "%d" % ctx.attr.mtime)
     if ctx.attr.portable_mtime:
         args.add("--mtime", "portable")
+    if ctx.attr.modes:
+        for key in ctx.attr.modes:
+            args.add("--modes", "%s=%s" % (_quote(key), ctx.attr.modes[key]))
+    if ctx.attr.owners:
+        for key in ctx.attr.owners:
+            args.add("--owners", "%s=%s" % (_quote(key), ctx.attr.owners[key]))
+    if ctx.attr.ownernames:
+        for key in ctx.attr.ownernames:
+            args.add(
+                "--owner_names",
+                "%s=%s" % (_quote(key), ctx.attr.ownernames[key]),
+            )
 
     # Now we begin processing the files.
+    path_mapper = None
+    if ctx.attr.remap_paths:
+        path_mapper = lambda path: _remap(ctx.attr.remap_paths, path)
+
     mapping_context = create_mapping_context_from_ctx(
         ctx,
         label = ctx.label,
-        include_runfiles = False,  # TODO(aiuto): ctx.attr.include_runfiles,
+        include_runfiles = ctx.attr.include_runfiles,
         strip_prefix = ctx.attr.strip_prefix,
-        default_mode = ctx.attr.mode,
+        # build_tar does the default modes. Consider moving attribute mapping
+        # into mapping_context.
+        default_mode = None,
+        path_mapper = path_mapper,
     )
 
-    # Start with all the pkg_* inputs
-    for src in ctx.attr.srcs:
-        if not process_src(
-            mapping_context,
-            src = src,
-            origin = src.label,
-        ):
-            src_files = src[DefaultInfo].files.to_list()
-            if ctx.attr.include_runfiles:
-                runfiles = src[DefaultInfo].default_runfiles
-                if runfiles:
-                    mapping_context.file_deps.append(runfiles.files)
-                    src_files.extend(runfiles.files.to_list())
-
-            # Add in the files of srcs which are not pkg_* types
-            for f in src_files:
-                d_path = dest_path(f, data_path, data_path_without_prefix)
-
-                # Note: This extra remap is the bottleneck preventing this
-                # large block from being a utility method as shown below.
-                # Should we disallow mixing pkg_files in srcs with remap?
-                # I am fine with that if it makes the code more readable.
-                dest = _remap(remap_paths, d_path)
-                if f.is_directory:
-                    add_tree_artifact(mapping_context.content_map, dest, f, src.label)
-                else:
-                    # Note: This extra remap is the bottleneck preventing this
-                    # large block from being a utility method as shown below.
-                    # Should we disallow mixing pkg_files in srcs with remap?
-                    # I am fine with that if it makes the code more readable.
-                    dest = _remap(remap_paths, d_path)
-                    add_single_file(mapping_context, dest, f, src.label)
-
-    # TODO(aiuto): I want the code to look like this, but we don't have lambdas.
-    # transform_path = lambda f: _remap(
-    #    remap_paths, dest_path(f, data_path, data_path_without_prefix))
-    # add_label_list(mapping_context, ctx.attr.srcs, transform_path)
+    add_label_list(mapping_context, srcs = ctx.attr.srcs)
 
     # The files attribute is a map of labels to destinations. We can add them
     # directly to the content map.
@@ -174,18 +147,6 @@ def _pkg_tar_impl(ctx):
             target.label,
         )
 
-    if ctx.attr.modes:
-        for key in ctx.attr.modes:
-            args.add("--modes", "%s=%s" % (_quote(key), ctx.attr.modes[key]))
-    if ctx.attr.owners:
-        for key in ctx.attr.owners:
-            args.add("--owners", "%s=%s" % (_quote(key), ctx.attr.owners[key]))
-    if ctx.attr.ownernames:
-        for key in ctx.attr.ownernames:
-            args.add(
-                "--owner_names",
-                "%s=%s" % (_quote(key), ctx.attr.ownernames[key]),
-            )
     for empty_file in ctx.attr.empty_files:
         add_empty_file(mapping_context, empty_file, ctx.label)
     for empty_dir in ctx.attr.empty_dirs or []:
@@ -289,7 +250,10 @@ pkg_tar_impl = rule(
         "extension": attr.string(default = "tar"),
         "symlinks": attr.string_dict(),
         "empty_files": attr.string_list(),
-        "include_runfiles": attr.bool(),
+        "include_runfiles": attr.bool(
+            doc = ("""Include runfiles for executables. These appear as they would in bazel-bin."""
+                   + """For example: 'path/to/myprog.runfiles/path/to/my_data.txt'."""),
+        ),
         "empty_dirs": attr.string_list(),
         "remap_paths": attr.string_dict(),
         "compressor": attr.label(

diff --git a/pkg/verify_archive_test_main.py.tpl b/pkg/verify_archive_test_main.py.tpl
@@ -50,6 +50,8 @@ class VerifyArchiveTest(unittest.TestCase):
     Args:
         min_size: The minimum number of targets we expect.
     """
+    if min_size <= 0:
+      return
     actual_size = len(self.paths)
     self.assertGreaterEqual(
         len(self.paths),
@@ -63,7 +65,7 @@ class VerifyArchiveTest(unittest.TestCase):
     Args:
         max_size: The maximum number of targets we expect.
     """
-    if max_size < 0:
+    if max_size <= 0:
       return
     actual_size = len(self.paths)
     self.assertLessEqual(
@@ -78,7 +80,7 @@ class VerifyArchiveTest(unittest.TestCase):
       if path in plain_patterns:
         plain_patterns.remove(path)
     if len(plain_patterns) > 0:
-      self.fail('These required paths were not found: %s' % ','.join(plain_patterns))
+      self.fail('These required paths were not found: %s' % ','.join(plain_patterns) + ' in [%s]' % ','.join(self.paths))
 
   def check_must_not_contain(self, must_not_contain):
     plain_patterns = set(must_not_contain)

diff --git a/tests/BUILD b/tests/BUILD
@@ -116,7 +116,7 @@ cc_library(
 cc_binary(
     name = "an_executable",
     srcs = ["foo.cc"],
-    data = ["BUILD"],
+    data = ["foo.cc"],
     deps = [
         ":liba",
         ":libb",

diff --git a/tests/mappings/executable.manifest.golden b/tests/mappings/executable.manifest.golden
@@ -1,5 +1,5 @@
 [
-{"dest":"an_executable.runfiles/tests/BUILD","gid":null,"group":null,"mode":"","origin":"@//tests:an_executable","src":"tests/BUILD","type":"file","uid":null,"user":null},
+{"dest":"an_executable.runfiles/tests/foo.cc","gid":null,"group":null,"mode":"","origin":"@//tests:an_executable","src":"tests/foo.cc","type":"file","uid":null,"user":null},
 {"dest":"an_executable.runfiles/tests/an_executable","gid":null,"group":null,"mode":"0755","origin":"@//tests:an_executable","src":"tests/an_executable","type":"file","uid":null,"user":null},
 {"dest":"an_executable.runfiles/tests/testdata/hello.txt","gid":null,"group":null,"mode":"","origin":"@//tests:an_executable","src":"tests/testdata/hello.txt","type":"file","uid":null,"user":null},
 {"dest":"an_executable","gid":null,"group":null,"mode":"0755","origin":"@//tests:an_executable","src":"tests/an_executable","type":"file","uid":null,"user":null},

diff --git a/tests/mappings/executable.manifest.windows.golden b/tests/mappings/executable.manifest.windows.golden
@@ -1,5 +1,5 @@
 [
-{"dest":"an_executable.exe.runfiles/tests/BUILD","gid":null,"group":null,"mode":"","origin":"@//tests:an_executable","src":"tests/BUILD","type":"file","uid":null,"user":null},
+{"dest":"an_executable.exe.runfiles/tests/foo.cc","gid":null,"group":null,"mode":"","origin":"@//tests:an_executable","src":"tests/foo.cc","type":"file","uid":null,"user":null},
 {"dest":"an_executable.exe.runfiles/tests/an_executable.exe","gid":null,"group":null,"mode":"0755","origin":"@//tests:an_executable","src":"tests/an_executable.exe","type":"file","uid":null,"user":null},
 {"dest":"an_executable.exe.runfiles/tests/testdata/hello.txt","gid":null,"group":null,"mode":"","origin":"@//tests:an_executable","src":"tests/testdata/hello.txt","type":"file","uid":null,"user":null},
 {"dest":"an_executable.exe","gid":null,"group":null,"mode":"0755","origin":"@//tests:an_executable","src":"tests/an_executable.exe","type":"file","uid":null,"user":null},

diff --git a/tests/tar/BUILD b/tests/tar/BUILD
@@ -325,16 +325,41 @@ fake_artifact(
     name = "a_program",
     files = ["//tests:testdata/executable.sh"],
     runfiles = ["BUILD"],
+    executable = True,
 )
 
 pkg_tar(
     name = "test-tar-with-runfiles",
     srcs = [
         ":a_program",
+        "//tests:an_executable",
     ],
     include_runfiles = True,
 )
 
+verify_archive_test(
+    name = "runfiles_test",
+    target = ":test-tar-with-runfiles",
+    must_contain = [
+        "a_program",
+        "a_program.runfiles/tests/tar/BUILD",
+        "executable.sh",
+    ] + select({
+        "@platforms//os:windows": [
+            "an_executable.exe",
+            "an_executable.exe.runfiles/tests/foo.cc",
+            "an_executable.exe.runfiles/tests/an_executable.exe",
+            "an_executable.exe.runfiles/tests/testdata/hello.txt",
+        ],
+        "//conditions:default": [
+            "an_executable",
+            "an_executable.runfiles/tests/foo.cc",
+            "an_executable.runfiles/tests/an_executable",
+            "an_executable.runfiles/tests/testdata/hello.txt",
+        ]
+    }),
+)
+
 pkg_tar(
     name = "test_tar_leading_dotslash",
     srcs = [
@@ -366,7 +391,6 @@ py_test(
         ":test-tar-strip_prefix-substring.tar",
         ":test-tar-tree-artifact",
         ":test-tar-tree-artifact-noroot",
-        ":test-tar-with-runfiles",
         ":test-tree-input-with-strip-prefix",
         ":test_tar_leading_dotslash",
         ":test_tar_package_dir_substitution.tar",

diff --git a/tests/tar/pkg_tar_test.py b/tests/tar/pkg_tar_test.py
@@ -244,14 +244,6 @@ def test_tar_with_tree_artifact(self):
     self.assertTarFileContent('test-tar-tree-artifact-noroot.tar',
                               noroot_content)
 
-  def test_tar_with_runfiles(self):
-    content = [
-      {'name': 'BUILD' },
-      {'name': 'a_program' },
-      {'name': 'executable.sh' },
-    ]
-    self.assertTarFileContent('test-tar-with-runfiles.tar', content)
-
   def test_tar_leading_dotslash(self):
     content = [
       {'name': './loremipsum.txt'},