-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
go/build: build cache does not properly take AR into account #30046
Comments
We can cherry-pick the fix back to Go 1.12 if the fix happens after Go 1.12 is out. (this alone won't halt the release) |
/cc @ianlancetaylor |
Change https://golang.org/cl/160897 mentions this issue: |
Is this bug important enough to warrant inclusion in Go 1.12? I'm using tip so I don't care if this misses 1.12. Regardless, the fix seems simple; see the CL above. |
I don't think this needs to go into 1.12. You are likely the only person who is trying to change the ar program they are using. |
Yes, I agree. Though I suspect that there are at least two of us, given that I didn't author a063a22 :) |
The gccgo toolchain uses the archiver specified by the AR environment variable, or `ar` by default. Teach the build ID to take the value of this environment variable into account, since different archivers can produce different results. Fix #30046. Change-Id: Ia6821258d54eecedb9026afc38a515cd564c45cb Reviewed-on: https://go-review.googlesource.com/c/go/+/160897 Run-TryBot: Russ Cox <rsc@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Russ Cox <rsc@golang.org>
What version of Go are you using (
go version
)?What operating system and processor architecture are you using (
go env
)?go env
OutputWhat did you do?
AR=gcc-ar go build
What did you expect to see?
A successful build using the
gcc-ar
wrapper that supports LTO.What did you see instead?
A cached failure using the old value of AR:
Dropping the cache first makes the build properly use the specified AR:
By my read, a063a22 doesn't interact with the build cache at all, and it probably should.
The text was updated successfully, but these errors were encountered: