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 when using -toolexec #27628

Open
cherrymui opened this Issue Sep 11, 2018 · 6 comments

Comments

Projects
None yet
5 participants
@cherrymui
Contributor

cherrymui commented Sep 11, 2018

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

This comment has been minimized.

Contributor

dr2chase commented Sep 11, 2018

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

This comment has been minimized.

Contributor

mark-rushakoff commented Sep 11, 2018

#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

This comment has been minimized.

Member

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 and removed NeedsFix labels Sep 12, 2018

@bcmills bcmills added this to the Go1.12 milestone Sep 12, 2018

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Sep 12, 2018

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

This comment has been minimized.

Contributor

cherrymui commented Sep 12, 2018

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

I vote for this.

@bcmills bcmills added NeedsFix and removed NeedsDecision labels Sep 12, 2018

@bcmills

This comment has been minimized.

Member

bcmills commented Sep 12, 2018

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

@rsc rsc changed the title from cmd/go: -toolexec is mistakenly cached to cmd/go: do not cache tool output when using -toolexec Oct 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment