Recognize circular links #3593
Recognize circular links #3593
Conversation
MDrakos
commented
Nov 14, 2024
•
MDrakos
commented
 DX-3146 Checkout fail for `ActiveStateCommunityTesting/Py-3.10.13-Data-Science`
|
github-actions
bot
changed the base branch from
master
to
version/0-47-0-RC1
internal/smartlink/smartlink.go
Outdated
| multilog.Error("Build contains a circular symlink: %s -> %s", filepath.Join(src, entry.Name()), dest) | ||
| continue |
There was a problem hiding this comment.
I think we should error out on this, unless BE has a solution? Continuing would just give you a corrupted runtime.
There was a problem hiding this comment.
BE considers this an error case and has patched it, but advised that it could happen again so we should guard against it.
I'm not sure how this comment works with your next one. We should error our if we encounter a circular link but also support it?
| func isCircularLink(src string) (bool, error) { | ||
| if !fileutils.IsSymlink(src) || !fileutils.IsDir(src) { | ||
| return false, nil | ||
| } | ||
|
|
||
| target, err := os.Readlink(src) | ||
| if err != nil { | ||
| return false, errs.Wrap(err, "could not read symlink %s", src) | ||
| } | ||
|
|
||
| if !filepath.IsAbs(target) { | ||
| resolved, err := fileutils.ResolveUniquePath(src) | ||
| if err != nil { | ||
| return false, errs.Wrap(err, "Could not resolve src path") | ||
| } | ||
| target = resolved | ||
| } | ||
|
|
||
| // If the target points at the parent directory, we have a circular link | ||
| if filepath.Dir(src) == target { | ||
| return true, nil | ||
| } | ||
|
|
||
| return false, nil | ||
| } |
There was a problem hiding this comment.
This doesn't feel right. It's totally valid to have a link like this:
- dir1/file1
- dir1/dir2 > ../dir1
The problem is that our Linking mechanism doesn't understand this and will continue to recurse forever. We need to fix that, whilst still supporting a link that goes back up the path.
There was a problem hiding this comment.
Since this library doesn't link directories I'm not sure how we can support this beyond what this PR is doing. If we encounter a circular directory we skip it, all of the contents of the directory will still be linked.
I've moved the check to make it more clear. I think that in the end, all we can do is ensure all of the files in the directories and subdirectories have been linked with the same layout as the source.
There was a problem hiding this comment.
I still feel like this is solving the wrong problem.
The problem isn't that the link points to itself. It works fine in the tar archive, so it should work fine on disk. We should not be verifying the validity of symlinks created by the builders.
What we should guard against is our linking mechanism hitting a recursion. Like in this case we should be able to tell that we already traversed the directory the symlink goes to. Heck, we probably shouldn't traverse at all if we hit a symlink, we should just be copying the symlink. In fact I'm pretty sure that's what we do, but in this case it's a symlink of a file To a directory, so we might not be identifying that properly.
The current changeset is working around this problem by identifying a very specific way that this might arise. I think we should instead make our linking mechanic robust enough that this is a non-issue in the first place.
|
It seems to me that in order to solve this problem we either need to make the |
