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.

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

cmd/go: do not cache tool output if tools print to stdout/stderr #27628

Open
cherrymui opened this issue Sep 11, 2018 · 34 comments
Open

cmd/go: do not cache tool output if tools print to stdout/stderr #27628

cherrymui opened this issue Sep 11, 2018 · 34 comments

Comments

@cherrymui
Copy link
Member

cherrymui commented Sep 11, 2018

Update, Oct 7 2020: see #27628 (comment) for most recent proposal in this issue.

What version of Go are you using (go version)?

tip (2e5c325), also Go 1.11

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

darwin/amd64

What did you do?

$ go build -toolexec=/usr/bin/time hello.go
# command-line-arguments
        0.01 real         0.00 user         0.00 sys
# command-line-arguments
        0.12 real         0.11 user         0.02 sys
$ go build hello.go 
# command-line-arguments
        0.01 real         0.00 user         0.00 sys
# command-line-arguments
        0.12 real         0.11 user         0.02 sys
$

What did you expect to see?

The second invocation of go build doesn't have -toolexec, so it should not invoke the toolexec command (which I think it doesn't), nor reprint its output.

What did you see instead?

toolexec output is reprinted.

In fact, I think it probably should not cache at all if -toolexec is specified, since the external command that toolexec invokes may do anything, and (intentionally) not reproducible.

cc @dr2chase

@dr2chase
Copy link
Contributor

Using a cooked command "wrap" =

#!/bin/bash
echo -n `basename "$1"` 1>&2 ; /usr/bin/time "$@"
date 1>&2 

it keeps reprinting the same date+time. This is definitely cached output.

go build -toolexec wrap .
go build .
rm hello
go build .

@mark-rushakoff
Copy link
Contributor

#27207 is about -exec prevents test caching. Maybe there needs to be a more holistic review of -exec, -toolexec, and friends, and how they all relate to caching.

@bcmills bcmills added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Sep 12, 2018
@bcmills
Copy link
Contributor

bcmills commented Sep 12, 2018

I'm not sure whether we should cache the result of -toolexec (see #27207 (comment)), but if we do, the value of the -toolexec flag should be included in the cache key.

So there's definitely a problem here either way: we just need to decide whether the fix is to update the cache key or exclude the result from caching.

@bcmills bcmills added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsFix The path to resolution is known, but the work has not been done. labels Sep 12, 2018
@bcmills bcmills added this to the Go1.12 milestone Sep 12, 2018
@ianlancetaylor
Copy link
Contributor

I actually don't think those are the right choices. There is a plausible use of -toolexec to invoke a debugger or other interactive program, in which case we should never cache if -toolexec is used. And there is a plausible use of -toolexec to invoke a logger or other build annotator, in which we should completely ignore -toolexec and its argument for caching purposes. I can't think of an important use case in which we want to cache based on whether we are using the same -toolexec option as before.

I would probably pick "never cache if -toolexec is used" but I don't feel very strongly about it.

@cherrymui
Copy link
Member Author

I would probably pick "never cache if -toolexec is used"

I vote for this.

@bcmills bcmills added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Sep 12, 2018
@bcmills
Copy link
Contributor

bcmills commented Sep 12, 2018

It looks like we have a consensus. (Moving from NeedsDecision to NeedsFix.)

@rsc rsc changed the title cmd/go: -toolexec is mistakenly cached cmd/go: do not cache tool output when using -toolexec Oct 24, 2018
@bcmills bcmills modified the milestones: Go1.12, Go1.13 Jan 17, 2019
@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@mvdan
Copy link
Member

mvdan commented Dec 8, 2019

I agree that this bug needs fixing one way or another. The only way I find -toolexec useful right now is like -a -toolexec=whatever.

I have some thoughts about caching, though. It's unfortunate if we simply throw caching out the window, because it's entirely reasonable for toolexec programs to be deterministic given their input.

The way go build handles caching properly for go tool compile seems to be to first ask it compile -V=full, and use that as part of the cache key. This means the compile operations will need re-doing if the compiler version changes, but not otherwise.

Could we do the same for -toolexec as an opt-in? For example, if one does go build -toolexec=mytool, the Go tool would run a well defined version flag like mytool -toolversion, similar to the compiler's -V=full. If that fails, we do no caching at all with -toolexec=mytool. If it succeeds, its output is added to the cache key, and caching takes place.

I think this is the best of both worlds, because current non-deterministic toolexec programs would continue to use no caching at all, while other Go-specific complex tools could take advantage of the build cache.

We would have to define the bare minimum of information for -toolversion to report, though. For example, would a stringified https://golang.org/pkg/runtime/debug/#Module be enough, if available? Or perhaps a hash of the file contents in https://golang.org/pkg/os/#Executable?

@mvdan
Copy link
Member

mvdan commented Dec 9, 2019

Thinking about my comment above a bit more, I think this issue should still be fixed by simply doing no caching when -toolexec is used. That would be strictly better than what we have right now.

Once this issue is fixed, I can then file the "opt-in caching for -toolexec" idea as a separate proposal.

@rsc
Copy link
Contributor

rsc commented Sep 2, 2020

Going to mark this as a proposal since the answer is unclear and it is bound up in #41145. Maybe they should be merged.

@rsc rsc added Proposal and removed NeedsFix The path to resolution is known, but the work has not been done. labels Sep 2, 2020
@rsc rsc added this to Active in Proposals (old) Sep 2, 2020
@rsc rsc changed the title cmd/go: do not cache tool output when using -toolexec proposal: cmd/go: do not cache tool output when using -toolexec Sep 2, 2020
@rsc rsc modified the milestones: Proposal, Backlog Oct 21, 2020
@rsc rsc changed the title proposal: cmd/go: do not cache tool output if tools print to stdout/stderr cmd/go: do not cache tool output if tools print to stdout/stderr Oct 21, 2020
@bcmills
Copy link
Contributor

bcmills commented May 26, 2021

It turns out that this also affects the cmd/go.TestScript/toolexec and cmd/go.TestScript/vet.

The vet test expects go vet to be silent for packages with no vet errors, but if the toolexec test ends up rebuilding shared packages in the standard library (e.g. because they are otherwise stale; see #46347), its output will pollute the vet test.

@janflyborg
Copy link

I have seen the same thing with printouts from a previous run with GODEBUG=gctrace=1, being printed, even if I start without that environment variable, later on (please see https://groups.google.com/g/golang-nuts/c/UP9hIrvqDj4 for a better explanation).

Is this related?

@gopherbot
Copy link

Change https://golang.org/cl/359614 mentions this issue: cmd/go: do not cache compiler or linker output

@bcmills
Copy link
Contributor

bcmills commented Oct 29, 2021

I tried to make a run at this for Go 1.18, but ran into what appear to be some existing bugs in go test output caching that I think I'm going to have to detangle to get it into a testable state. 😞

@bcmills
Copy link
Contributor

bcmills commented Nov 1, 2021

(Filed the above test-caching bug as #49267.)

@bcmills
Copy link
Contributor

bcmills commented May 12, 2023

I don't think this is going to land for 1.21. (Too many existing caching bugs to work through to get it to a testable state.)

@bcmills bcmills modified the milestones: Go1.21, Go1.22 May 12, 2023
@bcmills bcmills modified the milestones: Go1.22, Backlog Feb 1, 2024
@bcmills
Copy link
Contributor

bcmills commented Feb 1, 2024

Moving to Backlog. This will require diagnosing and fixing the existing caching bugs so that we can write an integration test for the new caching behavior, and that is a fairly large project.

@bcmills bcmills removed their assignment Mar 15, 2024
@bcmills bcmills added the GoCommand cmd/go label Mar 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted
Development

No branches or pull requests