manifest: Allow for TreeArtifacts in PackageFilesInfo (#421)

The package-independent JSON manifests currently do not consider whether a
"file" in a `PackageFilesInfo` provider is actually a file or a directory.  This
change provides this awareness.

This also fixes up some other (apparent) logic issues related to directories,
and additionally provides a test.  Since the tests now involve generated
TreeArtifacts, this change also forces the manifest generation tests to use the
manifest "short_path".

There may be a better way to do this, but it seems to work for now.

Additionally, this change fixes some Windows-specific path separator consistency
issues exposed in `pkg_tar` by the aforementioned change.  Thanks to
@beasleyr-vmw for investigating this.

Co-authored-by: Ryan Beasley <[email protected]>

pkg/private/build_tar.py

@@ -223,10 +223,14 @@ def add_tree(self, tree_top, destpath, mode=None, ids=None, names=None):
        names: (username, groupname) for the file to set ownership. `f` will be
          copied to `self.directory/destfile` in the layer.
     """
+    # We expect /-style paths.
+    tree_top = os.path.normpath(tree_top).replace(os.path.sep, '/')
+
     dest = destpath.strip('/')  # redundant, dests should never have / here
     if self.directory and self.directory != '/':
       dest = self.directory.lstrip('/') + '/' + dest
 
+    # Again, we expect /-style paths.
     dest = os.path.normpath(dest).replace(os.path.sep, '/')
     if ids is None:
       ids = (0, 0)
@@ -235,6 +239,10 @@ def add_tree(self, tree_top, destpath, mode=None, ids=None, names=None):
 
     to_write = {}
     for root, dirs, files in os.walk(tree_top):
+      # While `tree_top` uses '/' as a path separator, results returned by
+      # `os.walk` and `os.path.join` on Windows may not.
+      root = os.path.normpath(root).replace(os.path.sep, '/')
+
       dirs = sorted(dirs)
       rel_path_from_top = root[len(tree_top):].lstrip('/')
       if rel_path_from_top:
@@ -244,7 +252,7 @@ def add_tree(self, tree_top, destpath, mode=None, ids=None, names=None):
       for dir in dirs:
         to_write[dest_dir + dir] = None
       for file in sorted(files):
-        to_write[dest_dir + file] = os.path.join(root, file)
+        to_write[dest_dir + file] = root + '/' + file
 
     for path in sorted(to_write.keys()):
       content_path = to_write[path]

pkg/private/pkg_files.bzl

@@ -105,6 +105,7 @@ def _process_pkg_files(content_map, pkg_files_info, origin, default_mode, defaul
         _check_dest(content_map, dest, src, origin)
         content_map[dest] = _DestFile(
             src = src,
+            entry_type = ENTRY_IS_TREE if src.is_directory else ENTRY_IS_FILE,
             mode = attrs[0],
             user = attrs[1],
             group = attrs[2],
@@ -367,7 +368,7 @@ def _encode_manifest_entry(dest, df, use_short_path):
     entry_type = df.entry_type if hasattr(df, "entry_type") else ENTRY_IS_FILE
     if df.src:
         src = df.src.short_path if use_short_path else df.src.path
-        entry_type = ENTRY_IS_FILE
+        # entry_type is left as-is
     elif hasattr(df, "link_to"):
         src = df.link_to
         entry_type = ENTRY_IS_LINK

pkg/tests/mappings/BUILD

@@ -28,7 +28,7 @@ load(
     "pkg_mklink",
     "strip_prefix",
 )
-load("//tests/util:defs.bzl", "write_content_manifest")
+load("//tests/util:defs.bzl", "directory", "write_content_manifest")
 load("@rules_python//python:defs.bzl", "py_test")
 
 py_library(
@@ -46,6 +46,21 @@ mappings_analysis_tests()
 
 mappings_external_repo_analysis_tests()
 
+################################################################################
+# Packaging rule manifest tests
+################################################################################
+
+directory(
+    name = "treeartifact",
+    filenames = ["artifact_in_tree1", "artifact_in_tree2"],
+)
+
+pkg_files(
+    name = "directory-with-contents",
+    srcs = ["treeartifact"],
+    attributes = pkg_attributes(mode = "0644"),
+)
+
 pkg_mkdirs(
     name = "dirs",
     attributes = pkg_attributes(
@@ -65,12 +80,15 @@ pkg_files(
     ],
 )
 
+# TODO: this seems to write the JSON in a particular order; regardless of input
+# order.  Investigate when opportune, or make the test order-agnostic.
 write_content_manifest(
     name = "all",
     srcs = [
         "BUILD",
         ":dirs",
         ":files",
+        ":directory-with-contents",
     ],
 )
 

pkg/tests/mappings/all.manifest.golden

@@ -1,5 +1,6 @@
 [
 [0, "BUILD","tests/mappings/BUILD","",null,null],
 [2, "foodir",null,"711","foo","bar"],
-[0, "mappings_test.bzl","tests/mappings/mappings_test.bzl","0644",null,null]
+[0, "mappings_test.bzl","tests/mappings/mappings_test.bzl","0644",null,null],
+[3, "treeartifact","tests/mappings/treeartifact","0644",null,null]
 ]

pkg/tests/util/defs.bzl

@@ -105,7 +105,7 @@ def _write_content_manifest_impl(ctx):
     content_map = {}  # content handled in the manifest
     file_deps = []  # inputs we depend on
     add_label_list(ctx, content_map, file_deps, ctx.attr.srcs)
-    write_manifest(ctx, ctx.outputs.out, content_map)
+    write_manifest(ctx, ctx.outputs.out, content_map, use_short_path = ctx.attr.use_short_path)
 
 _write_content_manifest = rule(
     doc = """Helper rule to write the content manifest for a pkg_*.
@@ -118,11 +118,27 @@ This is intended only for testing the manifest creation features.""",
             allow_files = True,
         ),
         "out": attr.output(),
+        "use_short_path": attr.bool(
+            doc = """Use the rootless path in the manifest.
+
+            Useful to ensure that the platform-specific prefix (i.e. parts
+            including something like "x64_windows-fastbuild") isn't present in
+            paths in the manifest.
+
+            See also https://docs.bazel.build/versions/main/skylark/lib/File.html#path
+            """,
+            default = True,
+        ),
     },
 )
 
 def write_content_manifest(name, srcs):
-    _write_content_manifest(name = name, srcs = srcs, out = name + ".manifest")
+    _write_content_manifest(
+        name = name,
+        srcs = srcs,
+        use_short_path = True,
+        out = name + ".manifest"
+    )
 
 ############################################################
 # Test boilerplate