Feature: add option to create view by copying/relocating files#16480
Feature: add option to create view by copying/relocating files#16480
Conversation
2d0c818 to
34c12bf
Compare
247010e to
20b030e
Compare
|
Closing-bouncing to restart tests. |
alalazo
left a comment
There was a problem hiding this comment.
Mostly LGTM. The only requested change is that spack view remove doesn't fail on relocated trees. Also left a few other style comments, let me know if you disagree with anything.
lib/spack/spack/filesystem_view.py
Outdated
| def view_copy(src, dst, view, spec): | ||
| shutil.copyfile(src, dst) | ||
| if spec: | ||
| # Not metadata, we have to relocate it | ||
|
|
||
| # What type of file are we relocating | ||
| relocate_method = spack.relocate.relocate_text_bin \ | ||
| if spack.relocate.is_binary(dst) else spack.relocate.relocate_text | ||
|
|
||
| # Get information on where to relocate from/to | ||
| prefix_to_prefix = dict( | ||
| (dep.prefix, view.get_projection_for_spec(dep)) | ||
| for dep in spec.traverse() | ||
| ) | ||
|
|
||
| # Call actual relocation method | ||
| relocate_method( | ||
| [dst], spack.store.layout.root, view._root, | ||
| spec.prefix, view.get_projection_for_spec(spec), | ||
| spack.paths.spack_root, view._root, prefix_to_prefix | ||
| ) |
There was a problem hiding this comment.
Relocation might take some time. Since this function doesn't prompt anything up to completion, relocating a package might give users the feeling Spack is hanging:
$ time spack view relocate ~/tmp/spack-relocate netlib-scalapack
real 0m25,616s
user 0m18,356s
sys 0m7,839s
lib/spack/spack/filesystem_view.py
Outdated
| def view_link_wrapper(view, link_method): | ||
| def view_link(src, dst, spec=None): | ||
| link_method(src, dst, view, spec) | ||
|
|
||
| return view_link |
There was a problem hiding this comment.
We can probably use functools.partial instead of this wrapper.
There was a problem hiding this comment.
This proved tricky, because of the way the wrapper was setting the default for the spec argument (which can't be set in the function signature, because view is a mandatory argument.
I couldn't swap the order, because then the keyword argument view comes before the argument spec once I call functools.partial.
The only thing that worked was to make both into keyword arguments. I'm perfectly okay with that, and I've done so.
| @pytest.mark.parametrize('cmd', ['hardlink', 'symlink', 'hard', 'add', | ||
| 'copy', 'relocate']) |
There was a problem hiding this comment.
Can we rework the test to receive an additional parametrized value is_link:
| @pytest.mark.parametrize('cmd', ['hardlink', 'symlink', 'hard', 'add', | |
| 'copy', 'relocate']) | |
| @pytest.mark.parametrize('cmd,is_link', [ | |
| ('hardlink', False), | |
| ('symlink', True), | |
| ('hard', False), | |
| ('add', True), | |
| ('copy', False), | |
| ('relocate', False) | |
| ]) |
? In my opinion that would be clearer than (cmd in ('symlink', 'add'))
There was a problem hiding this comment.
I used a different method that I felt was cleaner to make this clearer, let me know if that works for you.
alalazo
left a comment
There was a problem hiding this comment.
Travis is failing because in the current implementation spec and view should be strings when used as keys for kwargs. That said, I think you can avoid having to use kwargs at all if you give default values to spec in the signature.
3c91c86 to
19e11cb
Compare
|
@alalazo had to rebase this to account for your changes to |
alalazo
left a comment
There was a problem hiding this comment.
The spack view copy command works as intended, and code LGTM. There are several issues with spack view remove that are very likely not related to this PR. I'll post a comment below the review, but imo we can merge this as it is and fix spack view remove in another PR.
|
I saw a couple of issues while trying the $ tree -L 2 spack-view
spack-view
├── copy
├── hardlink
└── symlink
$ spack view copy spack-view/copy hdf5
$ tree -L 2 spack-view
spack-view
├── copy
│ ├── bin
│ ├── include
│ ├── lib
│ └── share
├── hardlink
└── symlink
7 directories, 0 files
$ spack view remove -a spack-view/copy
$ tree -L 2 spack-view
spack-view
├── copy
│ ├── bin
│ └── lib
├── hardlink
└── symlinkThe second issue is that $ tree -L 2 spack-view
spack-view
├── copy
├── hardlink
└── symlink
$ spack view hardlink ~/tmp/spack-view/hardlink hdf5
$ tree -L 2 spack-view
spack-view
├── copy
├── hardlink
│ ├── bin
│ ├── include
│ ├── lib
│ └── share
└── symlink
$ spack view remove ~/tmp/spack-view/hardlink hdf5
==> Error: [Errno 2] No such file or directory: '/home/culpo/tmp/spack-view/hardlink/lib/libhdf5.so.103'
|
Views can currently be created by symlinks or hardlinks. In both cases, dependencies are silently used from the original location.
In deployment contexts, this can cause permissions problems if one user installs a package and creates a public view for other users.
The
spack view copysubcommand is an alternative that copies files into place, using logic fromlib/spack/spack/relocate.pyto edit the files in the new location to point to each other.TODO: