Handle recursive links in runtime sources. #3600
Conversation
mitchell-as
commented
Nov 19, 2024
•
mitchell-as
commented
 DX-3167 Our depot linking mechanic is aware of directories it has already traversed
|
mitchell-as
force-pushed
the
mitchell/dx-3167-2
branch
from
6fb3a1e to
d33a4d0
Compare
This results in a destination without directory links, which are not supported on Windows.
internal/smartlink/smartlink.go
Outdated
| if visited == nil { | ||
| visited = make(map[string]bool) | ||
| } | ||
| if _, exists := visited[src]; exists && srcWasSymlink { |
There was a problem hiding this comment.
Hmm I think this is now trying to solve the same issue two ways. If the src is a symlink then we should just make the deployment a link also, and stop iterating further down because it's a symlink. In this case what we already visited is irrelevant.
So I think we can get rid of the visited logic altogether, and simply revise it to make dir links if the src is a dir link, and otherwise create the dir and continue recursion.
mitchell-as
force-pushed
the
mitchell/dx-3167-2
branch
from
230b4c4 to
cd9e2d3
Compare
mitchell-as
force-pushed
the
mitchell/dx-3167-2
branch
from
cd9e2d3 to
b75482b
Compare
|
I am a bit nervous about these changes, as I don't know if they'll have unintended consequences. The critical tests seem to be passing, so that brings a little comfort. |
|
@Naatan ping |
internal/smartlink/smartlink.go
Outdated
| @@ -47,6 +47,13 @@ func Link(src, dest string) error { | |||
| } | |||
|
|
|||
| if fileutils.IsDir(src) { | |||
| if isSymlink(src) { | |||
There was a problem hiding this comment.
| if isSymlink(src) { | |
| if fileutils.IsSymlink(src) { |
internal/smartlink/smartlink.go
Outdated
| func isSymlink(src string) bool { | ||
| target, err := fileutils.SymlinkTarget(src) | ||
| return err == nil && src != target | ||
| } | ||
|
|
There was a problem hiding this comment.
| func isSymlink(src string) bool { | |
| target, err := fileutils.SymlinkTarget(src) | |
| return err == nil && src != target | |
| } |
src != target does not assert whether something is a symlink. Is this a concern we have? We could add the error check and return an error, but this doesn't feel relevant to the issue at hand. ie. the bug we're addressing isn't that a file is linking to itself.
internal/smartlink/smartlink.go
Outdated
| src, err = filepath.Abs(filepath.Clean(src)) | ||
| if err != nil { | ||
| return "", "", errs.Wrap(err, "Could not resolve src path") | ||
| } | ||
| dest, err = fileutils.ResolveUniquePath(dest) | ||
| dest, err = filepath.Abs(filepath.Clean(dest)) |
There was a problem hiding this comment.
Please revert. Absolute path does not give a canonical result, whereas ResolveUniquePath does (or at least -tries to-).
mitchell-as
force-pushed
the
mitchell/dx-3167-2
branch
from
66e4bdf to
d1a7e18
Compare
