Prefactor package hashing #4816

nathanhammond · 2023-05-04T08:27:06Z

We currently do file hashing for packages (and fallbacks) in lots of different places, making it very difficult to trace through or (more-importantly) modify the set of files whose hashes we need to include.

This PR makes no material changes, each commit passes all of our tests. At the end of it all of the hashing logic ends up in just one spot.

vercel · 2023-05-04T08:27:16Z

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

Name	Status	Preview	Comments	Updated (UTC)
examples-svelte-web	🔄 Building (Inspect)			May 4, 2023 1:19pm
turbo-site	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 4, 2023 1:19pm

9 Ignored Deployments

Name	Status	Updated (UTC)
examples-basic-web	⬜️ Ignored (Inspect)	May 4, 2023 1:19pm
examples-cra-web	⬜️ Ignored (Inspect)	May 4, 2023 1:19pm
examples-designsystem-docs	⬜️ Ignored (Inspect)	May 4, 2023 1:19pm
examples-gatsby-web	⬜️ Ignored (Inspect)	May 4, 2023 1:19pm
examples-kitchensink-blog	⬜️ Ignored (Inspect)	May 4, 2023 1:19pm
examples-native-web	⬜️ Ignored (Inspect)	May 4, 2023 1:19pm
examples-nonmonorepo	⬜️ Ignored (Inspect)	May 4, 2023 1:19pm
examples-tailwind-web	⬜️ Ignored (Inspect)	May 4, 2023 1:19pm
examples-vite-web	⬜️ Ignored (Inspect)	May 4, 2023 1:19pm

vercel · 2023-05-04T08:27:24Z

Someone is attempting to deploy a commit to the Vercel Team on Vercel.

A member of the Team first needs to authorize it.

nathanhammond

Reviewer's guide!

There were two goals here:

Force all file hashing into a single straight-through code path.
Have the packageFileHashInputs available for modification based upon TaskDefinition more-generally.

Along the way this fixed two bugs; one of which entirely broke manual hashing in a couple scenarios.

Review it commit-by-commit.

nathanhammond · 2023-05-04T11:29:43Z

cli/internal/hashing/package_deps_hash.go

@@ -404,7 +421,7 @@ func (s statusCode) isDelete() bool {
 // We need to calculate where the repository's location is in order to determine what the full path is
 // before we can return those paths relative to the calling directory, normalizing to the behavior of
 // `ls-files` and `ls-tree`.
-func gitStatus(rootPath turbopath.AbsoluteSystemPath, patterns []string) (map[turbopath.AnchoredUnixPath]statusCode, error) {
+func gitStatus(rootPath turbopath.AbsoluteSystemPath) (map[turbopath.AnchoredUnixPath]statusCode, error) {


I discovered that our current use does not pass any patterns to git status.

nathanhammond · 2023-05-04T12:26:21Z

cli/internal/hashing/package_deps_hash.go

-		for filePath, hash := range hashes {
-			result[filePath] = hash
-		}
+		return getPackageFileHashesFromGitIndex(rootPath, p.PackagePath)


Moved the body of this branch.

nathanhammond · 2023-05-04T12:28:23Z

cli/internal/hashing/package_deps_hash.go

+	if len(p.InputPatterns) == 0 {
+		return getPackageFileHashesFromGitIndex(rootPath, p.PackagePath)
+	} else {
+		return getPackageFileHashesFromInputs(rootPath, p.PackagePath, p.InputPatterns)


In this commit the second side of the if statement got moved into its own function.

nathanhammond · 2023-05-04T12:30:23Z

cli/internal/taskhash/taskhash.go

-		return nil, err
-	}
-	return hashObject, nil
+	return hashing.GetPackageFileHashesFromProcessingGitIgnore(rootPath, pkg.Dir, inputs)


Turns out that we have a third method of doing file hashes for packages, tucked inside of taskhash.go.

This moves the body of the function to an exported place in package-deps-hash.go.

nathanhammond · 2023-05-04T12:31:08Z

cli/internal/hashing/package_deps_hash.go

@@ -68,8 +70,93 @@ func getPackageFileHashesFromGitIndex(rootPath turbopath.AbsoluteSystemPath, pac
 	return result, nil
 }

-// func getFileHashesFromProcessingGitIgnore(rootPath turbopath.AbsoluteSystemPath, packagePath turbopath.AnchoredSystemPath, inputs []string) (map[turbopath.AnchoredUnixPath]string, error) {
-// }
+func safeCompileIgnoreFile(filepath turbopath.AbsoluteSystemPath) (*gitignore.GitIgnore, error) {


This helper function came along for the ride and got converted to turbopath, cleaning up a couple of small bits of its invocation.

nathanhammond · 2023-05-04T13:07:26Z

cli/internal/taskhash/taskhash.go

-		pkgName, _ := util.GetPackageTaskFromId(taskID)
-		if pkgName == th.rootNode {
+
+		packageName, _ := util.GetPackageTaskFromId(taskID)


We're not character constrained, so clear names that match structs are good.

nathanhammond · 2023-05-04T13:09:43Z

cli/internal/taskhash/taskhash.go

-				hashObject, err := packageFileSpec.getHashObject(pkg, repoRoot)
+
+				// Get the hashes of each file, keyed by the path.
+				hashObject, err := hashing.GetPackageFileHashes(repoRoot, pkg.Dir, packageFileHashInputs.taskDefinition.Inputs)


Intentionally keeps the same signature that everywhere else does and doesn't redirect it so that we don't accidentally end up in the "N ways to do things" situation again.

nathanhammond · 2023-05-04T13:10:40Z

cli/internal/taskhash/taskhash.go

+				hashes[packageFileHashInputs.taskID] = hash
+				hashObjects[packageFileHashInputs.taskID] = hashObject


taskID is a much better key, making the whole thing understandable.

yesss. this tripped me up a lot when i was working on this code, thank you

I think this is going to result in rehashing the same package multiple times if there is more than one task in that package.

For instance, if test depends on build, and neither specifies inputs, this will double the amount of hashing that we do.

Ok, I doubled checked what I wrote and it looks like there's a bug where it's not doing what we expect. This change would result in rehashing the same package multiple times if we were already properly deduping sets of files to hash, but we are not.

The reason we are not currently properly deduping is because we are adding the addresses of packageFileSpec instances to a set, which are all going to be unique, even if the actual values are equivalent.

The original author (me) probably should've noticed this given the effort to produce a key for a map later on, and knowing that the set is implemented via a map under the hood. I'm going to file an issue to fix this post-port, since it will be much easier to do the struct value equality in Rust.

The actual deduping I want to do is to push all files into a set, do a set intersection, and only hash files that don't already have a hash. There's no particular reason to tie it to a package or task.

nathanhammond · 2023-05-04T13:11:34Z

cli/internal/taskhash/taskhash.go

-				hash, err := packageFileSpec.hash(hashObject)
+
+				// Get the combined hash of all the files.
+				hash, err := fs.HashObject(hashObject)


No indirection where behavior can be modified spookily at a distance. Optimizing for clarity in hash construction.

nathanhammond · 2023-05-04T13:12:17Z

cli/internal/taskhash/taskhash.go

 	if !ok {
-		return "", fmt.Errorf("cannot find package-file hash for %v", pkgFileHashKey)
+		return "", fmt.Errorf("cannot find package-file hash for %v", packageTask.TaskID)


This error message is now infinitely better.

I'm surprised you didn't say "rounds-to-infinity" in true nathan style 😉 😛

mehulkar

This is great, thanks for the step by step and the review notes on each commit. Couple comments:

The last few commits tripped me up a little because the PR says there are "no material changes", but the error propagation does seem to have changed, (in that it fixes a real bug where a fallback hashing strategy was not being used?)
There's a bug where the manual hashing fallback doesn't seem to work on relative paths, which I ran into because git hash-object fails on a large amount of files. Neither of these tickets are relevant to this PR except to say: I feel more confident about fixing those bugs after this PR, so thank you!

mehulkar · 2023-05-04T17:54:26Z

cli/internal/hashing/package_deps_hash.go

+		if err != nil {
+			result, err := getPackageFileHashesFromProcessingGitIgnore(rootPath, packagePath, nil)
+			if err != nil {
+				return make(map[turbopath.AnchoredUnixPath]string)


~~Is this default return new?~~

after carefully stepping through commits I realize this is the thing that you're trying to get to in the first place 🤣

mehulkar · 2023-05-04T17:56:17Z

cli/internal/hashing/package_deps_hash.go

+	if len(inputs) == 0 {
+		result, err := getPackageFileHashesFromGitIndex(rootPath, packagePath)
+		if err != nil {
+			result, err := getPackageFileHashesFromProcessingGitIgnore(rootPath, packagePath, nil)


What's the actual difference between getPackageFileHashesFromGitIndex and getPackageFileHashesFromProcessingGitIgnore? And is it intentional?

I don't know yet! I copy/pasted the implementations. Those three code paths should probably be just one.

mehulkar · 2023-05-04T17:57:26Z

cli/internal/fs/hash.go

@@ -19,21 +19,6 @@ func HashObject(i interface{}) (string, error) {
 	return hex.EncodeToString(hash.Sum(nil)), err
 }

-func HashFile(filePath string) (string, error) {


hah nice find

mehulkar · 2023-05-04T17:59:27Z

cli/internal/hashing/package_deps_hash.go

@@ -253,7 +253,7 @@ func GetPackageFileHashes(rootPath turbopath.AbsoluteSystemPath, packagePath tur
 func manuallyHashFiles(rootPath turbopath.AbsoluteSystemPath, files []turbopath.AnchoredSystemPath) (map[turbopath.AnchoredUnixPath]string, error) {
 	hashObject := make(map[turbopath.AnchoredUnixPath]string)
 	for _, file := range files {
-		hash, err := fs.GitLikeHashFile(file.ToString())
+		hash, err := fs.GitLikeHashFile(file.RestoreAnchor(rootPath))


I like to add TODO: or NOTE: comments so it's clear that a bug was found and it will be fixed as a follow on. Someone reading this code after this PR that may not have any "fixes" merged would like to know this is a problem as well.

mehulkar · 2023-05-04T18:04:43Z

cli/internal/taskhash/taskhash.go

+				hashes[packageFileHashInputs.taskID] = hash
+				hashObjects[packageFileHashInputs.taskID] = hashObject


yesss. this tripped me up a lot when i was working on this code, thank you

mehulkar · 2023-05-04T18:05:15Z

cli/internal/taskhash/taskhash.go

 	if !ok {
-		return "", fmt.Errorf("cannot find package-file hash for %v", pkgFileHashKey)
+		return "", fmt.Errorf("cannot find package-file hash for %v", packageTask.TaskID)


I'm surprised you didn't say "rounds-to-infinity" in true nathan style 😉 😛

gsoltis · 2023-05-04T20:49:27Z

cli/internal/hashing/package_deps_hash.go

@@ -253,7 +253,7 @@ func GetPackageFileHashes(rootPath turbopath.AbsoluteSystemPath, packagePath tur
 func manuallyHashFiles(rootPath turbopath.AbsoluteSystemPath, files []turbopath.AnchoredSystemPath) (map[turbopath.AnchoredUnixPath]string, error) {
 	hashObject := make(map[turbopath.AnchoredUnixPath]string)
 	for _, file := range files {
-		hash, err := fs.GitLikeHashFile(file.ToString())
+		hash, err := fs.GitLikeHashFile(file.RestoreAnchor(rootPath))


IIUC, this fixes the bug. Previously we called os.Open() on a relative path (file). That has cwd-dependent behavior. Now we use an absolute path. This is why we use turbopath.

gsoltis · 2023-05-04T21:02:20Z

cli/internal/taskhash/taskhash.go

+				hashes[packageFileHashInputs.taskID] = hash
+				hashObjects[packageFileHashInputs.taskID] = hashObject


I think this is going to result in rehashing the same package multiple times if there is more than one task in that package.

For instance, if test depends on build, and neither specifies inputs, this will double the amount of hashing that we do.

gsoltis · 2023-05-04T21:20:07Z

cli/internal/taskhash/taskhash.go

+				hashes[packageFileHashInputs.taskID] = hash
+				hashObjects[packageFileHashInputs.taskID] = hashObject


Ok, I doubled checked what I wrote and it looks like there's a bug where it's not doing what we expect. This change would result in rehashing the same package multiple times if we were already properly deduping sets of files to hash, but we are not.

The reason we are not currently properly deduping is because we are adding the addresses of packageFileSpec instances to a set, which are all going to be unique, even if the actual values are equivalent.

The original author (me) probably should've noticed this given the effort to produce a key for a map later on, and knowing that the set is implemented via a map under the hood. I'm going to file an issue to fix this post-port, since it will be much easier to do the struct value equality in Rust.

We currently do file hashing for packages (and fallbacks) in _lots_ of different places, making it _very_ difficult to trace through or (more-importantly) modify the set of files whose hashes we need to include. This PR makes no material changes, each commit passes all of our tests. At the end of it all of the hashing logic ends up in just one spot. --------- Co-authored-by: Nathan Hammond <Nathan Hammond>

Nathan Hammond added 3 commits May 4, 2023 16:04

Split out getPackageFileHashesFromGitIndex.

d835690

Split out getPackageFileHashesFromInputs.

c760457

Split out GetPackageFileHashesFromProcessingGitIgnore.

4720f40

nathanhammond requested a review from a team as a code owner May 4, 2023 08:27

nathanhammond requested review from mehulkar and NicholasLYang May 4, 2023 08:27

turbo-orchestrator bot added the team: turborepo label May 4, 2023

Nathan Hammond added 3 commits May 4, 2023 17:07

Move test.

4b1fd4f

Move the fallback behavior inline.

be50d8e

Make linter happy.

2b7e154

nathanhammond requested a review from gsoltis May 4, 2023 09:31

vercel bot deployed to Preview – turbo-site May 4, 2023 09:32 View deployment

Nathan Hammond added 7 commits May 4, 2023 17:49

Remove unused function.

7bb65aa

BUGFIX: use turbopath, correct path error.

57cd58e

Force hashes through a helper function that handles fallback.

8258d86

Stop directly calling gitHashObject

a55c004

Force getting hashes for files through a single code path.

a5f3b1b

BUGFIX: manual fallback now propagates error.

5f3bcbe

Remove packageFileSpec. Key needs to be shareable.

269e49b

nathanhammond commented May 4, 2023

View reviewed changes

Make linter happy.

2df5983

vercel bot deployed to Preview – turbo-site May 4, 2023 13:19 View deployment

mehulkar approved these changes May 4, 2023

View reviewed changes

gsoltis approved these changes May 4, 2023

View reviewed changes

gsoltis added the pr: automerge Kodiak will merge these automatically after checks pass label May 4, 2023

kodiakhq bot merged commit 0cae83b into vercel:main May 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prefactor package hashing #4816

Prefactor package hashing #4816

nathanhammond commented May 4, 2023

vercel bot commented May 4, 2023 •

edited

Loading

vercel bot commented May 4, 2023

nathanhammond left a comment

nathanhammond May 4, 2023

nathanhammond May 4, 2023

nathanhammond May 4, 2023

nathanhammond May 4, 2023

nathanhammond May 4, 2023

nathanhammond May 4, 2023

nathanhammond May 4, 2023

nathanhammond May 4, 2023

mehulkar May 4, 2023

gsoltis May 4, 2023

gsoltis May 4, 2023

nathanhammond May 5, 2023

nathanhammond May 4, 2023

nathanhammond May 4, 2023

mehulkar May 4, 2023

mehulkar left a comment

mehulkar May 4, 2023

mehulkar May 4, 2023

nathanhammond May 5, 2023

mehulkar May 4, 2023

mehulkar May 4, 2023

mehulkar May 4, 2023

mehulkar May 4, 2023

gsoltis May 4, 2023

gsoltis May 4, 2023

gsoltis May 4, 2023

		hashes[packageFileHashInputs.taskID] = hash
		hashObjects[packageFileHashInputs.taskID] = hashObject

Prefactor package hashing #4816

Prefactor package hashing #4816

Conversation

nathanhammond commented May 4, 2023

vercel bot commented May 4, 2023 • edited Loading

vercel bot commented May 4, 2023

nathanhammond left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mehulkar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vercel bot commented May 4, 2023 •

edited

Loading