From 5c6aec64627c91f8ae71820173766237311d4ac6 Mon Sep 17 00:00:00 2001 From: elsk Date: Fri, 27 Sep 2024 18:27:52 -0700 Subject: [PATCH] install: Add --wipe_destdir option (#894) * install: test uses pathlib. Cleans up a lot of code! * install: add --wipe_destdir option. If specified, wipe destination directory before installing. Fixes #893. * install: Update doc for --wipe_destdir. Clarify that this will delete the whole directory. --------- Co-authored-by: HONG Yifan --- pkg/private/install.py.tpl | 15 +++++++-- tests/install/test.py | 62 +++++++++++++++++++++++--------------- 2 files changed, 50 insertions(+), 27 deletions(-) diff --git a/pkg/private/install.py.tpl b/pkg/private/install.py.tpl index b6c276be..93c39677 100644 --- a/pkg/private/install.py.tpl +++ b/pkg/private/install.py.tpl @@ -53,10 +53,12 @@ RUNFILE_PREFIX = os.path.join(os.getenv("RUNFILES_DIR"), WORKSPACE_NAME) if os.g # # See also https://bugs.python.org/issue37157. class NativeInstaller(object): - def __init__(self, default_user=None, default_group=None, destdir=None): + def __init__(self, default_user=None, default_group=None, destdir=None, + wipe_destdir=False): self.default_user = default_user self.default_group = default_group self.destdir = destdir + self.wipe_destdir = wipe_destdir self.entries = [] # Logger helper method, may not be necessary or desired @@ -171,6 +173,9 @@ class NativeInstaller(object): def do_the_thing(self): logging.info("Installing to %s", self.destdir) + if self.wipe_destdir: + logging.debug("RM %s", self.destdir) + shutil.rmtree(self.destdir, ignore_errors=True) for entry in self.entries: if entry.type == manifest.ENTRY_IS_FILE: self._install_file(entry) @@ -236,6 +241,9 @@ def main(args): f"Relative paths are interpreted against " f"BUILD_WORKSPACE_DIRECTORY " f"({os.getenv('BUILD_WORKSPACE_DIRECTORY')})") + parser.add_argument("--wipe_destdir", action="store_true", default=False, + help="Delete destdir tree (including destdir itself) " + "before installing") args = parser.parse_args() @@ -251,7 +259,10 @@ def main(args): level=level, format="%(levelname)s: %(message)s" ) - installer = NativeInstaller(destdir=args.destdir) + installer = NativeInstaller( + destdir=args.destdir, + wipe_destdir=args.wipe_destdir, + ) if not CALLED_FROM_BAZEL_RUN and RUNFILE_PREFIX is None: logging.critical("RUNFILES_DIR must be set in your environment when this is run as a bazel build tool.") diff --git a/tests/install/test.py b/tests/install/test.py index 3ee2562f..56490b80 100644 --- a/tests/install/test.py +++ b/tests/install/test.py @@ -14,8 +14,8 @@ # See the License for the specific language governing permissions and # limitations under the License. -import itertools import os +import pathlib import unittest import stat import subprocess @@ -24,7 +24,7 @@ from pkg.private import manifest -class PkgInstallTest(unittest.TestCase): +class PkgInstallTestBase(unittest.TestCase): @classmethod def setUpClass(cls): cls.runfiles = runfiles.Create() @@ -36,8 +36,14 @@ def setUpClass(cls): cls.manifest_data = {} for entry in manifest_entries: - cls.manifest_data[entry.dest] = entry - cls.installdir = os.path.join(os.getenv("TEST_TMPDIR"), "installdir") + cls.manifest_data[pathlib.Path(entry.dest)] = entry + cls.installdir = pathlib.Path(os.getenv("TEST_TMPDIR")) / "installdir" + + +class PkgInstallTest(PkgInstallTestBase): + @classmethod + def setUpClass(cls): + super().setUpClass() env = {} env.update(cls.runfiles.EnvVars()) subprocess.check_call([ @@ -48,11 +54,11 @@ def setUpClass(cls): env=env) def entity_type_at_path(self, path): - if os.path.islink(path): + if path.is_symlink(): return manifest.ENTRY_IS_LINK - elif os.path.isfile(path): + elif path.is_file(): return manifest.ENTRY_IS_FILE - elif os.path.isdir(path): + elif path.is_dir(): return manifest.ENTRY_IS_DIR else: # We can't infer what TreeArtifacts are by looking at them -- the @@ -97,10 +103,13 @@ def assertEntryModeMatches(self, entry, actual_path, def _find_tree_entry(self, path, owned_trees): for tree_root in owned_trees: - if path == tree_root or path.startswith(tree_root + "/"): + if self._path_starts_with(path, tree_root): return tree_root return None + def _path_starts_with(self, path, other): + return path.parts[:len(other.parts)] == other.parts + def test_manifest_matches(self): unowned_dirs = set() owned_dirs = set() @@ -119,28 +128,17 @@ def test_manifest_matches(self): 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. - # This may not be the best thing. - unowned_dirs.update([p for p in itertools.accumulate(os.path.dirname(dest).split('/'), - func=lambda accum, new: accum + '/' + new)]) + unowned_dirs.update(dest.parents) # In the above loop, unowned_dirs contains all possible directories that # are in the manifest. Prune them here. unowned_dirs -= owned_dirs # TODO: check for ownership (user, group) - found_entries = {dest: False for dest in self.manifest_data.keys()} + found_entries = {dest: False for dest in self.manifest_data} for root, dirs, files in os.walk(self.installdir): - rel_root_path = os.path.relpath(root, self.installdir) - - # The rest of this uses string comparison. To reduce potential - # confusion, ensure that the "." doesn't show up elsewhere. - # - # TODO(nacl) consider using pathlib here, which will reduce the - # need for path cleverness. - if rel_root_path == '.': - rel_root_path = '' + root = pathlib.Path(root) + rel_root_path = root.relative_to(self.installdir) # Directory ownership tests if len(files) == 0 and len(dirs) == 0: @@ -182,10 +180,10 @@ def test_manifest_matches(self): # needed. It maybe worth setting the keys in the manifest_data # dictionary to pathlib.Path or otherwise converting them to # native paths. - fpath = os.path.normpath("/".join([root, f])) + fpath = root / f # The path inside the manifest (relative to the install # destdir). - rel_fpath = os.path.normpath("/".join([rel_root_path, f])) + rel_fpath = rel_root_path / f 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: @@ -211,5 +209,19 @@ def test_manifest_matches(self): self.assertEqual(num_missing, 0) +class WipeTest(PkgInstallTestBase): + def test_wipe(self): + self.installdir.mkdir(exist_ok=True) + (self.installdir / "should_be_deleted.txt").touch() + + subprocess.check_call([ + self.runfiles.Rlocation("rules_pkg/tests/install/test_installer"), + "--destdir", self.installdir, + "--wipe_destdir", + ], + env=self.runfiles.EnvVars()) + self.assertFalse((self.installdir / "should_be_deleted.txt").exists()) + + if __name__ == "__main__": unittest.main()