I'm concerned this test isn't fully exercising the possible failures related to symlink relocation because the binary cache is installed in the same location where it was created from. Ideally the test (or a new test) would choose a new location to install to. Ideally the test would also confirm that the symlinks point to where you expect (right now it just does the binary package and install).

I think you may be able to avoid some of the setup that occurs here by just checking binary_distribution.build_tarball.

Because the symlinks are changed into the workdir by install_tree, it's not possible to have the sort of hidden bug related to the final destination not changing that you're worried about here.

If any of the symlinks are still pointing to the workdir after one install, the subsequent create command will fail. This test found a bug when I added linking to it through exactly that failure mode.

I think your approach is sufficient for catching a number of bugs. Depending on the implementation though (or how it changes with future edits), I think it's possible that there are cases where unpacking into the same directory structure would succeed but unpacking into a different directory structure will fail. The test ensures that regardless of the implementation, the end desired result (successful relocation into a different root) succeeds.

The comment above needs an update -- you could work it into the branches of the if for more clarity

This is minor and bikesheddy but I really really prefer [] for this. I use list() when I want to create a list from a sequence but [] for new empty list.

I have the same preference, but I kept it this way to mirror what we already have elsewhere in this function. LMK if you prefer I change it.

this is minor, but when there are this many imports, I would just import spack.relocate as rel or something like that, and prefix the usages in the code with rel.

I generally prefer to import them directly, but only for flake line-length reasons. I can double check whether we'll run into line-length problems if I change this.

Should this only be done if rel == False? Otherwise when rel == True, all the symlinks will be relativized when the tarball is built (see make_package_relative in build_tarball) so they won't need to be relocated.

This is how make_package_relative knows which links to make relative. The same thing is done with RPATHS that are made relative.

Ah ok it was throwing me off that they were stored in the binary cache since the relative-izing was handled before creating it. However, there is a guard relocate_package which only uses the 'relocate_links' entry if rel == True. Even though it is entirely handled before creating the cache, it could still be worthwhile to keep 'relocate_links' around to indicate what was changed.