Initial support for pkg_* rules as srcs of pkg_tar (#367)

* Initial support for pkg_* rules as srcs of pkg_tar

1. Create helper methods to turn srcs of type Pkg*Info into a manifest file that can drive pkg_tar.
2. Test generation of the manifest using those rules
3. Use it in pkg_tar
4. Change compute_data_path to use ctx as an input instead of
   output_file. This is important refactoring for a future step where we can
   move the loop which creates the manifest from srcs to a helper method
   that all the pkg_* rules can share. I can break that out to a cleanup
   first.
5. Add support for tree artifacts in the manifest and pkg_tar.

Design points:
- Do we disallow srcs with pkg_files to be mixed with remap in pkg_tar?
- Does the pf test strategy, by looking at the generated manifest seem right?

Next steps:
- Now that we don't pass --files or --empty_dirs to build_tar, eliminate
  them from build_tar. Not done in this PR to keep it smaller.
- we may need to add the concept of uid/gid to Package*Info along with
  username/group name. This is for tar, which can keep both. We do not
  have to solve this before moving on.

pkg/distro/BUILD

@@ -98,13 +98,14 @@ genrule(
 bzl_library(
     name = "rules_pkg_lib",
     srcs = [
-        "//private:util.bzl",
         "//:package_variables.bzl",
         "//:path.bzl",
         "//:pkg.bzl",
         "//:providers.bzl",
         "//:rpm.bzl",
         "//:version.bzl",
+        "//private:pkg_files.bzl",
+        "//private:util.bzl",
     ],
     visibility = ["//visibility:private"],
 )

pkg/pkg.bzl

@@ -14,8 +14,22 @@
 """Rules for manipulation of various packaging."""
 
 load(":path.bzl", "compute_data_path", "dest_path")
-load(":providers.bzl", "PackageArtifactInfo", "PackageVariablesInfo")
+load(
+    ":providers.bzl",
+    "PackageArtifactInfo",
+    "PackageFilegroupInfo",
+    "PackageFilesInfo",
+    "PackageVariablesInfo",
+)
 load("//private:util.bzl", "setup_output_files", "substitute_package_variables")
+load(
+    "//private:pkg_files.bzl",
+    "add_directory",
+    "add_label_list",
+    "add_single_file",
+    "add_tree_artifact",
+    "process_src",
+    "write_manifest")
 
 # TODO(aiuto): Figure  out how to get this from the python toolchain.
 # See check for lzma in archive.py for a hint at a method.
@@ -103,33 +117,69 @@ def _pkg_tar_impl(ctx):
     if ctx.attr.portable_mtime:
         args.append("--mtime=portable")
 
+    # Now we begin processing the files.
+    file_deps = []  # inputs we depend on
+    content_map = {}  # content handled in the manifest
+
+    # Start with all the pkg_* inputs
+    for src in ctx.attr.srcs:
+        # Gather the files for every srcs entry here, even if it is not from
+        # a pkg_* rule.
+        if DefaultInfo in src:
+            file_deps.append(src[DefaultInfo].files)
+        if not process_src(
+            content_map,
+            src,
+            src.label,
+            default_mode = None,
+            default_user = None,
+            default_group = None,
+        ):
+            # Add in the files of srcs which are not pkg_* types
+            for f in src.files.to_list():
+                d_path = dest_path(f, data_path, data_path_without_prefix)
+                if f.is_directory:
+                    # Tree artifacts need a name, but the name is never really
+                    # the important part. The likely behavior people want is
+                    # just the content, so we strip the directory name.
+                    dest = '/'.join(d_path.split('/')[0:-1])
+                    add_tree_artifact(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(content_map, 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(ctx, content_map, file_deps, ctx.attr.srcs, transform_path)
+
     # Add runfiles if requested
-    file_inputs = []
+    runfiles_depsets = []
     if ctx.attr.include_runfiles:
-        runfiles_depsets = []
+        # TODO(#339): Rethink this w.r.t. binaries in pkg_files() rules.
         for f in ctx.attr.srcs:
             default_runfiles = f[DefaultInfo].default_runfiles
             if default_runfiles != None:
                 runfiles_depsets.append(default_runfiles.files)
 
-        # deduplicates files in srcs attribute and their runfiles
-        file_inputs = depset(ctx.files.srcs, transitive = runfiles_depsets).to_list()
-    else:
-        file_inputs = ctx.files.srcs[:]
-
-    args += [
-        "--file=%s=%s" % (_quote(f.path), _remap(
-            remap_paths,
-            dest_path(f, data_path, data_path_without_prefix),
-        ))
-        for f in file_inputs
-    ]
+    # The files attribute is a map of labels to destinations. We can add them
+    # directly to the content map.
     for target, f_dest_path in ctx.attr.files.items():
         target_files = target.files.to_list()
         if len(target_files) != 1:
             fail("Each input must describe exactly one file.", attr = "files")
-        file_inputs += target_files
-        args += ["--file=%s=%s" % (_quote(target_files[0].path), f_dest_path)]
+        file_deps.append(depset([target_files[0]]))
+        add_single_file(
+            content_map,
+            f_dest_path,
+            target_files[0],
+            target.label,
+        )
+
     if ctx.attr.modes:
         args += [
             "--modes=%s=%s" % (_quote(key), ctx.attr.modes[key])
@@ -147,8 +197,8 @@ def _pkg_tar_impl(ctx):
         ]
     if ctx.attr.empty_files:
         args += ["--empty_file=%s" % empty_file for empty_file in ctx.attr.empty_files]
-    if ctx.attr.empty_dirs:
-        args += ["--empty_dir=%s" % empty_dir for empty_dir in ctx.attr.empty_dirs]
+    for empty_dir in ctx.attr.empty_dirs or []:
+        add_directory(content_map, empty_dir, ctx.label)
     args += ["--tar=" + f.path for f in ctx.files.deps]
     args += [
         "--link=%s:%s" % (_quote(k, protect = ":"), ctx.attr.symlinks[k])
@@ -158,14 +208,21 @@ def _pkg_tar_impl(ctx):
                                ctx.attr.private_stamp_detect):
         args.append("--stamp_from=%s" % ctx.version_file.path)
         files.append(ctx.version_file)
+
+    file_inputs = depset(transitive = file_deps + runfiles_depsets)
+    manifest_file = ctx.actions.declare_file(ctx.label.name + ".manifest")
+    files.append(manifest_file)
+    write_manifest(ctx, manifest_file, content_map)
+    args.append("--manifest=%s" % manifest_file.path)
+
     arg_file = ctx.actions.declare_file(ctx.label.name + ".args")
     files.append(arg_file)
     ctx.actions.write(arg_file, "\n".join(args))
 
     ctx.actions.run(
         mnemonic = "PackageTar",
         progress_message = "Writing: %s" % output_file.path,
-        inputs = file_inputs + ctx.files.deps + files,
+        inputs = file_inputs.to_list() + ctx.files.deps + files,
         tools = [ctx.executable.compressor] if ctx.executable.compressor else [],
         executable = ctx.executable.build_tar,
         arguments = ["@" + arg_file.path],

pkg/private/build_tar.py

@@ -23,6 +23,12 @@
 import helpers
 import build_info
 
+# These must be kept in sync with the values from private/pkg_files.bzl
+ENTRY_IS_FILE = 0  # Entry is a file: take content from <src>
+ENTRY_IS_LINK = 1  # Entry is a symlink: dest -> <src>
+ENTRY_IS_DIR = 2  # Entry is an empty dir
+ENTRY_IS_TREE = 3  # Entry is a tree artifact: take tree from <src>
+
 
 class TarFile(object):
   """A class to generates a TAR file."""
@@ -57,7 +63,7 @@ def add_file(self, f, destfile, mode=None, ids=None, names=None):
     Args:
        f: the file to add to the layer
        destfile: the name of the file in the layer
-       mode: force to set the specified mode, by default the value from the
+       mode: (int) force to set the specified mode, by default the value from the
          source is taken.
        ids: (uid, gid) for the file to set ownership
        names: (username, groupname) for the file to set ownership. `f` will be
@@ -197,6 +203,92 @@ def add_deb(self, deb):
       self.add_tar(tmpfile[1])
       os.remove(tmpfile[1])
 
+  def add_tree(self, tree_top, destpath, mode=None, ids=None, names=None):
+    """Add a tree artifact to the tar file.
+
+    Args:
+       tree_top: the top of the tree to add
+       destpath: the path under which to place the files
+       mode: (int) force to set the specified posix mode (e.g. 0o755). The
+         default is derived from the source
+       ids: (uid, gid) for the file to set ownership
+       names: (username, groupname) for the file to set ownership. `f` will be
+         copied to `self.directory/destfile` in the layer.
+    """
+    dest = destpath.strip('/')  # redundant, dests should never have / here
+    if self.directory and self.directory != '/':
+      dest = self.directory.lstrip('/') + '/' + dest
+
+    dest = os.path.normpath(dest).replace(os.path.sep, '/')
+    if ids is None:
+      ids = (0, 0)
+    if names is None:
+      names = ('', '')
+
+    to_write = {}
+    for root, dirs, files in os.walk(tree_top):
+      dirs = sorted(dirs)
+      rel_path_from_top = root[len(tree_top):].lstrip('/')
+      if rel_path_from_top:
+        dest_dir = dest + '/' + rel_path_from_top + '/'
+      else:
+        dest_dir = dest + '/'
+      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)
+
+    for path in sorted(to_write.keys()):
+      content_path = to_write[path]
+      # If mode is unspecified, derive the mode from the file's mode.
+      if mode is None:
+        f_mode = 0o755 if os.access(content_path, os.X_OK) else 0o644
+      else:
+        f_mode = mode
+      if not content_path:
+        self.add_empty_file(
+            path,
+            mode=f_mode,
+            ids=ids,
+            names=names,
+            kind=tarfile.DIRTYPE)
+      else:
+        self.tarfile.add_file(
+            path,
+            file_content=content_path,
+            mode=f_mode,
+            uid=ids[0],
+            gid=ids[1],
+            uname=names[0],
+            gname=names[1])
+
+  def add_manifest_entry(self, entry, file_attributes):
+    entry_type, dest, src, mode, user, group = entry
+
+    # Use the pkg_tar mode/owner remaping as a fallback
+    non_abs_path = dest.strip('/')
+    if file_attributes:
+      attrs = file_attributes(non_abs_path)
+    else:
+      attrs = {}
+    # But any attributes from the manifest have higher precedence
+    if mode is not None and mode != '':
+      attrs['mode'] = int(mode, 8)
+    if user:
+      if group:
+        attrs['names'] = (user, group)
+      else:
+        # Use group that legacy tar process would assign
+        attrs['names'] = (user, attrs.get('names')[1])
+    if entry_type == ENTRY_IS_LINK:
+      self.add_link(dest, src)
+    elif entry_type == ENTRY_IS_DIR:
+      self.add_empty_dir(dest, **attrs)
+    elif entry_type == ENTRY_IS_TREE:
+      self.add_tree(src, dest, **attrs)
+    else:
+      self.add_file(src, dest, **attrs)
+
 
 def main():
   parser = argparse.ArgumentParser(
@@ -207,7 +299,9 @@ def main():
   parser.add_argument('--file', action='append',
                       help='A file to add to the layer.')
   parser.add_argument('--manifest',
-                      help='JSON manifest of contents to add to the layer.')
+                      help='manifest of contents to add to the layer.')
+  parser.add_argument('--legacy_manifest',
+                      help='DEPRECATED: JSON manifest of contents to add to the layer.')
   parser.add_argument('--mode',
                       help='Force the mode on the added files (in octal).')
   parser.add_argument(
@@ -325,8 +419,9 @@ def file_attributes(filename):
           'names': names_map.get(filename, default_ownername),
       }
 
-    if options.manifest:
-      with open(options.manifest, 'r') as manifest_fp:
+    # TODO(aiuto): Make sure this is unused and remove the code.
+    if options.legacy_manifest:
+      with open(options.legacy_manifest, 'r') as manifest_fp:
         manifest = json.load(manifest_fp)
         for f in manifest.get('files', []):
           output.add_file(f['src'], f['dst'], **file_attributes(f['dst']))
@@ -343,6 +438,12 @@ def file_attributes(filename):
         for deb in manifest.get('debs', []):
           output.add_deb(deb)
 
+    if options.manifest:
+      with open(options.manifest, 'r') as manifest_fp:
+        manifest = json.load(manifest_fp)
+        for entry in manifest:
+          output.add_manifest_entry(entry, file_attributes)
+
     for f in options.file or []:
       (inf, tof) = helpers.SplitNameValuePairAtSeparator(f, '=')
       output.add_file(inf, tof, **file_attributes(tof))