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: build cache does not check #included headers for changes #24355
Comments
Can you show us a small standalone example? I'm not clear on what you mean by "included cgo files". Do you mean explicitly files included using |
Yes. |
One possibility would be to use |
Just to make sure I'm following. Did you mean add the contents of all listed files or just the list? I had running with -E and hashing the output in mind, but I guess your idea may be more efficient (is it?). |
I meant the contents of the listed files, as in |
This is also affecting me in a more general sense that with 1.10 there is no way to model the build dependency to statically linked libraries anymore. If the library changes, the cached files are still reused and I silently end up using an older version of the library unless I do |
I think this is basically working as expected. |
There is a way to make it work with the help of (hello.h) #define WORLDNUM 3 (hello.c) #include <stdio.h>
#include "hello.h"
int main() {
printf("Hello world (%d)!\n", WORLDNUM);
}
This way if there is something like // #include "mycode.cinc"
import "C" Cgo could see the dependency on |
Sorry guys, I misclicked. Didn't mean to close. What @ianlancetaylor and @navytux suggest seems like a good fix to me. |
While true, there's a hidden security subtlety here. Lets suppose there's a crypto library called I - as a user of the This same issue will happen arbitrarily high a dependency chain, where anyone forgetting to rebuild with Btw, I'm not saying I know how to fix this or whether it's even fixable. I just wanted to add a bit of weight behind this issue. |
Just a quick note because nobody has mentioned solutions to the more general issue @rsc pointed out:
This is going to cause confusing issues in practice. I doubt that everyone is aware of all packages that use C in some sub-dependency. Similarly a lot of people won't always know whether the compiler got updated recently. As @karalabe points out this is a potential security risk. But it is also a general usability problem, as it may very well break builds or even the resulting binaries. These problems seem pretty similar to the issues ccache and zapcc face. I don't know where this is documented for zapcc, but ccache has a few pointers here: https://ccache.samba.org/manual/latest.html#_common_hashed_information In general I don't see much harm in hashing a little more of the environment ( |
I think so too, a couple of safeguards could save a lot of people a lot of time and confusion. After many people update the package code, they don't even know that the cache of go1.10 caused the bug to not be fixed. |
As a naive user, I got bitten by this, but at least it was really obvious: there was a bug in the glfw package's C code (caught by newer compiler, which issued a warning). so i changed the code, ran go build... same error. it was not at all obvious why it was giving me an error that couldn't possibly refer to any existing file on the disk, but apparently "-a" would have helped... but that's extremely non-obvious, and there's no reason that i should have to rebuild other unrelated packages to hint "actually, this package has a thing that has changed". my actual quick workaround: a blank line in the .go file including the affected .c file. |
We ran into this yesterday in our CI environment. I'm happy to look into fixing this for 1.14; the -MD solution seems like a good first step, but as @FlorianUekermann points out above, we may want to consider mixing in a bit more information. I wonder if it is too late to consider adding info to cgo documentation for 1.13? |
@dhobsd for 1.13, documentation is fine. Code changes not so much :) |
Ah, actually I see that there is already material included about GOCACHE and cgo interaction in the go tool docs; I'll hold off until the 1.14 cycle is open to poke at fixing this. |
So it seems like GOCACHE is simply not safe when linking to C at all. |
As I have trouble finding what you meant there, I'm gonna copy-paste it from https://golang.org/cmd/go/#hdr-Build_and_test_caching for future readers of this ticket:
In other words, if you use Cgo, you MUST use |
Go does not notice when the C code changes, so we have to use `go test -a`. Workaround for golang/go#24355 .
Actually, The
This seems to fix it, so probably better to use this instead of (in addition to?) the
|
The behavoir after `go test -a` is somewhat surprising, so add `go clean`, which seems to actuall bring everything up to date. golang/go#24355 (comment)
As described in https://golang.org/cmd/cgo/ , Cgo only notices changes to .c and .h files in the same folder. Move the testsuite to the top-level folder to get rid of the manual cache cleaning which made running the tests so much slower. golang/go#24355
It (may) be nice to have a mode that invalidates all cgo but not proper go. |
Possibly works around golang/go#24355 Signed-off-by: Jakub Sztandera <kubuxu@protocol.ai>
After 6 years this issue is still open :( |
go 1.10 linux/amd64
The
go build
andgo test
commands don't rebuild packages if included cgo files changed. I guess a solution would be to run the preprocessor first or disable caching for packages that use cgo altogether.The text was updated successfully, but these errors were encountered: