Erik Sipsma pushed the above changes I

[inner moon]

The output looks a lot nicer overall! The one thing that is missing is when I run dagger checks in the server env I don't see output for each go test (I had also added another unit test there to check that output, fixed the conflict w/ your commit that did the same thing 🙂 )

This was the previous output before rebasing, which is uglier except that it shows each test name

[lusty edge]

oh yeah, I changed it to do per-package instead of per-test, we can tweak that back if we want but I noticed per-test would just straight up cause failures due to load

[inner moon]

Ah right, I wonder if we can just show more output too so that the go test output itself shows the names of the tests or something

Running all the package tests at once in a single check makes sense though

I'll change Verbose: true in the GoTestOpts and see what happens

[lusty edge]

sounds good!

[inner moon]

Yep works perfectly! Amazing!

[lusty edge]

niiice

lemme know if you have any TUI nitpicks. definitely a "i spent 12 hours finishing the shading on the upper lip" kind of thing

(that might be an obscure reference)

[inner moon]

One small thing, I think the progress is swallowing the --help output here; I can see that output if I run w/ --progress=plain

[lusty edge]

ah makes sense, we probably want to redirect all that output to a vertex or something. I'm also working on "zoomed vertex" support in Progrock, which could help here; it basically tells it to take up the full output region, which will be used for dagger shell

that way it's all within the same system, without feeling shoehorned into vertex semantics

[inner moon]

Just pushed a change to show it via vtx for now, just to hold us over for the demo, looks good enough!

Oh and that commit also snuck in a fix to the thing where only integ-test showed up once added: https://github.com/dagger/dagger/pull/5443/commits/185a0bb937a78d9c891d7dbc31a90f8208b04a66#diff-19fac5e7a3dc2c582243cf38dcc99ada898b5701ac9e17b7834585831ad6d100R132

Thank god go is finally fixing that stupid "capturing loop variable" problem...

[lusty edge]

Lol, dammit, nice makes sense

[inner moon]

@lusty edge I think the reason the check output was not showing up when I expected it to in the demo before was that:

1. previously I ran everything and then added all the "mistakes", which was as a side-effect invalidating my cache, but for that one I already had run all those commands and thus was hitting cache
2. the change to print the results to the vertexes server-side resulted in us skipping printing results when cached

So I think I'm gonna make a quick change to move the check result vtx printing back to the client-side (i.e. cmd/dagger/check.go). Trying that now but do you see any potential problems up front?

[lusty edge]

Does that mean you'll have to recurse over all the checks yourself to print them?

And is it this cache that's the issue? https://github.com/sipsma/dagger/blob/58940ccb61b4b3d0b2c7b60448506ee2b70d04f1/core/schema/env.go#L663

If so, maybe we could just move the vertex plumbing outside of checkResultInner so it always runs 🤔

[inner moon]

No I don't think so, I think the issue is that we print results during the resolver call of the various checks, but due to caching those resolver calls don't actually always run. Which is what we want, but doesn't work when we are printing them server side

[lusty edge]

oh wait, the whole reason I added the caching was because it was repeating output multiple times. hmm

[inner moon]

Yes, I think that will be the actual solution, to just do this all client-side. But I started doing it and got a headache so instead I'm just seeing what happens if I include a cache buster arg to all the check resolver calls so they always run. Not optimal but for the current demo code not a big deal

[lusty edge]

Yeah, I'm not sure where the progress stuff should land; the precedent so far is to trust the server with all that where possible, so the client side is just a dumb renderer. But this case is a little awkward to handle

[lusty edge]

fwiw I suspect it actually is that cache that I added, since we should at least be running the checkResolver API calls, even if the underlying resolver/container/whatever ends up being cached

one alternative route could be for Progrock to support a "hey here's all of the vertex output" API or something, so it doesn't repeat

[inner moon]

I don't think it could be because the problem happens across different dagger checks calls, which are all separate clients/servers, and that cache is instantiated per-server

[lusty edge]

ah ok

[inner moon]

I think it's just that when everything is uncached, all of the resolvers are called, which results in the vtx printing being called. But when things are cached, only the "top-level" check resolver is called, and the cached result is read. That result does have all of the subresults from before, but we only print the result of the top-level check in question for each resolver.

Another solution would be to leave it server-side but print results recursively and then de-dupe one way or another like you said. I'm not sure if that's what we want here though. It feels like the result is just an object being queried, so printing it to progress server side would be like printing the entire container object every time it's resolved. It feels like how to display the result should be up to the client. But that's more of a philosophical issue, low confidence

Cachebusting seems to work for now 👍