Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Prefactor package hashing #4816

Merged
merged 14 commits into from May 4, 2023
Merged

Prefactor package hashing #4816

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
d835690
Split out getPackageFileHashesFromGitIndex.
May 4, 2023
c760457
Split out getPackageFileHashesFromInputs.
May 4, 2023
4720f40
Split out GetPackageFileHashesFromProcessingGitIgnore.
May 4, 2023
4b1fd4f
Move test.
May 4, 2023
be50d8e
Move the fallback behavior inline.
May 4, 2023
2b7e154
Make linter happy.
May 4, 2023
7bb65aa
Remove unused function.
May 4, 2023
57cd58e
BUGFIX: use turbopath, correct path error.
May 4, 2023
8258d86
Force hashes through a helper function that handles fallback.
May 4, 2023
a55c004
Stop directly calling gitHashObject
May 4, 2023
a5f3b1b
Force getting hashes for files through a single code path.
May 4, 2023
5f3bcbe
BUGFIX: manual fallback now propagates error.
May 4, 2023
269e49b
Remove packageFileSpec. Key needs to be shareable.
May 4, 2023
2df5983
Make linter happy.
May 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions cli/internal/fs/hash.go
View file
Open in desktop
Expand Up @@ -5,9 +5,9 @@ import (
"encoding/hex"
"fmt"
"io"
"os"
"strconv"

"github.com/vercel/turbo/cli/internal/turbopath"
"github.com/vercel/turbo/cli/internal/xxhash"
)

Expand All @@ -21,8 +21,8 @@ func HashObject(i interface{}) (string, error) {

// GitLikeHashFile is a function that mimics how Git
// calculates the SHA1 for a file (or, in Git terms, a "blob") (without git)
func GitLikeHashFile(filePath string) (string, error) {
file, err := os.Open(filePath)
func GitLikeHashFile(filePath turbopath.AbsoluteSystemPath) (string, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since I needed to touch this, I made it a turbopath...

file, err := filePath.Open()
if err != nil {
return "", err
}
Expand Down
4 changes: 2 additions & 2 deletions cli/internal/hashing/package_deps_hash.go
View file
Open in desktop
Expand Up @@ -138,7 +138,7 @@ func getPackageFileHashesFromProcessingGitIgnore(rootPath turbopath.AbsoluteSyst
return nil
}
}
hash, err := fs.GitLikeHashFile(convertedName.ToString())
hash, err := fs.GitLikeHashFile(convertedName)
if err != nil {
return fmt.Errorf("could not hash file %v. \n%w", convertedName.ToString(), err)
}
Expand Down Expand Up @@ -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))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But here we can see that if you end up manually hashing files that we would fail if you were using --cwd or running at a place that wasn't the project root.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, this bug is fully addressed.

if err != nil {
return nil, fmt.Errorf("could not hash file %v. \n%w", file.ToString(), err)
}
Expand Down