Change fs.ReadPackageJSON to operate on AbsolutePath instances #1773
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
afab0ee to
db96142
Compare
db96142 to
c2cac53
Compare
There was a problem hiding this comment.
Three changes requested, none of which are particularly material. I will rebase #1779 on top of this.
cli/internal/context/context.go
Outdated
| @@ -305,21 +304,25 @@ func (c *Context) populateTopologicGraphForPackageJSON(pkg *fs.PackageJSON, root | |||
| return nil | |||
| } | |||
|
|
|||
| func (c *Context) parsePackageJSON(buildFilePath string) error { | |||
| func (c *Context) parsePackageJSON(repoRoot fs.AbsolutePath, pkgJSONPath fs.AbsolutePath) error { | |||
| c.mutex.Lock() | |||
| defer c.mutex.Unlock() | |||
|
|
|||
| // log.Printf("[TRACE] reading package.json : %+v", buildFilePath) | |||
There was a problem hiding this comment.
(request for change)
Let's drop this log which now logs something that doesn't exist.
| c.mutex.Lock() | ||
| defer c.mutex.Unlock() | ||
|
|
||
| // log.Printf("[TRACE] reading package.json : %+v", buildFilePath) | ||
| if fs.FileExists(buildFilePath) { |
There was a problem hiding this comment.
That implicit dependency on cwd. 😅
| pkg.PackageJSONPath = turbopath.AnchoredSystemPathFromUpstream(relativePkgJSONPath) | ||
| pkg.Dir = turbopath.AnchoredSystemPathFromUpstream(filepath.Dir(relativePkgJSONPath)) |
There was a problem hiding this comment.
These are indeed both AnchoredSystemPaths. 👍
| @@ -143,6 +144,7 @@ func WithGraph(repoRoot fs.AbsolutePath, rootPackageJSON *fs.PackageJSON, cacheD | |||
| } | |||
|
|
|||
| // Get the workspaces from the package manager. | |||
| // workspaces are absolute paths | |||
There was a problem hiding this comment.
That moment when you finally find where you're going to stop pulling the thread.
| @@ -154,12 +156,9 @@ func WithGraph(repoRoot fs.AbsolutePath, rootPackageJSON *fs.PackageJSON, cacheD | |||
| // until all parsing is complete | |||
| parseJSONWaitGroup := &errgroup.Group{} | |||
| for _, workspace := range workspaces { | |||
| relativePkgPath, err := filepath.Rel(rootpath, workspace) | |||
| if err != nil { | |||
| return fmt.Errorf("non-nested package.json path %w", err) | |||
There was a problem hiding this comment.
Note for readers: this error got moved into the function. It's currently impossible to reach this error case because of how the paths are generated. 🤷♂️
| @@ -22,7 +22,7 @@ func Test_ReadTurboConfig(t *testing.T) { | |||
| rootDir := "testdata" | |||
| turboJSONPath := cwd.Join(rootDir) | |||
| packageJSONPath := cwd.Join(rootDir, "package.json") | |||
| rootPackageJSON, pkgJSONReadErr := ReadPackageJSON(packageJSONPath.ToStringDuringMigration()) | |||
| @@ -824,7 +824,7 @@ func (r *run) executeDryRun(ctx gocontext.Context, engine *core.Scheduler, g *co | |||
| Package: pt.PackageName, | |||
| Hash: hash, | |||
| Command: command, | |||
| Dir: pt.Pkg.Dir, | |||
| Dir: pt.Pkg.Dir.ToString(), | |||
There was a problem hiding this comment.
(request for change)
| Dir: pt.Pkg.Dir.ToString(), | |
| Dir: pt.Pkg.Dir.ToStringDuringMigration(), |
There was a problem hiding this comment.
This is a JSON representation, not something we use for further computation. I think .ToString() is correct.
| @@ -937,7 +937,8 @@ func (e *execContext) exec(ctx gocontext.Context, pt *nodes.PackageTask, deps da | |||
| } | |||
|
|
|||
| cmd := exec.Command(e.packageManager.Command, argsactual...) | |||
| cmd.Dir = pt.Pkg.Dir | |||
| // TODO(gsoltis): I think this might be an actual bug? cmd.Dir should probably be an absolute path | |||
There was a problem hiding this comment.
I agree that this is a "bug" but I don't think it matters so long as you have to run at root.
There was a problem hiding this comment.
I believe it is a real bug affecting --cwd. I have a follow-up PR to fix it.
| @@ -24,7 +24,7 @@ func Test_manuallyHashPackage(t *testing.T) { | |||
| t.Fatalf("failed to create temp dir: %v", err) | |||
| } | |||
| repoRoot := turbopath.AbsoluteSystemPathFromUpstream(root) | |||
| pkgName := turbopath.RelativeSystemPath("libA") | |||
| pkgName := turbopath.AnchoredSystemPath("libA") | |||
There was a problem hiding this comment.
This is a correct change, which hallelujah can be detected just from types.
| @@ -51,7 +51,7 @@ func Test_manuallyHashPackage(t *testing.T) { | |||
| t.Fatalf("failed to write contents to .gitignore: %v", err) | |||
| } | |||
| rootIgnoreFile.Close() | |||
| pkgIgnoreFilename := repoRoot.Join(pkgName, ".gitignore") | |||
| pkgIgnoreFilename := repoRoot.Join(turbopath.RelativeSystemPath(pkgName.ToString()), ".gitignore") | |||
There was a problem hiding this comment.
(request for change)
This isn't supposed to work, I'm not actually sure why it does? Is that literal .gitignore somehow getting past the type check?
func (p AbsoluteSystemPath) Join(additional ...RelativeSystemPath) AbsoluteSystemPath {
cast := RelativeSystemPathArray(additional)
return AbsoluteSystemPath(filepath.Join(p.ToString(), filepath.Join(cast.ToStringArray()...)))
}
pkgIgnoreFilename := repoRoot.Join( // AbsoluteSystemPath
turbopath.RelativeSystemPath(
pkgName.ToString() // AnchoredSystemPath
),
".gitignore" // string, wat?
)The way I envisioned doing this:
pkgName.RestoreAnchor(repoRoot).Join(turbopath.RelativeSystemPath(".gitignore");There was a problem hiding this comment.
literals are a hole in Go's newtype implementation. They are implicitly treated as the expected type.
Setting `cmd.Dir` to an absolute path ensures that it will continue to work in the presence of the `--cwd` flag. Bonus drop unused `turboCache` from `execContext`. Note that this PR stacks on top of #1773
Updates
fs.ReadPackageJSONto take anAbsolutePathso that we respect--cwdPulls the thread on updating
PackageJSON.DirandPackageJSON.PackageJSONPathto have the correctturbopathtypes.cc @nathanhammond note in particular changes to
taskhash_test.go