Disable linking. #1354
Disable linking. #1354
Conversation
|
@nathanhammond is attempting to deploy a commit to the Vercel Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
| if !got { | ||
| t.Errorf("SameFile(%v, %v) got false, want true", aPath, dstAPath) | ||
| if got { | ||
| t.Errorf("SameFile(%v, %v) got true, want false", aPath, dstAPath) |
There was a problem hiding this comment.
This clause gets flipped because we always copy a new file.
| if target != linkTarget { | ||
| t.Errorf("Readlink got %v, want %v", target, linkTarget) | ||
| } | ||
| target, err := os.Lstat(dstLinkPath) |
There was a problem hiding this comment.
Blindly reading a link here will throw an error in a world where we always copy a file.
| } | ||
| target, err := os.Lstat(dstLinkPath) | ||
| assert.NilError(t, err, "Lstat") | ||
| assert.Check(t, target.Mode().IsRegular(), "the cached file is a regular file") |
There was a problem hiding this comment.
Instead we want to make sure it is just a normal, boring file.
| t.Errorf("Readlink got %v, want missing", target) | ||
| } | ||
| _, err = os.Lstat(dstBrokenLinkPath) | ||
| assert.ErrorIs(t, err, os.ErrNotExist) |
There was a problem hiding this comment.
This checks for approximately the same outcome. Except in this case the link is not present. This doesn't actually matter since we don't presently restore broken symlinks.
| } | ||
| dstLstat, dstLstErr := os.Lstat(dstLinkPath) | ||
| assert.NilError(t, dstLstErr, "Lstat") | ||
| assert.Check(t, dstLstat.Mode().IsRegular(), "the cached file is a regular file") |
There was a problem hiding this comment.
Still making sure it's all regular files.
| fromFile, err := os.Open(from) | ||
| if err != nil { | ||
| if isSymlink { |
There was a problem hiding this comment.
Since we didn't previously error in the event of encountering a broken symlink this maintains that behavior.
We can revisit whether or not this is a good idea later, but this PR aims to maintain consistency.
| @@ -41,7 +41,7 @@ func (f *fsCache) Fetch(target, hash string, _unusedOutputGlobs []string) (bool, | |||
| } | |||
|
|
|||
| // Otherwise, copy it into position | |||
| err := fs.RecursiveCopyOrLinkFile(cachedFolder, target, fs.DirPermissions, true, true) | |||
| err := fs.RecursiveCopyOrLinkFile(cachedFolder, target, fs.DirPermissions, false, false) | |||
There was a problem hiding this comment.
This flips the toggles to say, "please don't try to link me."
|
(We can actively remove the linking branches in a followup PR, I started that way, but it's a bit more-invasive.) |
8c27728 to
075b060
Compare
|
The code changes look good, I think they do indeed do what is described. I think we need to get a handle on perf implications before merging though. |
|
Ok, I inadvertently merged this, but then went and did the perf investigation. I was unable to find significant perf differences across a range of workloads, so I'm good to leave this merged. |
On #1354 Kodiak trolled us. This expands the blocking labels to ensure that the labels that look like they should be blocking are indeed blocking.
This brings back some behavior from before #1354. The filesystem cache will save all symlinks, and restore non-broken symlinks to files. Symlinks to directories continue to get restored as empty directories. Still TBD what we're going to do with broken symlinks in the filesystem cache, they currently are not restored. For reference: the http cache saves and restores all symlinks.
This is the simplest possible change to fix #1323 which addresses the root issue: turning off the hard linking behavior via function arguments.
This is an alternative to #1351.