Skip to content

filesystem: in recursive mtime, check only files that exist#32175

Merged
haampie merged 2 commits intospack:developfrom
wdconinc:patch-14
Aug 16, 2022
Merged

filesystem: in recursive mtime, check only files that exist#32175
haampie merged 2 commits intospack:developfrom
wdconinc:patch-14

Conversation

@wdconinc
Copy link
Copy Markdown
Contributor

When a develop path contains a dead symlink, the os.stat in the recursive mtime determination trips up over it. This ensures that the recursive mtime only stats existing files.

Closes #32165.

When a `develop` path contains a dead symlink, the `os.stat` in the recursive `mtime` determination trips up over it. This ensures that the recursive `mtime` only `stat`s existing files.

Closes spack#32165.
@spackbot-app spackbot-app bot added core PR affects Spack core functionality utilities labels Aug 16, 2022
@haampie
Copy link
Copy Markdown
Member

haampie commented Aug 16, 2022

You should use lstat instead of stat.

@wdconinc
Copy link
Copy Markdown
Contributor Author

Another 70 os.stat calls in lib/... if those should also be replaced.

@wdconinc wdconinc requested a review from haampie August 16, 2022 14:33
@haampie
Copy link
Copy Markdown
Member

haampie commented Aug 16, 2022

Another implementation is

class MaxMtimeVisitor(BaseDirectoryVisitor):
    def __init__(self):
        self.max_mtime = 0

    def visit_file(self, root, rel_path, depth):
        t = os.lstat(os.path.join(root, rel_path)).st_mtime
        if t > self.max_mtime:
            self.max_mtime = t

    def visit_symlinked_file(self, root, rel_path, depth):
        self.visit_file(root, rel_path, depth)

    def before_visit_dir(self, root, rel_path, depth):
        return True

    def before_visit_symlinked_dir(self, root, rel_path, depth):
        return False

def last_modification_time_recursive(path):
    visitor = MaxMtimeVisitor()
    visit_directory_tree(path, visitor)
    return visitor.max_mtime

If you wanna make it faster in general you could also consider skipping .gitignore patterns etc and use that in before_visit_dir ;p And there are probably other ways to make it early exit, such as stopping at the first file newer than the threshold.

@haampie haampie enabled auto-merge (squash) August 16, 2022 14:43
@wdconinc
Copy link
Copy Markdown
Contributor Author

If you wanna make it faster

I want it not to throw an error :-)

@haampie haampie merged commit 8f34a3a into spack:develop Aug 16, 2022
haampie pushed a commit that referenced this pull request Aug 17, 2022
* filesystem: use lstat in recursive mtime

When a `develop` path contains a dead symlink, the `os.stat` in the recursive `mtime` determination trips up over it.

Closes #32165.
haampie pushed a commit that referenced this pull request Aug 18, 2022
* filesystem: use lstat in recursive mtime

When a `develop` path contains a dead symlink, the `os.stat` in the recursive `mtime` determination trips up over it.

Closes #32165.
@haampie haampie mentioned this pull request Aug 18, 2022
12 tasks
ma595 pushed a commit to ma595/spack that referenced this pull request Sep 13, 2022
)

* filesystem: use lstat in recursive mtime

When a `develop` path contains a dead symlink, the `os.stat` in the recursive `mtime` determination trips up over it.

Closes spack#32165.
@wdconinc wdconinc deleted the patch-14 branch April 23, 2023 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core PR affects Spack core functionality utilities

Projects

No open projects

Development

Successfully merging this pull request may close these issues.

spack develop trips up over dead links

2 participants