Initial support for pkg_files* in pkg_zip (#373)

* Initial support for pkg_files* in pkg_zip

- This is a mostly minimal PR to switch to the newer style. That is, only plain files and directories.
- Followup CLs will add links and tree artifacts.

* merge #384

pkg/pkg.bzl

@@ -579,10 +579,40 @@ def _pkg_zip_impl(ctx):
         inputs.append(ctx.version_file)
 
     data_path = compute_data_path(ctx, ctx.attr.strip_prefix)
-    for f in ctx.files.srcs:
-        arg = "%s=%s" % (_quote(f.path), dest_path(f, data_path))
-        args.add(arg)
+    data_path_without_prefix = compute_data_path(ctx, ".")
 
+    content_map = {}  # content handled in the manifest
+    # TODO(aiuto): Refactor this loop out of pkg_tar and pkg_zip into a helper
+    # that both can use.
+    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:
+            inputs.extend(src[DefaultInfo].files.to_list())
+        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:
+                    add_single_file(content_map, d_path, f, src.label)
+
+    manifest_file = ctx.actions.declare_file(ctx.label.name + ".manifest")
+    inputs.append(manifest_file)
+    write_manifest(ctx, manifest_file, content_map)
+    args.add("--manifest", manifest_file.path)
     args.set_param_file_format("multiline")
     args.use_param_file("@%s")
 

pkg/private/BUILD

@@ -105,6 +105,7 @@ py_binary(
         ":archive",
         ":build_info",
         ":helpers",
+        ":manifest",
     ],
 )
 

pkg/private/build_zip.py

@@ -15,13 +15,18 @@
 
 import argparse
 import datetime
+import json
 import zipfile
 
 from rules_pkg.private import build_info
 from rules_pkg.private import helpers
+from rules_pkg.private import manifest
 
 ZIP_EPOCH = 315532800
 
+# Unix dir bit and Windows dir bit. Magic from zip spec
+UNIX_DIR_BIT = 0o40000
+MSDOS_DIR_BIT = 0x10
 
 def _create_argument_parser():
   """Creates the command line arg parser."""
@@ -41,6 +46,8 @@ def _create_argument_parser():
   parser.add_argument(
       '-m', '--mode',
       help='The file system mode to use for files added into the zip.')
+  parser.add_argument('--manifest',
+                      help='manifest of contents to add to the layer.')
   parser.add_argument(
       'files', type=str, nargs='*',
       help='Files to be added to the zip, in the form of {srcpath}={dstpath}.')
@@ -60,6 +67,48 @@ def parse_date(ts):
   ts = datetime.datetime.utcfromtimestamp(ts)
   return (ts.year, ts.month, ts.day, ts.hour, ts.minute, ts.second)
 
+def _add_manifest_entry(options, zip_file, entry, default_mode, ts):
+  """Add an entry to the zip file.
+
+  Args:
+    options: parsed options
+    zip_file: ZipFile to write to
+    entry: manifest entry
+    default_mode: (int) file mode to use if not specified in the entry.
+    ts: (int) time stamp to add to files
+  """
+
+  entry_type, dest, src, mode, user, group = entry
+
+  # Use the pkg_tar mode/owner remaping as a fallback
+  non_abs_path = dest.strip('/')
+  dst_path = _combine_paths(options.directory, non_abs_path)
+  if entry_type == manifest.ENTRY_IS_DIR and not dst_path.endswith('/'):
+    dst_path += '/'
+  entry_info = zipfile.ZipInfo(filename=dst_path, date_time=ts)
+  # See http://www.pkware.com/documents/casestudies/APPNOTE.TXT
+  # denotes UTF-8 encoded file name.
+  entry_info.flag_bits |= 0x800
+  if mode:
+    f_mode = int(mode, 8)
+  else:
+    f_mode = default_mode
+
+  # See: https://trac.edgewall.org/attachment/ticket/8919/ZipDownload.patch
+  # external_attr is 4 bytes in size. The high order two bytes represent UNIX
+  # permission and file type bits, while the low order two contain MS-DOS FAT file
+  # attributes.
+  entry_info.external_attr = f_mode << 16
+  if entry_type == manifest.ENTRY_IS_FILE:
+    entry_info.compress_type = zipfile.ZIP_DEFLATED
+    with open(src, 'rb') as src:
+      zip_file.writestr(entry_info, src.read())
+  elif entry_type == manifest.ENTRY_IS_DIR:
+    entry_info.compress_type = zipfile.ZIP_STORED
+    # Set directory bits
+    entry_info.external_attr |= (UNIX_DIR_BIT << 16) | MSDOS_DIR_BIT
+    zip_file.writestr(entry_info, '')
+  # TODO(#309): All the rest
 
 def main(args):
   unix_ts = max(ZIP_EPOCH, args.timestamp)
@@ -70,24 +119,13 @@ def main(args):
   if args.mode:
     default_mode = int(args.mode, 8)
 
-  with zipfile.ZipFile(args.output, 'w') as zip_file:
-    for f in args.files or []:
-      (src_path, dst_path) = helpers.SplitNameValuePairAtSeparator(f, '=')
+  with zipfile.ZipFile(args.output, mode='w') as zip_file:
+    if args.manifest:
+      with open(args.manifest, 'r') as manifest_fp:
+        manifest = json.load(manifest_fp)
+        for entry in manifest:
+          _add_manifest_entry(args, zip_file, entry, default_mode, ts)
 
-      dst_path = _combine_paths(args.directory, dst_path)
-
-      entry_info = zipfile.ZipInfo(filename=dst_path, date_time=ts)
-
-      if default_mode:
-        entry_info.external_attr = default_mode << 16
-
-      entry_info.compress_type = zipfile.ZIP_DEFLATED
-
-      # the zipfile library doesn't support adding a file by path with write()
-      # and specifying a ZipInfo at the same time.
-      with open(src_path, 'rb') as src:
-        data = src.read()
-        zip_file.writestr(entry_info, data)
 
 if __name__ == '__main__':
   arg_parser = _create_argument_parser()

pkg/tests/BUILD

@@ -13,6 +13,7 @@
 # limitations under the License.
 # -*- coding: utf-8 -*-
 
+load("//:mappings.bzl", "pkg_attributes", "pkg_mkdirs", "pkg_mklink",)
 load("//:pkg.bzl", "SUPPORTED_TAR_COMPRESSIONS", "pkg_deb", "pkg_tar", "pkg_zip")
 load("//tests/util:defs.bzl", "directory")
 load("@rules_python//python:defs.bzl", "py_test")
@@ -89,6 +90,18 @@ genrule(
     cmd = "for i in $(OUTS); do echo 1 >$$i; done",
 )
 
+pkg_mkdirs(
+    name = "dirs",
+    attributes = pkg_attributes(
+        group = "bar",
+        mode = "711",
+        user = "foo",
+    ),
+    dirs = [
+        "foodir",
+    ],
+)
+
 directory(
     name = "generate_tree",
     filenames = [
@@ -219,6 +232,7 @@ pkg_zip(
 pkg_zip(
     name = "test_zip_basic",
     srcs = [
+        ":dirs",
         "testdata/hello.txt",
         "testdata/loremipsum.txt",
     ],
@@ -246,6 +260,7 @@ pkg_zip(
 pkg_zip(
     name = "test_zip_basic_timestamp_before_epoch",
     srcs = [
+        ":dirs",
         "testdata/hello.txt",
         "testdata/loremipsum.txt",
     ],

pkg/tests/zip_test.py

@@ -23,6 +23,9 @@
 LOREM_CRC = 2178844372
 EXECUTABLE_CRC = 342626072
 
+# Unix dir bit and Windows dir bit. Magic from zip spec
+UNIX_DIR_BIT = 0o40000
+MSDOS_DIR_BIT = 0x10
 
 # The ZIP epoch date: (1980, 1, 1, 0, 0, 0)
 _ZIP_EPOCH_DT = datetime.datetime(1980, 1, 1, 0, 0, 0, tzinfo=datetime.timezone.utc)
@@ -64,17 +67,24 @@ def assertZipFileContent(self, zip_file, content):
 
       for info, expected in zip(infos, content):
         self.assertEqual(info.filename, expected["filename"])
-        self.assertEqual(info.CRC, expected["crc"])
+        if "crc" in expected:
+          self.assertEqual(info.CRC, expected["crc"])
 
         ts = seconds_to_ziptime(expected.get("timestamp", _ZIP_EPOCH_S))
         self.assertEqual(info.date_time, ts)
-        self.assertEqual(info.external_attr >> 16, expected.get("attr", 0o555))
+        if "isdir" in expected:
+          expect_dir_bits = UNIX_DIR_BIT << 16 | MSDOS_DIR_BIT
+          self.assertEqual(info.external_attr & expect_dir_bits,
+                           expect_dir_bits)
+        self.assertEqual(info.external_attr >> 16 & ~UNIX_DIR_BIT,
+                         expected.get("attr", 0o555))
 
   def test_empty(self):
     self.assertZipFileContent("test_zip_empty.zip", [])
 
   def test_basic(self):
     self.assertZipFileContent("test_zip_basic.zip", [
+        {"filename": "foodir/", "isdir": True, "attr": 0o711},
         {"filename": "hello.txt", "crc": HELLO_CRC},
         {"filename": "loremipsum.txt", "crc": LOREM_CRC},
     ])