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

make GLFW C revision a part of input to Go build cache #269

Closed
dmitshur opened this issue Feb 16, 2020 · 4 comments · Fixed by #310
Closed

make GLFW C revision a part of input to Go build cache #269

dmitshur opened this issue Feb 16, 2020 · 4 comments · Fixed by #310

Comments

@dmitshur
Copy link
Member

From https://golang.org/cmd/go/#hdr-Build_and_test_caching:

The build cache correctly accounts for changes to Go source files, compilers, compiler options, and so on: cleaning the cache explicitly should not be necessary in typical use. However, the build cache does not detect changes to C libraries imported with cgo. If you have made changes to the C libraries on your system, you will need to clean the cache explicitly or else use the -a build flag (see 'go help build') to force rebuilding of packages that depend on the updated C libraries.

There's currently a problem where updating the GLFW C version does not invalidate the Go build cache for the github.com/go-gl/glfw/v3.3/glfw package.

We have a GLFW_C_REVISION.txt file that tracks the revision of the GLFW C library being used.

We can rename the GLFW_C_REVISION.txt file to something like glfwcrev.go and include the GLFW C revision as a comment:

package glfw

// GLFW_C_REVISION = 0a49ef0a00baa3ab520ddc452f0e3b1e099c5580

Then changing the GLFW C revision will cause one of the .go files to become different, and the old build cache will no longer get in the way.

This is somewhat related to #251 and #258.

@pwaller
Copy link
Member

pwaller commented Jun 25, 2020

A friend of mine just hit a related issue, when they tried to diagnose an issue by editing the C files, they found that the build cache meant that it wouldn't actually pick up the new files. My workaround was this: GODEBUG=gocacheverify=1 go build -v.

Not ideal. I know we shouldn't be editing the files but it can be useful for diagnostics.

The only other way I'm aware of that we could fix this in principle on the glfw side is to put the C files into the same directory as the package. AIUI, this would result in Go building those C files. So we'd have to have a package per platform, or #ifdef toggles, essentially. I don't see a neat way which allows us to drop in the upstream glfw sources, at a glance.

@dmitshur do you have any other ideas? Do we have to live with the behaviour as it is?

@dmitshur
Copy link
Member Author

dmitshur commented Oct 24, 2020

@dmitshur do you have any other ideas? Do we have to live with the behaviour as it is?

@pwaller My suggestion is:

We have a GLFW_C_REVISION.txt file that tracks the revision of the GLFW C library being used.

We can rename the GLFW_C_REVISION.txt file to something like glfwcrev.go and include the GLFW C revision as a comment:

package glfw

// GLFW_C_REVISION = 0a49ef0a00baa3ab520ddc452f0e3b1e099c5580

Then changing the GLFW C revision will cause one of the .go files to become different, and the old build cache will no longer get in the way.

(I'm not sure if you missed it in the original issue report.)

Maybe it should be an unexported constant instead of a comment. Both are probably fine.

@dmitshur
Copy link
Member Author

Oh, I see you were referring to editing C files for debugging specifically.

Given how rarely that happens, and it's done by package developers, I'm okay with them having to be mindful of the build cache for now.

Let's fix this problem for users of the package first (what this issue is about), then we can see if there's a reasonable way to also fix it for developers. I don't think we should do it if it complicates things too much.

pwaller added a commit to pwaller/glfw that referenced this issue Mar 6, 2021
This is a workaround for the issue described in go-gl#269. Go unfortunately doesn't
consider the C source files as part of its build cache calculation. Therefore,
invalidate it by writing a hash which is computed across the full C source
directory.

This hash is only updated when `go generate` is called, but this will at least
help the majority of users, when glfw is updated.

Resolves go-gl#269.
pwaller added a commit to pwaller/glfw that referenced this issue Mar 6, 2021
This is a workaround for the issue described in go-gl#269. Go unfortunately doesn't
consider the C source files as part of its build cache calculation. Therefore,
invalidate it by writing a hash which is computed across the full C source
directory.

This hash is only updated when `go generate` is called, but this will at least
help the majority of users, when glfw is updated. This is encoded in
grab-upstream.sh which is used for updating the sources.

This hash is intended to be protected from divergance by CI, too.

Resolves go-gl#269.
pwaller added a commit to pwaller/glfw that referenced this issue Mar 6, 2021
This is a workaround for the issue described in go-gl#269. Go unfortunately doesn't
consider the C source files as part of its build cache calculation. Therefore,
invalidate it by writing a hash which is computed across the full C source
directory.

This hash is only updated when `go generate` is called, but this will at least
help the majority of users, when glfw is updated. This is encoded in
grab-upstream.sh which is used for updating the sources.

This hash is intended to be protected from divergance by CI, too.

Resolves go-gl#269.
pwaller added a commit to pwaller/glfw that referenced this issue Mar 6, 2021
This is a workaround for the issue described in go-gl#269. Go unfortunately doesn't
consider the C source files as part of its build cache calculation. Therefore,
invalidate it by writing a hash which is computed across the full C source
directory.

This hash is only updated when `go generate` is called, but this will at least
help the majority of users, when glfw is updated. This is encoded in
grab-upstream.sh which is used for updating the sources.

This hash is intended to be protected from divergance by CI, too.

Resolves go-gl#269.
pwaller added a commit to pwaller/glfw that referenced this issue Mar 6, 2021
This is a workaround for the issue described in go-gl#269. Go unfortunately doesn't
consider the C source files as part of its build cache calculation. Therefore,
invalidate it by writing a hash which is computed across the full C source
directory.

This hash is only updated when `go generate` is called, but this will at least
help the majority of users, when glfw is updated. This is encoded in
grab-upstream.sh which is used for updating the sources.

This hash is intended to be protected from divergance by CI, too.

Resolves go-gl#269.
@pwaller
Copy link
Member

pwaller commented Mar 6, 2021

I've merged #310 as a solution to this (currently only implemented for v3.3), since #309 upgraded the sources to v3.3.3 and it seems important that infected caches don't confuse things in hard-to-detect ways as users who may have built prior to v3.3.3 try to pull this in.

It addresses both of our use cases, I think, by having the hash reflect the files as they are on disk at the time of invoking go generate. go generate is invoked when grab-upstream.sh is run, however, go generate does not yet invoke grab-upstream as we discussed it might (or similar) in #271.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants