buildEnv: Skip paths excluded by pathsToLink as soon as they are encountered. #60396
Conversation
…untered. The current code only checked at the end of recursive processing which paths should be excluded by pathsToLink. This resulted in false-positive collision warnings or errors for excluded paths.
|
Waiting for reviews before merging. |
|
I can hopefully have a look at this tomorrow, however I'd really wait for a review of at least either eelco or aszlig as I don't consider myself sufficiently qualified to approve changes in such a core component :) |
| my $len1 = length($path1); | ||
| my $len2 = length($path2); | ||
| if ($len1 == $len2) { | ||
| return $path1 eq $path2; |
There was a problem hiding this comment.
If you have /a/b/xxxxx and /a/b/c/xxx, the function will return false, but it from the description I'd imply it should return true, right?
There was a problem hiding this comment.
Correct me if I'm wrong, but if I see this correctly, this problem won't appear in our implementation as $path2 is always an element from $pathsToLink. So if $path1 is related to $path2 (the path to link) and has the same size as $path2, your scenario shouldn't appear, right? At least that's how I understood the code and interpreted Determine if a path is related to any of the paths in pathsToLink :)
There was a problem hiding this comment.
Given /a/b/xxxxx and /a/b/c/xxx, the function will indeed return false. And this is correct because the paths are not related, i.e. one is not above/below the other. See that they diverge at xxxxx and c. It is easy to see if you imagine it as path/graph. This is consistent with the description, which says the paths are related when "they are equal or one is below the other". These paths are not equal, and neither is below the other.
As for why this is the right behavior. Consider that pathsToLink is equal to [/a/b/c/xxx]. Paths related to this path (the processing of which will not be skipped) are:
- The paths above this path: "",
/a,/a/b,/a/b/c(for this case these are all such paths). - This path itself:
/a/b/c/xxx. - Paths below this path, e.g.
/a/b/c/xxx/y,/a/b/c/xxx/y/z... (there is an infinite number of these).
All of these related paths must not be skipped during recursive processing as that would result in skipping paths that must not be skipped since they are "covered" by pathsToLink. The first point is especially important because e.g. if you skip /a then you effectively also skip /a/b/c/xxx.
Paths that are not related to /a/b/c/xxx can safely be skipped during processing, such as /a/b/xxxxx. Just think about it, if you only want to include /a/b/c/xxx, then you do not want /a/b/xxxxx or anything under that.
@Ma27 Sorry I don't understand what you mean. As I explained, I don't see any incorrect behavior :)
There was a problem hiding this comment.
@Ma27 Sorry I don't understand what you mean. As I explained, I don't see any incorrect behavior :)
Well, I actually agree with you, I just didn't explain this as good as you did :D
I probably misinterpreted aszlig's comment as I guessed that with the xxx{,xx} being a file the function might be supposed to return true if the directory paths match and I tried to explain that this won't happen as $path2 is a directory path from pathsToLink.
So I guess we mean the same (a.k.a. that the current implementation is appropriate), my previous comment wasn't sufficiently well-phrased :)
There was a problem hiding this comment.
First of all sorry for not getting back to you for so long. I was pretty much swamped with notifications and only recently cleared all of them, which is why I got one now from the stale bot.
You're right, that in this case it doesn't really matter and what I should have probably emphasized on was that I was referring to the fact that both paths have the same length. In practice however as you said: It doesn't matter.
There was a problem hiding this comment.
So after having a look at the code and testing it locally, this seems fine for me. However we should resolve the comment of @aszlig first, as I'm not 100% sure if that case won't happen as I'm not 100% familiar with the buildEnv implementation :)
|
What's the conclusion? A fix is needed or can this go in? |
|
Thank you for your contributions.
|
stale
bot
added
the
2.status: stale
stale
bot
removed
the
2.status: stale
|
I marked this as stale due to inactivity. → More info |
stale
bot
added
the
2.status: stale
| my $len1 = length($path1); | ||
| my $len2 = length($path2); | ||
| if ($len1 == $len2) { | ||
| return $path1 eq $path2; |
There was a problem hiding this comment.
First of all sorry for not getting back to you for so long. I was pretty much swamped with notifications and only recently cleared all of them, which is why I got one now from the stale bot.
You're right, that in this case it doesn't really matter and what I should have probably emphasized on was that I was referring to the fact that both paths have the same length. In practice however as you said: It doesn't matter.
| return $path1 eq substr($path2, 0, $len1) && substr($path2, $len1, 1) eq "/"; | ||
| } else { | ||
| return $path2 eq substr($path1, 0, $len2) && substr($path1, $len2, 1) eq "/"; |
There was a problem hiding this comment.
I looked at this again and there is one corner case left: ('/a', '/') -> 0
| return $path1 eq substr($path2, 0, $len1) && substr($path2, $len1, 1) eq "/"; | |
| } else { | |
| return $path2 eq substr($path1, 0, $len2) && substr($path1, $len2, 1) eq "/"; | |
| return $path1 eq substr($path2, 0, $len1) && substr($path2, $len1 == 1 ? 0 : $len1, 1) eq "/"; | |
| } else { | |
| return $path2 eq substr($path1, 0, $len2) && substr($path1, $len2 == 1 ? 0 : $len2, 1) eq "/"; |
stale
bot
removed
the
2.status: stale
|
I marked this as stale due to inactivity. → More info |
stale
bot
added
the
2.status: stale
stale
bot
removed
2.status: stale
wegank
added
the
2.status: stale
|
What with the constant moving milestones on this? (It's already uh pretty stale) |
|
@nyabinary I'm not sure, I removed it from the 24.11 milestone since the release is soon and it looks like this won't be unstale anytime soon. |
stale
bot
removed
the
2.status: stale
|
Can you add some kind of test with assert that can be built and check each case that this should handle? Something like a derivation that brings two folders that have different conflict scenarios and a assert to check in the output if the right stuff happened. |
wegank
added
the
2.status: stale
wegank
added
the
2.status: merge conflict
stale
bot
removed
the
2.status: stale
The current code only checked at the end of recursive processing which paths should be excluded by pathsToLink. This resulted in false-positive collision warnings or errors for excluded paths.
Motivation for this change
False-positive collision warning from
nixos-rebuild(pathsToLinkexcludes/libexecin my configuration):collision between `/nix/store/wmxqm38g1y1y7sd7s9vg7an3klffaiyz-gnutar-1.31/libexec/rmt' and `/nix/store/yxxfkhl59hsy6x198r2df2xr7jd75i23-cpio-2.12/libexec/rmt'Things done
Tested with the NixOS system-path and checked that it produces the exact same result as before, but does not generate that collision warning.
sandboxinnix.confon non-NixOS)nix-shell -p nix-review --run "nix-review wip"./result/bin/)nix path-info -Sbefore and after)