I'm finding this difficult to understand. Reading it out: "src_a and src_b are the same file, so use src_b"... (In my head: "how can this matter?") - I think this works for #40773 (comment) because

• we traverse from parent to child
• we visit python second (after python-venv), which is a file vs. a symlink
• we overwrite with that (our src_b)
• when it comes time to view_copy, it's not a symlink to a relative .../bin/python so it doesn't relocate the link

So reversing the order of traversals would recreate #40773 (comment). However unlikely, it seems we might as well make this robust to that.

I'm inclined to say this should explicitly resolve the realpath of either, then make that the new self.files[proj_rel_path] (and if they are both not symlinks, there should be no need to overwrite, although I think that cannot happen).

Other option: pick whichever one is a file, and use that

So reversing the order of traversals would recreate #40773 (comment). However unlikely, it seems we might as well make this robust to that.

The test_env_view_ignores_different_file_conflicts test would fail, so it's robust already.

I'm inclined to say this should explicitly resolve the realpath of either, then make that the new self.files[proj_rel_path]

That would be completely wrong for multiple reasons, realpath would be a ... real mistake

1. realpath(x) may not exist, causing link failure later
2. It can cause redundant file copies in copy-type views where we can copy a symlink in a chain like <prefix A>/x -> <prefix B>/x -> <prefix C>/y.
3. If <prefix A>/x -> <prefix B>/x -> /system/x -> /system/y, and /system/x changes, it would not be reflected in the view, breaking it. E.g. curl -> openssl certs=system -> /etc/ssl/certs/* or so. (For internal spack symlinks i'm assuming prefixes are immutable).
4. src_root has to be the prefix of the current Spack package (but that may be an implementation detail)

Other option: pick whichever one is a file, and use that

First, that doesn't work when both are links. Second, realpath triggers O(depth + chain length) readlink calls which I want to avoid, mine is just two stat calls. This prefix scan is supposed to be fast.

So, let's stick to samefile.

[Another pitfall with realpath is you'd need to worry about case insensitive file systems, but to be fair that's another bug already present anyways, since normcase is missing when keying self.files]

Other option: pick whichever one is a file, and use that

First, that doesn't work when both are links.

If they're both links, it should be fine to use either

Second, realpath triggers O(depth + chain length) readlink calls

Could we lstat the files to determine whether they are links (which I think would involve fewer calls than realpath):

if samefile() and xor(islink(x) for x in src_a, src_b):
  # one of these is a symlink to the other, use the original file

?

As a general question: Doesn't samefile have to do the same thing as realpath? How could samefile work if it's not chasing down the link targets (transitively)?

The visitor already has visit_symlink.

If they're both links, it should be fine to use either

Then why suggest to distinguish instead of leaving it at os.samefile?

Doesn't samefile have to do the same thing as realpath? How could samefile work if it's not chasing down the link targets (transitively)?

The kernel can avoid chasing through file system caches you can't have in userland, cause there you don't know how to invalidate them. But also it's about not paying syscall overhead, and slow python loops.

The visitor already has visit_symlink.

In that case the check would be:

if samefile() and islink(src_a):
  # src_a is a symlink to src_b, which is a file: use the file.
  # We do this because src_a would become self-referential
  # if relocated relative to the link_tree's root

I think this comment would be useful compared to

            # Resolve file-file conflicts when two files are identical: this deals with cases of
            # of symlink to symlink, symlink to regular file and hardlinked files, where in
            # principle the file we pick is arbitrary. However, for environment views that copy,
            # we want the file to be copied, not the symlink, so we pick the underlying file, which
            # should be the current, assuming proper ordering.

(the former gets into the why, and avoids caring about the cases we don't need to change)

Then why suggest to distinguish instead of leaving it at os.samefile

Doing something that doesn't need to be done (i.e. replacing with src_b when we don't need to) is confusing in this case: it obfuscates the reasons for doing it.

I'm open to other approaches to making this less confusing (e.g. the comment I suggested above would help).
Perhaps (also) putting this in the docstring for SourceMergeVisitor (and LinkTree.merge) would help: test_source_merge_visitor_does_not_register_identical_file_conflicts enforces that behavior for the tests, but the behavior of merge for these types of issues seems like something that ought to be explicit in the method's description.

To be honest I don't think islink(src_a) improves things that much,

This is convoluted and I'm trying to find any way I can to make it less so. If you think this makes it even the slightest bit clearer, then it's worthwhile. Without checking islink the comment I suggested is imprecise and therefore less useful.

Zooming out, doing this in add_files_to_view would have been more clear because the concerns would have been addressed precisely where they arise. Another mechanism would be for example registering a conflict-resolution callback with the visitor that views could add their own implementation to. There's plenty of angles that avoid the requirement for discussing actions that only make sense for isolated users of this, so we could also consider those if it's impossible to make this particular approach more clear.

cause you still need to explain the assumption on order: dependents before dependencies, symlinks only go from dependent -> dependency.

I thought the islink(src_a) part of if samefile and islink(src_a) would make it work in both directions (we only replace with src_b if src_b is the file.

would make it work in both directions

True. I've done the reverse now since that information is directly available, and I can't care so much about the hardlink case (neither about the symlink case, but well): if current file clashes, but identical, and regular file, take it.

If you care about the hardlink case too, feel free to submit a follow-up pr.

From what I knew, self.files is a {}, which does not have ordering guarantees

Apparently, for Python >= 3.7, it is ordered. But we support older versions (https://spack.readthedocs.io/en/latest/getting_started.html#python says 3.2+ but our changelog and https://spack.readthedocs.io/en/latest/getting_started.html#system-prerequisites indicate 3.6+).

Either way, if we depend on ordering, we'll want an OrderedDict

Thanks, I thought that was an issue of the past.

Edit: dicts are ordered in cpython 3.6, it's just that that was set in stone from 3.7 onwards. Do we care? (I asked on Slack, looks like "no")

link_tree has existed for a while and used dict, and didn't depend on ordering. So being careful here seems to be introducing new API guarantees. I'm inclined to say that:

• For now don't explicitly del, or
• Add a comment here about how we want to improve link_tree's behavior to do an ordered traversal and we are leaning on the (implementation-specific) details of python dictionaries

The order is relied on in SimpleFilesystemView for about two years. This PR does not introduce new behavior or API. The del is necessary, if you remove it a test fails.

If you are concerned about this detail, can't you deal with it in a separate PR? In principle this PR can be backported as a bugfix, I don't want to lay out a plan of future improvements in comments, cause I don't think it needs to be, I'd rather leave that to you since you seem concerned about it.

The del is necessary, if you remove it a test fails.

Are you saying tests other than the ones added in this PR would fail if this del+reinsert logic was just an override?

Or that the logic in general would fail if this overwrite-without-delete occurred (because perhaps the dirs need to be made before the files in them)?

I'm not sure how either would be a problem: if it was ok to have src_a at this point, it seems like it's always OK to have src_b inserted at the same time.

Files are later grouped by prefix. Group by assumes consecutive files from same prefix.

OK, how about we just augment the comment to point to this function (_source_merge_visitor_to_merge_map). Ideally we'd also update the docstring for SourceMergeVisitor to make this official. IMO this is also a good cautionary tale against reaching into the members of objects.

I want it to be clear we depend on preservation of order as an internal implementation detail. Ideally we could have a _replace_file_item function that codifies this internal constraint.

A comment like # src_b in tuple form would help

Given that we are bound to internal implementation details IMO we might as well point at the exact culprit instead of hinting at it, which is why I suggested mentioning _source_merge_visitor_to_merge_map explicitly (a reader of this code may be curious about this, so they look for groupby in this file, maybe followed by group_by and maybe it occurs to them to look outside of the file, or maybe not).

I would recommend modifying:

Remove the link in favor of the actual file (src_b). The del is necessary to maintain the...

resolving what "the actual file" means requires reading from the beginning of the function.