Add path types for cache. #2059

nathanhammond · 2022-09-23T08:50:20Z

As prework for merging cache-to-tar, this ensures that we know what types of paths we're dealing with at (almost) every step in the cache process.

It also makes the first argument to Put (previously unused) and Fetch the anchor of the enumerated list of files being saved or restored.

vercel · 2022-09-23T08:50:26Z

@nathanhammond is attempting to deploy a commit to the Vercel Team on Vercel.

A member of the Team first needs to authorize it.

nathanhammond

A guide to the non-mechanical changes.

nathanhammond · 2022-09-23T09:01:46Z

cli/internal/cache/cache_fs.go


 	// If it's not in the cache bail now
-	if !fs.PathExists(cachedFolder) {
+	if !cachedFolder.DirExists() {


This should have always been a DirExists check, not a PathExists check.

nathanhammond · 2022-09-23T09:04:01Z

cli/internal/cache/cache_fs.go


-	if !fs.PathExists(cachedFolder) {
+	if !cachedFolder.DirExists() {


This should have always been a DirExists check, not a PathExists check.

nathanhammond · 2022-09-23T09:05:06Z

cli/internal/cache/cache.go

 			return ok, actualFiles, duration, err
 		}
 	}
-	return false, files, 0, nil
+
+	return false, nil, 0, nil


With ok being false, we should be returning nil instead of the input list of files.

nathanhammond · 2022-09-23T09:08:54Z

cli/internal/cache/cache_fs_test.go

-type testingUtil interface {
-	Helper()
-	Cleanup(f func())
-}
-
-// subdirForTest creates a sub directory of `cwd` and registers it for
-// deletion by the testing framework at the end of the test.
-// Some cache code currently assumes relative paths from `cwd`, so it is not
-// yet feasible to use temp directories.
-func subdirForTest(t *testing.T) string {
-	var tt interface{} = t
-	if tu, ok := tt.(testingUtil); ok {
-		tu.Helper()
-	}
-	cwd, err := fs.GetCwd()
-	assert.NilError(t, err, "cwd")
-	dir, err := os.MkdirTemp(cwd.ToString(), "turbo-test")
-	assert.NilError(t, err, "MkdirTemp")
-	deleteOnFinish(t, dir)
-	return filepath.Base(dir)
-}
-
-func deleteOnFinish(t *testing.T, dir string) {
-	var tt interface{} = t
-	if tu, ok := tt.(testingUtil); ok {
-		tu.Cleanup(func() { _ = os.RemoveAll(dir) })
-	}
-}
-


Directories are now specified by the first argument to Put and Fetch.

nathanhammond · 2022-09-23T09:21:03Z

cli/internal/cache/cache_http.go

-		files = append(files, hdr.Name)
-		filename := root.UntypedJoin(hdr.Name)
+		// FIXME: THIS IS A BUG.
+		restoredName := turbopath.AnchoredUnixPath(hdr.Name)


There are cases where this is a bug. This will be addressed in the cache-to-tar portion.

nathanhammond · 2022-09-23T09:22:25Z

cli/internal/cache/cache_http.go

-		filename := root.UntypedJoin(hdr.Name)
+		// FIXME: THIS IS A BUG.
+		restoredName := turbopath.AnchoredUnixPath(hdr.Name)
+		files = append(files, restoredName.ToSystemPath())


This is conversion can do the wrong thing with \.

nathanhammond · 2022-09-23T09:23:05Z

cli/internal/cache/cache_http.go

+		// FIXME: THIS IS A BUG.
+		restoredName := turbopath.AnchoredUnixPath(hdr.Name)
+		files = append(files, restoredName.ToSystemPath())
+		filename := restoredName.ToSystemPath().RestoreAnchor(root)


Extra-verbose for now, will get cleaned up in cache-to-tar.

nathanhammond · 2022-09-23T09:25:42Z

cli/internal/cache/cache_http_test.go

+	for _, file := range files {
+		gotSet.Add(file.ToString())
+	}


Files are no longer strings! 🎉

nathanhammond · 2022-09-23T09:29:26Z

cli/internal/runcache/runcache.go

@@ -283,10 +284,10 @@ func (tc TaskCache) SaveOutputs(ctx context.Context, logger hclog.Logger, termin
 			terminal.Error(fmt.Sprintf("%s%s", ui.ERROR_PREFIX, color.RedString(" %v", fmt.Errorf("File path cannot be made relative: %w", err))))
 			continue
 		}
-		relativePaths[index] = relativePath
+		relativePaths[index] = fs.UnsafeToAnchoredSystemPath(relativePath)


This is a known value at this point, but I think this whole loop should be cleaned up. We're doing too much work here.

nathanhammond · 2022-09-23T09:30:40Z

cli/internal/runcache/runcache.go

 	}

-	if err = tc.rc.cache.Put(tc.pt.Pkg.Dir.ToStringDuringMigration(), tc.hash, duration, relativePaths); err != nil {
+	if err = tc.rc.cache.Put(tc.rc.repoRoot, tc.hash, duration, relativePaths); err != nil {


This looks like a material change, but previously the value passed in here was unused.

It now uses this as the anchor for the relativePaths slice that is getting passed in here and prevents us from depending on cwd.

nathanhammond · 2022-09-23T09:43:04Z

cli/internal/cache/cache_http_test.go

 	}
 	files, err := restoreTar(root, tar)
 	assert.NilError(t, err, "readTar")

-	expectedSet := util.SetFromStrings(expectedFiles)
+	expectedSet := make(util.Set)


Previously restoreTar was always returning Unix-separated paths, even on Windows. However, downstream uses were treating these as system-separated paths. Likely didn't manifest in user-facing issues because Windows supports / as a separator, but no guarantees.

vercel · 2022-09-23T09:43:21Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
turbo-site	✅ Ready (Inspect)	Visit Preview	Sep 26, 2022 at 7:31AM (UTC)

nathanhammond · 2022-09-23T09:51:06Z

cli/internal/cache/async_cache.go

@@ -76,7 +76,7 @@ func (c *asyncCache) Shutdown() {
 // run implements the actual async logic.
 func (c *asyncCache) run() {
 	for r := range c.requests {
-		c.realCache.Put(r.anchor, r.key, r.duration, r.files)
+		_ = c.realCache.Put(r.anchor, r.key, r.duration, r.files)


I'm doing this to silence the lint error. It doesn't make sense unless we have an error group here and that's something that cache-to-tar will obviate the need for.

gsoltis · 2022-09-23T16:20:51Z

cli/internal/cache/cache_fs_test.go

-	src := filepath.Join(cacheDir, "the-hash", "some-package")
-	err = os.MkdirAll(src, os.ModeDir|0777)
+	// cwd, err := fs.GetCwd()
+	// assert.NilError(t, err, "GetCwd")


Delete commented-out code

gsoltis · 2022-09-23T16:23:11Z

cli/internal/cache/cache_fs_test.go

-	src := subdirForTest(t)
-	childDir := filepath.Join(src, "child")
-	err := os.Mkdir(childDir, os.ModeDir|0777)
+	src := turbopath.AbsoluteSystemPath(t.TempDir())


Not blocking, but I'm really not a fan of casting for getting an instance of a path type:

it's hard to grep / find-usages for

they can be auto-inserted by the IDE

they don't communicate anything about why this is ok.
I would expect this to be something like AbsoluteSystemPathFromUpstream, which communicates that the author has verified that wherever the string is coming from produces absolute paths.

The "rule" I've been following is that I've only been doing type casts in test code, and only on initialization from "raw" values (string primitives, or calls into stdlib). You'll see I didn't do casts in any application code or even in any of the more-hairy test code.

I don't feel strongly either way, so I'll defer to your opinion on this and get the casts swapped out before landing Monday.

If we're going to make this a rule we should write a custom linter that disallows all casts into turbopath that aren't from a string:

(Splitting this change into a separate PR.)

Followup PR: #2077

nathanhammond requested a review from a team as a code owner September 23, 2022 08:50

nathanhammond requested review from tknickman and NicholasLYang September 23, 2022 08:50

nathanhammond requested review from gaspar09 and gsoltis and removed request for tknickman and NicholasLYang September 23, 2022 08:50

nathanhammond commented Sep 23, 2022

View reviewed changes

vercel bot deployed to Preview September 23, 2022 09:44 View deployment

nathanhammond force-pushed the cache-dir-absolute-path branch from 0269228 to 05974dc Compare September 23, 2022 09:49

nathanhammond commented Sep 23, 2022

View reviewed changes

vercel bot deployed to Preview September 23, 2022 09:52 View deployment

gsoltis added the pr: fixship A PR which is approved with notes and does not require re-review. label Sep 23, 2022

gsoltis approved these changes Sep 23, 2022

View reviewed changes

nathanhammond added 13 commits September 26, 2022 13:26

Cache restoration from absolute path.

c035a98

Rework Put test.

8c27013

Put AnchoredSystemPaths

eaaa6da

Fetch AnchoredSystemPaths

85c9b06

Repair Fetch test.

6c25a60

Ensure Put and Fetch have an anchor.

38cd3a9

Correct existence check.

c4fac4d

Remove unnecessary comment.

6ccd054

Correct flipped boolean.

2d53e5d

Additional path migration opportunity.

c883b33

Remove unused repoRoot property.

e440b15

Use make instead.

749d73b

File types detected a Windows bug.

edb9beb

Fix lint issues.

1f53362

nathanhammond force-pushed the cache-dir-absolute-path branch from 05974dc to 1f53362 Compare September 26, 2022 05:33

Remove commented code.

f39d5bb

vercel bot deployed to Preview September 26, 2022 07:31 View deployment

nathanhammond added pr: automerge Kodiak will merge these automatically after checks pass and removed pr: fixship A PR which is approved with notes and does not require re-review. labels Sep 26, 2022

kodiakhq bot merged commit 45ff205 into vercel:main Sep 26, 2022

nathanhammond deleted the cache-dir-absolute-path branch September 26, 2022 08:25


		if !fs.PathExists(cachedFolder) {
		if !cachedFolder.DirExists() {

Add path types for cache. #2059

Add path types for cache. #2059

Uh oh!

Conversation

nathanhammond commented Sep 23, 2022

Uh oh!

vercel bot commented Sep 23, 2022

Uh oh!

nathanhammond left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vercel bot commented Sep 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vercel bot commented Sep 23, 2022 •

edited

Loading