This is an automated email from the ASF dual-hosted git repository. tvb pushed a commit to branch tristan/virtual-directory-cleanup in repository https://gitbox.apache.org/repos/asf/buildstream.git
commit 650b34f1e2ecc5c2344cbedd26857ae467a9d0c0 Author: Tristan van Berkom <[email protected]> AuthorDate: Fri Mar 4 17:06:28 2022 +0900 storage/directory.py: Removed mark_unmodified/list_modified_paths This was much implementation plumbing just for the sake of showing which files were modified due to integration commands being run in the compose plugin. I think we're better off not carrying this weight in the new API unless there is a strong motivation to bring it back in the future. This removes a test case which was testing this functionality, along with the implementations in the cas/file directory backends. --- src/buildstream/plugins/elements/compose.py | 14 ++---- src/buildstream/storage/_casbaseddirectory.py | 69 +++----------------------- src/buildstream/storage/_filebaseddirectory.py | 21 +------- src/buildstream/storage/directory.py | 16 ------ tests/internals/storage.py | 19 ------- 5 files changed, 11 insertions(+), 128 deletions(-) diff --git a/src/buildstream/plugins/elements/compose.py b/src/buildstream/plugins/elements/compose.py index 36cf96c..40c2472 100644 --- a/src/buildstream/plugins/elements/compose.py +++ b/src/buildstream/plugins/elements/compose.py @@ -100,7 +100,6 @@ class ComposeElement(Element): # Make a snapshot of all the files. vbasedir = sandbox.get_virtual_directory() - modified_files = set() removed_files = set() added_files = set() @@ -112,16 +111,14 @@ class ComposeElement(Element): # Make a snapshot of all the files before integration-commands are run. snapshot = set(vbasedir.list_relative_paths()) - vbasedir.mark_unmodified() with sandbox.batch(): for dep in self.dependencies(): dep.integrate(sandbox) if require_split: - # Calculate added, modified and removed files + # Calculate added and removed files post_integration_snapshot = vbasedir.list_relative_paths() - modified_files = set(vbasedir.list_modified_paths()) basedir_contents = set(post_integration_snapshot) for path in manifest: if path in snapshot and path not in basedir_contents: @@ -130,19 +127,14 @@ class ComposeElement(Element): for path in basedir_contents: if path not in snapshot: added_files.add(path) - self.info( - "Integration modified {}, added {} and removed {} files".format( - len(modified_files), len(added_files), len(removed_files) - ) - ) + self.info("Integration added {} and removed {} files".format(len(added_files), len(removed_files))) # The remainder of this is expensive, make an early exit if # we're not being selective about what is to be included. if not require_split: return "/" - # Do we want to force include files which were modified by - # the integration commands, even if they were not added ? + # Update the manifest with files which were added/removed by integration commands # manifest.update(added_files) manifest.difference_update(removed_files) diff --git a/src/buildstream/storage/_casbaseddirectory.py b/src/buildstream/storage/_casbaseddirectory.py index 5130ded..dce706a 100644 --- a/src/buildstream/storage/_casbaseddirectory.py +++ b/src/buildstream/storage/_casbaseddirectory.py @@ -44,16 +44,7 @@ class IndexEntry: """ Directory entry used in CasBasedDirectory.index """ def __init__( - self, - name, - entrytype, - *, - digest=None, - target=None, - is_executable=False, - buildstream_object=None, - modified=False, - mtime=None + self, name, entrytype, *, digest=None, target=None, is_executable=False, buildstream_object=None, mtime=None ): self.name = name self.type = entrytype @@ -61,7 +52,6 @@ class IndexEntry: self.target = target self.is_executable = is_executable self.buildstream_object = buildstream_object - self.modified = modified self.mtime = mtime def get_directory(self, parent): @@ -218,7 +208,7 @@ class CasBasedDirectory(Directory): return newdir - def _add_file(self, name, path, modified=False, can_link=False, properties=None): + def _add_file(self, name, path, can_link=False, properties=None): digest = self.cas_cache.add_object(path=path) is_executable = os.access(path, os.X_OK) mtime = None @@ -226,14 +216,7 @@ class CasBasedDirectory(Directory): mtime = timestamp_pb2.Timestamp() utils._get_file_protobuf_mtimestamp(mtime, path) - entry = IndexEntry( - name, - _FileType.REGULAR_FILE, - digest=digest, - is_executable=is_executable, - modified=modified or name in self.index, - mtime=mtime, - ) + entry = IndexEntry(name, _FileType.REGULAR_FILE, digest=digest, is_executable=is_executable, mtime=mtime,) self.index[name] = entry self.__invalidate_digest() @@ -315,7 +298,7 @@ class CasBasedDirectory(Directory): self.__invalidate_digest() def _add_new_link_direct(self, name, target): - self.index[name] = IndexEntry(name, _FileType.SYMLINK, target=target, modified=name in self.index) + self.index[name] = IndexEntry(name, _FileType.SYMLINK, target=target) self.__invalidate_digest() @@ -503,7 +486,6 @@ class CasBasedDirectory(Directory): if self._check_replacement(name, relative_pathname, result): if entry.type == _FileType.REGULAR_FILE: self._add_entry(entry) - self.index[entry.name].modified = True else: assert entry.type == _FileType.SYMLINK self._add_new_link_direct(name=name, target=entry.target) @@ -548,10 +530,7 @@ class CasBasedDirectory(Directory): result = FileListResult() if self._check_replacement(os.path.basename(external_pathspec), os.path.dirname(external_pathspec), result): self._add_file( - os.path.basename(external_pathspec), - external_pathspec, - modified=os.path.basename(external_pathspec) in result.overwritten, - properties=properties, + os.path.basename(external_pathspec), external_pathspec, properties=properties, ) result.files_written.append(external_pathspec) return result @@ -616,30 +595,6 @@ class CasBasedDirectory(Directory): """ return len(self.index) == 0 - def _mark_directory_unmodified(self): - # Marks all entries in this directory and all child directories as unmodified. - for i in self.index.values(): - i.modified = False - if i.type == _FileType.DIRECTORY and i.buildstream_object: - i.buildstream_object._mark_directory_unmodified() - - def _mark_entry_unmodified(self, name): - # Marks an entry as unmodified. If the entry is a directory, it will - # recursively mark all its tree as unmodified. - self.index[name].modified = False - if self.index[name].buildstream_object: - self.index[name].buildstream_object._mark_directory_unmodified() - - def mark_unmodified(self): - """ Marks all files in this directory (recursively) as unmodified. - If we have a parent, we mark our own entry as unmodified in that parent's - index. - """ - if self.parent: - self.parent._mark_entry_unmodified(self._find_self_in_parent()) - else: - self._mark_directory_unmodified() - def _lightweight_resolve_to_index(self, path): """A lightweight function for transforming paths into IndexEntry objects. This does not follow symlinks. @@ -662,18 +617,6 @@ class CasBasedDirectory(Directory): return None return directory.index.get(path_components[-1], None) - def list_modified_paths(self): - """Provide a list of relative paths which have been modified since the - last call to mark_unmodified. - - Return value: List(str) - list of modified paths - """ - - for p in self.list_relative_paths(): - i = self._lightweight_resolve_to_index(p) - if i and i.modified: - yield p - def list_relative_paths(self): """Provide a list of all relative paths. @@ -801,7 +744,7 @@ class CasBasedDirectory(Directory): yield f # Import written temporary file into CAS f.flush() - subdir._add_file(path[-1], f.name, modified=True) + subdir._add_file(path[-1], f.name) def __str__(self): return "[CAS:{}]".format(self._get_identifier()) diff --git a/src/buildstream/storage/_filebaseddirectory.py b/src/buildstream/storage/_filebaseddirectory.py index 17d84ca..ab925a5 100644 --- a/src/buildstream/storage/_filebaseddirectory.py +++ b/src/buildstream/storage/_filebaseddirectory.py @@ -32,8 +32,8 @@ import stat from .directory import Directory, DirectoryError, _FileType from .. import utils -from ..utils import link_files, copy_files, list_relative_paths, _get_link_mtime, BST_ARBITRARY_TIMESTAMP -from ..utils import _set_deterministic_user, _set_deterministic_mtime +from ..utils import link_files, copy_files, list_relative_paths, BST_ARBITRARY_TIMESTAMP +from ..utils import _set_deterministic_user from ..utils import FileListResult # FileBasedDirectory intentionally doesn't call its superclass constuctor, @@ -203,23 +203,6 @@ class FileBasedDirectory(Directory): it = os.scandir(self.external_directory) return next(it, None) is None - def mark_unmodified(self): - """ Marks all files in this directory (recursively) as unmodified. - """ - _set_deterministic_mtime(self.external_directory) - - def list_modified_paths(self): - """Provide a list of relative paths which have been modified since the - last call to mark_unmodified. - - Return value: List(str) - list of modified paths - """ - return [ - f - for f in list_relative_paths(self.external_directory) - if _get_link_mtime(os.path.join(self.external_directory, f)) != BST_ARBITRARY_TIMESTAMP - ] - def list_relative_paths(self): """Provide a list of all relative paths. diff --git a/src/buildstream/storage/directory.py b/src/buildstream/storage/directory.py index 9edf42d..73bde46 100644 --- a/src/buildstream/storage/directory.py +++ b/src/buildstream/storage/directory.py @@ -157,22 +157,6 @@ class Directory: """ raise NotImplementedError() - def mark_unmodified(self): - """ Marks all files in this directory (recursively) as unmodified. - """ - raise NotImplementedError() - - def list_modified_paths(self): - """Provide a list of relative paths which have been modified since the - last call to mark_unmodified. Includes directories only if - they are empty. - - Yields: - (List(str)) - list of all modified files with relative paths. - - """ - raise NotImplementedError() - def list_relative_paths(self): """Provide a list of all relative paths in this directory. Includes directories only if they are empty. diff --git a/tests/internals/storage.py b/tests/internals/storage.py index d5a7159..3720e6d 100644 --- a/tests/internals/storage.py +++ b/tests/internals/storage.py @@ -45,25 +45,6 @@ def test_import(tmpdir, datafiles, backend): assert "bin/hello" in c.list_relative_paths() [email protected]("backend", [FileBasedDirectory, CasBasedDirectory]) [email protected](DATA_DIR) -def test_modified_file_list(tmpdir, datafiles, backend): - original = os.path.join(str(datafiles), "original") - overlay = os.path.join(str(datafiles), "overlay") - - with setup_backend(backend, str(tmpdir)) as c: - c.import_files(original) - - c.mark_unmodified() - - c.import_files(overlay) - - print("List of all paths in imported results: {}".format(c.list_relative_paths())) - assert "bin/bash" in c.list_relative_paths() - assert "bin/bash" in c.list_modified_paths() - assert "bin/hello" not in c.list_modified_paths() - - @pytest.mark.parametrize( "directories", [("merge-base", "merge-base"), ("empty", "empty"),], )
