Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

view_copy: relocate symlinks #32306

Merged
merged 2 commits into from
May 9, 2023
Merged

Conversation

haampie
Copy link
Member

@haampie haampie commented Aug 22, 2022

fixes #33606
fixes #19550

Previously spack:view:default:link_type:copy did not handle symlinks:
it changed symlinks into redundant file copies, errored on dangling
links, and failed at symlinked directories.

With this commit it actually creates symlinks, and relocates them if
necessary. However, it's far from perfect (doesn't properly check if a
symlink points into a prefix dir in multiple ways: relative symlinks
aren't checked whether they escape the prefix, and absolute symlinks are
not checked in realpath sense). But held to the standard of
Spack's current relocation logic nobody can complain...

Supersedes #31958

@spackbot-app spackbot-app bot added the core PR affects Spack core functionality label Aug 22, 2022
@haampie haampie requested a review from becker33 August 22, 2022 12:42
@haampie
Copy link
Member Author

haampie commented Aug 22, 2022

@becker33 this might need a proper refactor, since some valuable information is lost when entering view_copy. The directory visitor has much more data (relative paths, whether src is a symlinked file or symlinked dir, ...), the view_copy just receives two abs paths, which is cumbersome.

@vsoch
Copy link
Member

vsoch commented Aug 22, 2022

Yay! Thanks @haampie!

Copy link
Member

@becker33 becker33 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One request to update a comment, otherwise LGTM.

Oh also an entire conversation with myself about inaccurate old comments, which I'm leaving here in case it's useful for anyone, but should not hold up the PR.

lib/spack/spack/filesystem_view.py Outdated Show resolved Hide resolved
lib/spack/spack/filesystem_view.py Outdated Show resolved Hide resolved
lib/spack/spack/filesystem_view.py Outdated Show resolved Hide resolved
@haampie haampie requested a review from becker33 August 30, 2022 13:03
@vsoch
Copy link
Member

vsoch commented Sep 8, 2022

heyo! Just want to make sure we don't forget this one - it's blocking us from building a view of flux in our container bases PR. Thanks!

@haampie
Copy link
Member Author

haampie commented Sep 15, 2022

@becker33?

@haampie
Copy link
Member Author

haampie commented Oct 3, 2022

bump @becker33...

@haampie
Copy link
Member Author

haampie commented Oct 30, 2022

bump @becker33, a month later... can you please review or assign someone else? Thanks.

@tgamblin tgamblin added this to the v0.19.1 milestone Nov 9, 2022
@haampie haampie modified the milestones: v0.19.1, v0.19.2 Feb 9, 2023
@alalazo alalazo modified the milestones: v0.19.2, v0.19.3 Apr 4, 2023
@alalazo
Copy link
Member

alalazo commented May 3, 2023

@becker33 @haampie What is the status of this? Should we merge it or is it superseded by renameat2?

Copy link
Member

@alalazo alalazo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we come up with unit tests and briefly describe the various cases we test there? I think that would be valuable for future refactoring. Other than that, this basically LGTM.

lib/spack/spack/filesystem_view.py Outdated Show resolved Hide resolved
lib/spack/spack/filesystem_view.py Outdated Show resolved Hide resolved
@alalazo alalazo self-assigned this May 5, 2023
lib/spack/spack/filesystem_view.py Outdated Show resolved Hide resolved
lib/spack/spack/filesystem_view.py Outdated Show resolved Hide resolved
lib/spack/spack/filesystem_view.py Show resolved Hide resolved
@alalazo alalazo dismissed becker33’s stale review May 9, 2023 08:50

Review comments have been addressed

@alalazo alalazo merged commit 9e1440e into spack:develop May 9, 2023
32 of 33 checks passed
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Copy view still not working Symbolic links in binarycache bundles need to be relocated.
5 participants