pkg_install: Support TreeArtifacts. (#885)

* install: Delete unused params from internal functions.

They are so confusing; I would've thought that because they
took the parameters, they would set the user/group for me.
But in fact, _chown_chmod needs to be called afterwards.

* pkg_install: Support TreeArtifacts.

Implementation notes:
For tree artifacts, when creating directories, we mostly follow the
modes set for the whole TreeArtifact, but also +x to allow searching
the directory. This is similar to how pkg_tar etc. handles things.

Link: #308

* pkg_install: Also add test for modes in TreeArtifacts.

---------

Co-authored-by: HONG Yifan <[email protected]>

pkg/private/install.py.tpl

@@ -74,11 +74,11 @@ class NativeInstaller(object):
                 logging.info("CHOWN %s:%s %s", user, group, dest)
                 shutil.chown(dest, user, group)
 
-    def _do_file_copy(self, src, dest, mode, user, group):
+    def _do_file_copy(self, src, dest):
         logging.info("COPY %s <- %s", dest, src)
         shutil.copyfile(src, dest)
 
-    def _do_mkdir(self, dirname, mode, user, group):
+    def _do_mkdir(self, dirname, mode):
         logging.info("MKDIR %s %s", mode, dirname)
         os.makedirs(dirname, int(mode, 8), exist_ok=True)
 
@@ -93,25 +93,56 @@ class NativeInstaller(object):
 
     def _install_file(self, entry):
         self._maybe_make_unowned_dir(os.path.dirname(entry.dest))
-        self._do_file_copy(entry.src, entry.dest, entry.mode, entry.user, entry.group)
+        self._do_file_copy(entry.src, entry.dest)
         self._chown_chmod(entry.dest, entry.mode, entry.user, entry.group)
 
     def _install_directory(self, entry):
         self._maybe_make_unowned_dir(os.path.dirname(entry.dest))
-        self._do_mkdir(entry.dest, entry.mode, entry.user, entry.group)
+        self._do_mkdir(entry.dest, entry.mode)
         self._chown_chmod(entry.dest, entry.mode, entry.user, entry.group)
 
+    def _install_treeartifact_file(self, entry, src, dst):
+        self._do_file_copy(src, dst)
+        self._chown_chmod(dst, entry.mode, entry.user, entry.group)
+
     def _install_treeartifact(self, entry):
         logging.info("COPYTREE %s <- %s/**", entry.dest, entry.src)
-        raise NotImplementedError("treeartifact installation not yet supported")
-        for root, dirs, files in os.walk(entry.src):
-            relative_installdir = os.path.join(entry.dest, root)
+        shutil.copytree(
+            src=entry.src,
+            dst=entry.dest,
+            copy_function=lambda s, d:
+                self._install_treeartifact_file(entry, s, d),
+            dirs_exist_ok=True,
+            # Bazel gives us a directory of symlinks, so we dereference it.
+            # TODO: Handle symlinks within the TreeArtifact. This is not yet
+            # tested for other rules (e.g.
+            # https://github.com/bazelbuild/rules_pkg/issues/750)
+            symlinks=False,
+            ignore_dangling_symlinks=True,
+        )
+
+        # Set mode/user/group for intermediate directories.
+        # Bazel has no API to specify modes for this, so the least surprising
+        # thing we can do is make it the canonical rwxr-xr-x
+        intermediate_dir_mode = "755"
+        for root, dirs, _ in os.walk(entry.src, topdown=False):
+            relative_installdir = os.path.join(entry.dest,
+                                               os.path.relpath(root, entry.src))
             for d in dirs:
-                self._maybe_make_unowned_dir(os.path.join(relative_installdir, d))
-
-            logging.info("COPY_FROM_TREE %s <- %s", entry.dest, entry.src)
-            logging.info("CHMOD %s %s", entry.mode, entry.dest)
-            logging.info("CHOWN %s:%s %s", entry.user, entry.group, entry.dest)
+                self._chown_chmod(os.path.join(relative_installdir, d),
+                                  intermediate_dir_mode,
+                                  entry.user, entry.group)
+
+        # For top-level directory, use entry.mode +r +x if specified, otherwise
+        # use least-surprising canonical rwxr-xr-x
+        top_dir_mode = entry.mode
+        if top_dir_mode:
+            top_dir_mode = int(top_dir_mode, 8)
+            top_dir_mode |= 0o555
+            top_dir_mode = oct(top_dir_mode).removeprefix("0o")
+        else:
+            top_dir_mode = "755"
+        self._chown_chmod(entry.dest, top_dir_mode, entry.user, entry.group)
 
     def _install_symlink(self, entry):
         raise NotImplementedError("symlinking not yet supported")

tests/install/BUILD

@@ -15,7 +15,7 @@
 load("@rules_python//python:defs.bzl", "py_test")
 load("//pkg:install.bzl", "pkg_install")
 load("//pkg:mappings.bzl", "pkg_attributes", "pkg_files", "pkg_mkdirs")
-load("//tests/util:defs.bzl", "fake_artifact")
+load("//tests/util:defs.bzl", "directory", "fake_artifact")
 
 package(default_applicable_licenses = ["//:license"])
 
@@ -47,6 +47,7 @@ pkg_install(
         ":artifact-in-owned-dir",
         ":artifact-in-unowned-dir",
         ":dirs",
+        ":generate_tree_pkg_files",
     ],
 )
 
@@ -75,3 +76,24 @@ pkg_mkdirs(
         "owned-dir",
     ],
 )
+
+directory(
+    name = "generate_tree",
+    contents = "hello there",
+    filenames = [
+        # buildifier: don't sort
+        "b/e",
+        "a/a",
+        "b/c/d",
+        "b/d",
+        "a/b/c",
+    ],
+)
+
+pkg_files(
+    name = "generate_tree_pkg_files",
+    srcs = [":generate_tree"],
+    attributes = pkg_attributes(
+        mode = "640",
+    ),
+)

tests/install/test.py

@@ -61,29 +61,50 @@ def entity_type_at_path(self, path):
 
     def assertEntryTypeMatches(self, entry, actual_path):
         actual_entry_type = self.entity_type_at_path(actual_path)
+
+        # TreeArtifacts looks like directories.
+        if (entry.type == manifest.ENTRY_IS_TREE and
+                actual_entry_type == manifest.ENTRY_IS_DIR):
+            return
+
         self.assertEqual(actual_entry_type, entry.type,
                         "Entity {} should be a {}, but was actually {}".format(
                             entry.dest,
                             manifest.entry_type_to_string(entry.type),
                             manifest.entry_type_to_string(actual_entry_type),
                         ))
 
-    def assertEntryModeMatches(self, entry, actual_path):
+    def assertEntryModeMatches(self, entry, actual_path,
+                               is_tree_artifact_content=False):
         # TODO: permissions in windows are... tricky.  Don't bother
         # testing for them if we're in it for the time being
         if os.name == 'nt':
             return
 
         actual_mode = stat.S_IMODE(os.stat(actual_path).st_mode)
         expected_mode = int(entry.mode, 8)
+
+        if (not is_tree_artifact_content and
+                entry.type == manifest.ENTRY_IS_TREE):
+            expected_mode |= 0o555
+
         self.assertEqual(actual_mode, expected_mode,
-                         "Entry {} has mode {:04o}, expected {:04o}".format(
-                            entry.dest, actual_mode, expected_mode,
-                        ))
+            "Entry {}{} has mode {:04o}, expected {:04o}".format(
+            entry.dest,
+            f" ({actual_path})" if is_tree_artifact_content else "",
+            actual_mode, expected_mode,
+        ))
+
+    def _find_tree_entry(self, path, owned_trees):
+        for tree_root in owned_trees:
+            if path == tree_root or path.startswith(tree_root + "/"):
+                return tree_root
+        return None
 
     def test_manifest_matches(self):
         unowned_dirs = set()
         owned_dirs = set()
+        owned_trees = dict()
 
         # Figure out what directories we are supposed to own, and which ones we
         # aren't.
@@ -95,6 +116,8 @@ def test_manifest_matches(self):
         for dest, data in self.manifest_data.items():
             if data.type == manifest.ENTRY_IS_DIR:
                 owned_dirs.add(dest)
+            elif data.type == manifest.ENTRY_IS_TREE:
+                owned_trees[dest] = data
 
             # TODO(nacl): The initial stage of the accumulation returns an empty string,
             # which end up in the set representing the root of the manifest.
@@ -119,9 +142,6 @@ def test_manifest_matches(self):
             if rel_root_path == '.':
                 rel_root_path = ''
 
-            # TODO(nacl): check for treeartifacts here.  If so, prune `dirs`,
-            # and set the rest aside for future processing.
-
             # Directory ownership tests
             if len(files) == 0 and len(dirs) == 0:
                 # Empty directories must be explicitly requested by something
@@ -145,7 +165,10 @@ def test_manifest_matches(self):
                 else:
                     # If any unowned directories are here, they must be the
                     # prefix of some entity in the manifest.
-                    self.assertIn(rel_root_path, unowned_dirs)
+                    is_unowned = rel_root_path in unowned_dirs
+                    is_tree_intermediate_dir = bool(
+                        self._find_tree_entry(rel_root_path, owned_trees))
+                    self.assertTrue(is_unowned or is_tree_intermediate_dir)
 
             for f in files:
                 # The path on the filesystem in which the file actually exists.
@@ -163,16 +186,22 @@ def test_manifest_matches(self):
                 # The path inside the manifest (relative to the install
                 # destdir).
                 rel_fpath = os.path.normpath("/".join([rel_root_path, f]))
-                if rel_fpath not in self.manifest_data:
+                entity_in_manifest = rel_fpath in self.manifest_data
+                entity_tree_root = self._find_tree_entry(rel_fpath, owned_trees)
+                if not entity_in_manifest and not entity_tree_root:
                     self.fail("Entity {} not in manifest".format(rel_fpath))
 
-                entry = self.manifest_data[rel_fpath]
-                self.assertEntryTypeMatches(entry, fpath)
-                self.assertEntryModeMatches(entry, fpath)
+                if entity_in_manifest:
+                    entry = self.manifest_data[rel_fpath]
+                    self.assertEntryTypeMatches(entry, fpath)
+                    self.assertEntryModeMatches(entry, fpath)
 
-                found_entries[rel_fpath] = True
+                if entity_tree_root:
+                    entry = owned_trees[entity_tree_root]
+                    self.assertEntryModeMatches(entry, fpath,
+                                                is_tree_artifact_content=True)
 
-        # TODO(nacl): check for TreeArtifacts
+                found_entries[rel_fpath] = True
 
         num_missing = 0
         for dest, present in found_entries.items():