fix(cli): add colors back for cache hit log line #1346

tknickman · 2022-06-07T19:57:42Z

Fixes a bug introduced in 1.2.10 in which the first line (cache hit) is missing the correct color.

Caused by #1256

vercel · 2022-06-07T19:57:45Z

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

Name	Status	Preview	Updated
turbo-site	✅ Ready (Inspect)	Visit Preview	Jun 8, 2022 at 4:51PM (UTC)

tknickman · 2022-06-07T19:58:23Z

cli/internal/runcache/runcache.go

@@ -60,7 +61,7 @@ func AddFlags(opts *Opts, flags *pflag.FlagSet) {
 		Value:    &logsModeValue{opts: opts},
 	})
 	_ = 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 {
+	if err := flags.MarkDeprecated("stream", "[WARNING] The --stream flag is unnecessary and has been deprecated. It will be removed in future versions of turbo."); err != nil {


tknickman · 2022-06-07T19:58:54Z

cli/internal/run/run.go

@@ -796,7 +797,7 @@ func (e *execContext) exec(pt *nodes.PackageTask, deps dag.Set) error {
 		return nil
 	}
 	// Cache ---------------------------------------------
-	taskCache := e.runCache.TaskCache(pt, hash)
+	taskCache := e.runCache.TaskCache(pt, hash, e.colorCache)


Pass the initialized colorCache instead of creating a new one so we get the same colors.

If this stays constant throughout the run, maybe the reference should be on RunCache rather than TaskCache? Or maybe instead of sharing the colorCache, we should be sharing the prefix with the TaskCache?

Good call, I think the RunCache makes the most sense here.

cli/internal/colorcache/color_cache.go

gsoltis

I definitely like moving the ColorCache into its own package. However the rest of this goes, we should definitely do that.

gsoltis · 2022-06-07T20:21:19Z

cli/internal/runcache/runcache.go

 	bufWriter := bufio.NewWriter(output)
-	if _, err := bufWriter.WriteString(fmt.Sprintf("%s: cache hit, replaying output %s\n", tc.pt.OutputPrefix(), ui.Dim(tc.hash))); err != nil {
+	if _, err := bufWriter.WriteString(fmt.Sprintf("%s: cache hit, replaying output %s\n", actualPrefix, ui.Dim(tc.hash))); err != nil {


Hmm, are we already baking the color into the log files?

We are not prebaking this:

turborepo/cli/internal/run/run.go

Line 827 in 1cfaff0

logger := log.New(writer, "", 0)

cli/internal/colorcache/color_cache.go

gsoltis · 2022-06-07T20:24:31Z

cli/internal/run/run.go

@@ -796,7 +797,7 @@ func (e *execContext) exec(pt *nodes.PackageTask, deps dag.Set) error {
 		return nil
 	}
 	// Cache ---------------------------------------------
-	taskCache := e.runCache.TaskCache(pt, hash)
+	taskCache := e.runCache.TaskCache(pt, hash, e.colorCache)


If this stays constant throughout the run, maybe the reference should be on RunCache rather than TaskCache? Or maybe instead of sharing the colorCache, we should be sharing the prefix with the TaskCache?

nathanhammond

This is totally not the place for a feature request, but a feature request: my logs for a particular package are always the same color across runs.

With only five colors available this is likely a bad experience as incidents of mod(int(hash(packageName)), 5) resulting in adjacent duplicate colors will be frequent, but in some future true color terminal world...

Only one review comment is actually intended to block.

nathanhammond · 2022-06-07T23:30:04Z

cli/internal/colorcache/color_cache.go

@@ -1,4 +1,4 @@
-package run
+package colorcache


I'd maybe go with colorcache.go since that's how we'll see it in code every time.

Agreed, the file should technically be named colorcache.go.

nathanhammond · 2022-06-08T00:28:09Z

cli/internal/runcache/runcache.go

@@ -225,8 +227,10 @@ func (tc TaskCache) OutputWriter() (io.WriteCloser, error) {
 	if err != nil {
 		return nil, err
 	}
+	pref := tc.colorCache.PrefixColor(tc.pt.PackageName)
+	actualPrefix := pref(tc.pt.OutputPrefix())


computedPrefix?

I was using existing verbage to stay consistent -

https://github.com/vercel/turborepo/blob/main/cli/internal/run/run.go#L772

But I do like this better so I'll make this change everywhere.

nathanhammond · 2022-06-08T00:32:24Z

cli/internal/runcache/runcache.go

@@ -225,8 +227,10 @@ func (tc TaskCache) OutputWriter() (io.WriteCloser, error) {
 	if err != nil {
 		return nil, err
 	}
+	pref := tc.colorCache.PrefixColor(tc.pt.PackageName)


Actual strong opinion: pref is wired to "preferences" In my brain, can we maybe call this prefixColor?

pref is a function here (given a string, it colors it) - so I might tweak this slightly to be colorPrefixer

nathanhammond · 2022-06-08T00:56:54Z

cli/internal/runcache/runcache.go

 	bufWriter := bufio.NewWriter(output)
-	if _, err := bufWriter.WriteString(fmt.Sprintf("%s: cache hit, replaying output %s\n", tc.pt.OutputPrefix(), ui.Dim(tc.hash))); err != nil {
+	if _, err := bufWriter.WriteString(fmt.Sprintf("%s: cache hit, replaying output %s\n", actualPrefix, ui.Dim(tc.hash))); err != nil {


We are not prebaking this:

turborepo/cli/internal/run/run.go

Line 827 in 1cfaff0

logger := log.New(writer, "", 0)

remove extra semicolon

nathanhammond

Seems like a winner to me.

tknickman · 2022-06-08T16:35:35Z

Also @nathanhammond I didn't forget about your feature request. I want that too. But I think I'll tackle that as part of followup. A reliable method to go from task prefix to color would be awesome.

nathanhammond · 2022-06-08T16:49:02Z

I didn't forget about your feature request

Yeah, but it definitely doesn't belong in this PR. 😅 And Terminal.app isn't true color, so hard to justify...

gsoltis · 2022-06-08T17:00:27Z

cli/internal/colorcache/color_cache.go

@@ -1,4 +1,4 @@
-package run
+package colorcache


Agreed, the file should technically be named colorcache.go.

gsoltis · 2022-06-08T17:05:52Z

cli/internal/runcache/runcache.go

@@ -225,8 +228,10 @@ func (tc TaskCache) OutputWriter() (io.WriteCloser, error) {
 	if err != nil {
 		return nil, err
 	}
+	colorPrefixer := tc.rc.colorCache.PrefixColor(tc.pt.PackageName)


Probably not worth fixing right now, but if we end up revamping how we do coloring and logging, I think it would be good to have this prefix generation done once per task.

We also have a task somewhere to stop baking prefix (and color) into log files, but that's definitely beyond the scope here.

gsoltis · 2022-06-08T17:23:52Z

@nathanhammond the lines immediately following that log.New call add the color prefix, so we do bake color into the logs. We were just missing it for the direct write to the file that happens before we set up the log.

We should stop writing cache status, our color, and task prefix to the logs.

nathanhammond · 2022-06-09T02:56:21Z

@gsoltis At first glance it looked like there was already a layer of indirection with a logger and the logstreamers. I assumed that the streamers were consuming and rewriting things written to log for the purpose of stdout. But that is not what they're actually doing underneath the hood.

Agreed that we should tweak behavior, capture the process output 1:1, and then decorate it when we output to stdout (whether new run, replay, or otherwise).

Updates the behavior of the `--graph` CLI flag and fixes a few bugs. > This command will generate an svg, png, jpg, pdf, json, html, or [other supported output formats](https://graphviz.org/doc/info/output.html) of the current task graph. The output file format defaults to jpg, but can be controlled by specifying the filename's extension. > If Graphviz is not installed, or no filename is provided, this command prints the dot graph to `stdout` This PR also: 1. Updates docs to reflect the current state of the `--graph` CLI flag 1. Refactors the graph visualization code out of `run.go` 1. Cleans up the file name of colors_cache (follow up from #1346) Fixes #1286

Updates the behavior of the `--graph` CLI flag and fixes a few bugs. > This command will generate an svg, png, jpg, pdf, json, html, or [other supported output formats](https://graphviz.org/doc/info/output.html) of the current task graph. The output file format defaults to jpg, but can be controlled by specifying the filename's extension. > If Graphviz is not installed, or no filename is provided, this command prints the dot graph to `stdout` This PR also: 1. Updates docs to reflect the current state of the `--graph` CLI flag 1. Refactors the graph visualization code out of `run.go` 1. Cleans up the file name of colors_cache (follow up from vercel/turborepo#1346) Fixes vercel/turborepo#1286

tknickman requested review from jaredpalmer, gsoltis, gaspar09 and nathanhammond as code owners June 7, 2022 19:57

tknickman commented Jun 7, 2022

View reviewed changes

tknickman force-pushed the cache_hit_colors branch from 2425986 to 6ad201c Compare June 7, 2022 20:15

gsoltis suggested changes Jun 7, 2022

View reviewed changes

nathanhammond reviewed Jun 8, 2022

View reviewed changes

fix(cli): add colors back for cache hit log line

02157f2

remove extra semicolon

tknickman force-pushed the cache_hit_colors branch from 6ad201c to 9d17d14 Compare June 8, 2022 15:59

chore(cli): updates from PR review

03e60f6

tknickman force-pushed the cache_hit_colors branch from 9d17d14 to 03e60f6 Compare June 8, 2022 16:06

nathanhammond approved these changes Jun 8, 2022

View reviewed changes

Merge branch 'main' into cache_hit_colors

9cdb755

vercel bot deployed to Preview June 8, 2022 16:51 View deployment

gsoltis approved these changes Jun 8, 2022

View reviewed changes

kodiakhq bot merged commit b2bd1bc into main Jun 8, 2022

kodiakhq bot deleted the cache_hit_colors branch June 8, 2022 17:17

tknickman mentioned this pull request Jun 8, 2022

feat(cli): update the graph arg behavior #1353

Merged

fix(cli): add colors back for cache hit log line #1346

fix(cli): add colors back for cache hit log line #1346

Uh oh!

Conversation

tknickman commented Jun 7, 2022

Uh oh!

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

Uh oh!

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!

gsoltis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nathanhammond Jun 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

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!

tknickman Jun 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

nathanhammond Jun 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nathanhammond left a comment

Choose a reason for hiding this comment

Uh oh!

tknickman commented Jun 8, 2022

Uh oh!

nathanhammond commented Jun 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gsoltis commented Jun 8, 2022

Uh oh!

nathanhammond commented Jun 9, 2022

Uh oh!

Uh oh!

vercel bot commented Jun 7, 2022 •

edited

Loading

nathanhammond Jun 8, 2022 •

edited

Loading

tknickman Jun 8, 2022 •

edited

Loading

nathanhammond Jun 8, 2022 •

edited

Loading