Port run to cobra #1298

gsoltis · 2022-05-27T19:41:32Z

Removes some long-deprecated flags:

serial
includeDependencies
cacheFolder

vercel · 2022-05-27T19:41:35Z

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

1 Ignored Deployment

Name	Status	Preview	Comments	Updated
turbo-site	⬜️ Ignored (Inspect)			Jun 1, 2022 at 4:50PM (UTC)

nathanhammond

Some comments, some questions!

nathanhammond · 2022-06-02T07:12:10Z

cli/internal/cache/cache.go

+func AddFlags(opts *Opts, flags *pflag.FlagSet, repoRoot fs.AbsolutePath) {
+	// skipping remote caching not currently a flag
+	flags.BoolVar(&opts.SkipFilesystem, "remote-only", false, _remoteOnlyHelp)
+	fs.AbsolutePathVar(flags, &opts.Dir, "cache-dir", repoRoot, "Specify local filesystem cache directory.", "./node_modules/.cache/turbo")


The input path spec is unclear, it could be any of (Absolute|Relative)*(System|Unix). We should probably start communicating that users should use Unix separators, even on Windows.

I might be missing something, why do windows users need to use unix separators?

If we're running things in CI we should generally encourage consistent input otherwise the user can receive surprising output. I think that if we actually push path types through the entire system then we'd be okay, but right now I worry that there are edge cases is the user specifies Windows paths.

Hmm. We have a few different kinds of paths. Cache directory is pretty clearly machine-local, and thus seems like it should be strictly platform-dependent. Especially since I don't think it can or should be configured via shared config file.

Other paths are shared, for instance, global dependency paths. In my mind those are two different types (the global deps one is really a glob, even if you use a file path), but I'm sure we don't do a great job calling that out or helping with that distinction.

Actually, perhaps those are the only two in the user input? Absolute-or-relative platform-dependent path for things like cache directory, and unix-like glob for any paths shared across machines? Might be some good candidates for at least some internal types, if not external documentation.

In the CI case, I don't think it would make sense to share the same input value for cache directory across a linux machine and windows machine. Unless there's a windows convention I'm not aware of, I wouldn't expect a unix path to work to point to a local directory.

nathanhammond · 2022-06-02T07:16:43Z

cli/internal/fs/path.go

+
+func (pv *pathValue) String() string {
+	if *pv.current == "" {
+		return ResolveUnknownPath(pv.base, pv.defValue).ToString()


In this case, if pv.defValue is an absolute path but doesn't use os.PathSeparator, it still short-circuits out of calling Join which would result in a consistent system path by virtue of calling Clean.

In other words, on Windows: C:\path\to\thing and C:/path/to/thing can both come out of this. (Meets the strict definition of AbsolutePath but I've been treating it as a system path thus far.)

Relative paths should work just fine.

(Sorry, I've spent far too much time staring at paths recently and have all of this top-of-mind.)

IIUC, the issue is that if a user supplies C:/unix/like/path, we will pass it through as an AbsolutePath? I'm a little tempted to say "don't do that". Passing /unix/like/path on windows ends up getting treated as a relative path and the slashes appropriately handled. It is likely a user error, but not really one we can correct because we don't know the drive letter.

I'd mostly just pitch running it through Clean afterward so that it's consistent.

I did some research, and it appears that / is forbidden in windows filenames, so running Clean should be safe, as we are not removing their ability to reference a directory called "unix/like/path" contained within C:.

However, and I don't know if it matters, but without running it through Clean, some combination of the Go and windows filesystem APIs end up treating / as a separator. Calling os.MkdirAll("C:/some", ...) and ioutil.WriteFile("C:/some/path", ...) on windows successfully results in a file at C:\some\path.

I think I would've expected it to fail, given that / is both not the separator and not allowed in filenames.

nathanhammond · 2022-06-02T07:34:53Z

cli/internal/runcache/runcache.go

+	_ = flags.Bool("stream", true, "Unused")
+	if err := flags.MarkDeprecated("stream", "[WARNING] The --stream flag is unnecesary and has been deprecated. It will be removed in future versions of turbo."); err != nil {
+		// fail fast if we've misconfigured our flags
+		panic(err)


If I'm reading this correctly, this can only occur if we removed stream but forgot to remove it here?

Yeah. Unfortunately due to how the flags api works, there are some methods that return errors when they can't find the specified flag, as opposed to being able to set attributes on the flag declaration itself. I put those as panics since they should all be executed synchronously from main, so it should fail quickly and consistently if it's broken.

nathanhammond · 2022-06-02T07:36:44Z

cli/internal/runcache/runcache.go

+
+func (l *logsModeValue) Set(value string) error {
+	switch value {
+	case "full":


These come in as a string but can we make our internal references to a constant instead of string literals?

nathanhammond · 2022-06-02T07:49:22Z

cli/internal/runcache/runcache.go

+}
+
+func (l *logsModeValue) String() string {
+	if l.opts.CacheHitLogsMode == FullLogs && l.opts.CacheMissLogsMode == FullLogs {


MissMode↓ HitsMode→ Full No Hash

Full full new-only

No none

Hash hash-only

nathanhammond · 2022-06-02T08:34:11Z

cli/internal/run/run.go

 	if err != nil {
+		exitErr := &process.ChildExit{}
+		if errors.As(err, &exitErr) {
+			return exitErr.ExitCode


This is an interesting change; I don't know what I think about it.

Did we fail, or did the thing we called fail?

Should we replace the child error with a consistent error code at our level, or propagate the child error?

I'm not sure I have strong opinions here, but I do know that if we do this coding to Turbo's exit code API becomes irregular.

The intention is to exit with the highest exit code that we've observed. If we generate our own error, we return 1. I don't think it's worth doing more than checking for exit code == 0 when programming against this value currently.

If we wanted to improve this situation, we could attempt to smuggle more information through this single value, but I think we would need to see a compelling use case for an automated tool reacting differently to turbo failing vs something that turbo called failing.

Also, fwiw, unless I've missed something, this is not new behavior, it was previously implemented here

I may also misunderstand in trying to statically reason about it, I read it as a change from "always 1" to "whatever we have gotten back."

I agree that this isn't an API we've currently got planned, so I'm comfortable with 0 vs. non-zero.

nathanhammond · 2022-06-02T08:42:26Z

cli/internal/run/run.go

 	}
-
-	return exitCode
+	return nil


I like the change of hoisting exit code.

nathanhammond · 2022-06-02T08:45:53Z

cli/internal/run/run.go

-		opts.cacheOpts.SkipFilesystem = true
+func (d *dryRunValue) String() string {
+	if d.opts.dryRunJSON {
+		return "json"


Can we make these constants?

nathanhammond · 2022-06-02T08:48:09Z

cli/internal/run/run.go

+		// fail fast if we've messed up our flag configuration
+		panic(err)
+	}
+	aliases["dry"] = "dry-run"


While reading through I went back to find this, good catch. 👍

nathanhammond · 2022-06-02T08:49:01Z

cli/internal/run/run.go

-	if !config.IsLoggedIn() {
-		opts.cacheOpts.SkipRemote = true
-	}
+func (d *dryRunValue) Type() string {


I have no idea what this is doing. Can you help me understand? (It doesn't appear in old code either.) Is it specifying the alias for some help text somewhere?

The short answer is that this is required to implement the pflag.Value interface.

The longer answer is that it is used for a variety of things internal to how flags are parsed and displayed in help/usage text. I'll add some comments, or maybe pull this out into its own utility.

Added some comments. I think we need at least one more instance of this kind of flag before extracting it, and I'm not yet sure if it's feasbile or worthwhile w/ Go's type system. I expect to at least attempt this implementing #1286

@nathanhammond

Addressing some of @nathanhammond's review comments from #1298 I added a test for `AbsolutePathVar` so that we can have a way to discuss the intended behavior.

Base automatically changed from gsoltis/run_opts to main May 31, 2022 16:45

Port run to cobra

f42b7b3

gsoltis force-pushed the gsoltis/run_cobra branch from 68ea84a to f42b7b3 Compare May 31, 2022 23:26

Add comment for AbsolutePathVar

32371f5

gsoltis marked this pull request as ready for review June 1, 2022 15:54

gsoltis requested review from jaredpalmer, gaspar09 and nathanhammond as code owners June 1, 2022 15:54

Merge branch 'main' into gsoltis/run_cobra

cec9a8f

vercel bot deployed to Preview June 1, 2022 15:55 View deployment

Merge in main

b4917ae

gsoltis mentioned this pull request Jun 1, 2022

Make --graph option print to stdout by default #1286

Closed

jaredpalmer approved these changes Jun 1, 2022

View reviewed changes

kodiakhq bot merged commit e59d0a0 into main Jun 1, 2022

gsoltis deleted the gsoltis/run_cobra branch June 1, 2022 19:14

nathanhammond reviewed Jun 2, 2022

View reviewed changes

gsoltis mentioned this pull request Jun 2, 2022

Follow up to porting run to cobra #1327

Merged

kodiakhq bot pushed a commit that referenced this pull request Jun 6, 2022

Follow up to porting run to cobra (#1327)

6e2d314

Addressing some of @nathanhammond's review comments from #1298 I added a test for `AbsolutePathVar` so that we can have a way to discuss the intended behavior.

Port run to cobra #1298

Port run to cobra #1298

Uh oh!

Conversation

gsoltis commented May 27, 2022

Uh oh!

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

Uh oh!

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!

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!

Uh oh!

vercel bot commented May 27, 2022 •

edited

Loading