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: build caching failed to notice cgo header changes #25419

Closed
minux opened this issue May 16, 2018 · 4 comments
Closed

cmd/go: build caching failed to notice cgo header changes #25419

minux opened this issue May 16, 2018 · 4 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@minux
Copy link
Member

minux commented May 16, 2018

Please answer these questions before submitting your issue. Thanks!

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

go version devel +3868a371a8 Tue May 15 20:47:43 2018 +0000 linux/amd64

Does this issue reproduce with the latest release?

Yes.

Example

Two files t.go and t.h are needed. t.go just calls a static function defined in t.h.

// t.go
package main

// #include "t.h"
import "C"

func main() {
	C.hello()
}

// t.h
#include <stdio.h>
static void hello() {
	printf("hello\n");
}

Reproduction steps

go build ignores changes to the cgo header file.

$ go build t.go
$ ./t
hello
$ sed -i -e 's/"hello/&, world/' t.h    # modify t.h without changing t.go
$ go build t.go  # nothing is rebuild
$ ./t
hello   # output unchanged.
$ GODEBUG=gocacheverify=1 go build a.go  # didn't catch the problem
$ go clean -cache && go build t.go && ./t
hello, world   # clearing the build cache finally fixes the bug

Discussion

Of course, changes like this is arguably hard for the Go tool to detect, and in fact, sometimes, we
don't want the Go tools to read all transitively required header files just to determine whether
to rebuild a package that uses cgo. However, the current behavior is very confusing, as it quietly
disregards changes to the header files, even the one in the package's directory.

I think we either should handle this completely (i.e. tap into gcc's -MM and handle changes to any transitively included files), or at least handle the directly included header file in Go source files (and make this problem clear in release notes.)

@ianlancetaylor ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 16, 2018
@ianlancetaylor ianlancetaylor added this to the Go1.11 milestone May 16, 2018
@alexd765
Copy link
Contributor

Related Issue: #19883
There we decided against becoming a C build system (your first solution).
Detecting changes in the same directory used to work though.

@minux
Copy link
Member Author

minux commented May 17, 2018 via email

@AlexRouSg
Copy link
Contributor

I don't think just checking the direct includes is very useful, that would require one to include every single relevant header manually and missing even one would cause the "surprise".

I think it would be more piratical to make it a habit to invoke go clean -cache -testcache after changing the headers. So I would suggest changing the docs to include that (preferably in bold) instead.

@ianlancetaylor
Copy link
Contributor

Closing as dup of #24355 .

@golang golang locked and limited conversation to collaborators Jul 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants