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
28 changes: 22 additions & 6 deletions cli/internal/hashing/package_deps_hash.go
View file
Open in desktop
Expand Up @@ -78,7 +78,7 @@ func safeCompileIgnoreFile(filepath turbopath.AbsoluteSystemPath) (*gitignore.Gi
return gitignore.CompileIgnoreLines([]string{}...), nil
}

func GetPackageFileHashesFromProcessingGitIgnore(rootPath turbopath.AbsoluteSystemPath, packagePath turbopath.AnchoredSystemPath, inputs []string) (map[turbopath.AnchoredUnixPath]string, error) {
func getPackageFileHashesFromProcessingGitIgnore(rootPath turbopath.AbsoluteSystemPath, packagePath turbopath.AnchoredSystemPath, inputs []string) (map[turbopath.AnchoredUnixPath]string, error) {
result := make(map[turbopath.AnchoredUnixPath]string)
absolutePackagePath := packagePath.RestoreAnchor(rootPath)

Expand Down Expand Up @@ -224,12 +224,28 @@ func getPackageFileHashesFromInputs(rootPath turbopath.AbsoluteSystemPath, packa
return result, nil
}

// GetPackageDeps Builds an object containing git hashes for the files under the specified `packagePath` folder.
func GetPackageDeps(rootPath turbopath.AbsoluteSystemPath, p *PackageDepsOptions) (map[turbopath.AnchoredUnixPath]string, error) {
if len(p.InputPatterns) == 0 {
return getPackageFileHashesFromGitIndex(rootPath, p.PackagePath)
// GetPackageFileHashes Builds an object containing git hashes for the files under the specified `packagePath` folder.
func GetPackageFileHashes(rootPath turbopath.AbsoluteSystemPath, packagePath turbopath.AnchoredSystemPath, inputs []string) map[turbopath.AnchoredUnixPath]string {
if len(inputs) == 0 {
result, err := getPackageFileHashesFromGitIndex(rootPath, packagePath)
if err != nil {
result, err := getPackageFileHashesFromProcessingGitIgnore(rootPath, packagePath, nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

if err != nil {
return make(map[turbopath.AnchoredUnixPath]string)
Copy link
Contributor

Choose a reason for hiding this comment

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

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 🤣

}
return result
}
return result
} else {
return getPackageFileHashesFromInputs(rootPath, p.PackagePath, p.InputPatterns)
result, err := getPackageFileHashesFromInputs(rootPath, packagePath, inputs)
if err != nil {
result, err := getPackageFileHashesFromProcessingGitIgnore(rootPath, packagePath, inputs)
if err != nil {
return make(map[turbopath.AnchoredUnixPath]string)
}
return result
}
return result
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't fix the bug about not propagating the error yet. Right now I just faithfully migrate the bug into the new location.

You can simply look at this method and tell that there are going to be numerous issues in how we process things. Our fallbacks are definitely not consistent.

}
}

Expand Down
12 changes: 4 additions & 8 deletions cli/internal/hashing/package_deps_hash_test.go
View file
Open in desktop
Expand Up @@ -367,11 +367,7 @@ func TestGetPackageDeps(t *testing.T) {
},
}
for _, tt := range tests {
got, err := GetPackageDeps(repoRoot, tt.opts)
if err != nil {
t.Errorf("GetPackageDeps got error %v", err)
continue
}
got := GetPackageFileHashes(repoRoot, tt.opts.PackagePath, tt.opts.InputPatterns)
assert.DeepEqual(t, got, tt.expected)
}
}
Expand All @@ -385,7 +381,7 @@ func Test_memoizedGetTraversePath(t *testing.T) {
assert.Check(t, gotOne == gotTwo, "The strings are identical.")
}

func TestGetPackageFileHashesFromProcessingGitIgnore(t *testing.T) {
func Test_getPackageFileHashesFromProcessingGitIgnore(t *testing.T) {
rootIgnore := strings.Join([]string{
"ignoreme",
"ignorethisdir/",
Expand Down Expand Up @@ -460,7 +456,7 @@ func TestGetPackageFileHashesFromProcessingGitIgnore(t *testing.T) {
pkg := &fs.PackageJSON{
Dir: pkgName,
}
hashes, err := GetPackageFileHashesFromProcessingGitIgnore(repoRoot, pkg.Dir, []string{})
hashes, err := getPackageFileHashesFromProcessingGitIgnore(repoRoot, pkg.Dir, []string{})
if err != nil {
t.Fatalf("failed to calculate manual hashes: %v", err)
}
Expand All @@ -487,7 +483,7 @@ func TestGetPackageFileHashesFromProcessingGitIgnore(t *testing.T) {
}

count = 0
justFileHashes, err := GetPackageFileHashesFromProcessingGitIgnore(repoRoot, pkg.Dir, []string{filepath.FromSlash("**/*file"), "!" + filepath.FromSlash("some-dir/excluded-file")})
justFileHashes, err := getPackageFileHashesFromProcessingGitIgnore(repoRoot, pkg.Dir, []string{filepath.FromSlash("**/*file"), "!" + filepath.FromSlash("some-dir/excluded-file")})
if err != nil {
t.Fatalf("failed to calculate manual hashes: %v", err)
}
Expand Down
18 changes: 1 addition & 17 deletions cli/internal/taskhash/taskhash.go
View file
Open in desktop
Expand Up @@ -85,19 +85,7 @@ func (pfs packageFileSpec) ToKey() packageFileHashKey {
}

func (pfs *packageFileSpec) getHashObject(pkg *fs.PackageJSON, repoRoot turbopath.AbsoluteSystemPath) map[turbopath.AnchoredUnixPath]string {
hashObject, pkgDepsErr := hashing.GetPackageDeps(repoRoot, &hashing.PackageDepsOptions{
PackagePath: pkg.Dir,
InputPatterns: pfs.inputs,
})
if pkgDepsErr != nil {
manualHashObject, err := manuallyHashPackage(pkg, pfs.inputs, repoRoot)
if err != nil {
return make(map[turbopath.AnchoredUnixPath]string)
}
Comment on lines -104 to -106
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a bug that needs to get fixed.

hashObject = manualHashObject
}

return hashObject
return hashing.GetPackageFileHashes(repoRoot, pkg.Dir, pfs.inputs)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is to push the fallback behavior into the hashing module itself, not at the consumption location.

}

func (pfs *packageFileSpec) hash(hashObject map[turbopath.AnchoredUnixPath]string) (string, error) {
Expand All @@ -108,10 +96,6 @@ func (pfs *packageFileSpec) hash(hashObject map[turbopath.AnchoredUnixPath]strin
return hashOfFiles, nil
}

func manuallyHashPackage(pkg *fs.PackageJSON, inputs []string, rootPath turbopath.AbsoluteSystemPath) (map[turbopath.AnchoredUnixPath]string, error) {
return hashing.GetPackageFileHashesFromProcessingGitIgnore(rootPath, pkg.Dir, inputs)
}

// packageFileHashes is a map from a package and optional input globs to the hash of
// the matched files in the package.
type packageFileHashes map[packageFileHashKey]string
Expand Down