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

builder: use build ID as cache key #2366

Merged
merged 1 commit into from
Dec 28, 2021
Merged

builder: use build ID as cache key #2366

merged 1 commit into from
Dec 28, 2021

Conversation

aykevl
Copy link
Member

@aykevl aykevl commented Dec 9, 2021

Instead of storing an increasing version number in relevant packages (compiler.Version, interp.Version, cgo.Version, ...), read the build ID from the currently running executable. This has several benefits:

  • All changes relevant to the compiled packages are caught.
  • No need to bump the version for each change to these packages. This avoids merge conflicts.
  • During development, go install is usually enough. No need to run tinygo clean most of the time.

Of course, the drawback is that it might be updated a bit more often
than necessary but I think the overall benefit is big.

Regular release users shouldn't see any difference. Because the tinygo binary stays the same, the cache works well.


I made this PR mostly to make development for myself easier. I've been running rm ~/.cache/tinygo/pkg-* far too often to clean the cache. Previously I just ran tinygo clean but since #2110 that has become a lot more expensive.

@dgryski
Copy link
Member

dgryski commented Dec 9, 2021

Too bad we can't use Go's buildid package directly :(

@aykevl
Copy link
Member Author

aykevl commented Dec 9, 2021

Yeah, I've so often wanted to use their internal packages. I wish they were importable somehow. Even if they don't provide API stability guarantees, having access to them would be great (for API stability, there is of course version pinning using Go modules).

At the same time, by using GNU build IDs, I think we're also including all the non-Go code (LLVM mostly).

@niaow
Copy link
Member

niaow commented Dec 12, 2021

Another option could be to take the hash of the executable. This is probably more expensive, but is portable.

Hmm. . . nevermind it might be a bit too expensive.

@deadprogram
Copy link
Member

@aykevl aykevl marked this pull request as draft December 28, 2021 12:48
Instead of storing an increasing version number in relevant packages
(compiler.Version, interp.Version, cgo.Version, ...), read the build ID
from the currently running executable. This has several benefits:

  * All changes relevant to the compiled packages are caught.
  * No need to bump the version for each change to these packages.
    This avoids merge conflicts.
  * During development, `go install` is enough. No need to run
    `tinygo clean` all the time.

Of course, the drawback is that it might be updated a bit more often
than necessary but I think the overall benefit is big.

Regular release users shouldn't see any difference. Because the tinygo
binary stays the same, the cache works well.
@aykevl aykevl marked this pull request as ready for review December 28, 2021 14:37
@aykevl
Copy link
Member Author

aykevl commented Dec 28, 2021

I think I've fixed the Windows failure. Let's see whether it passes CI.

@aykevl
Copy link
Member Author

aykevl commented Dec 28, 2021

Another option could be to take the hash of the executable. This is probably more expensive, but is portable.

Hmm. . . nevermind it might be a bit too expensive.

Yeah exactly, that was what I first wanted to do but didn't because it seemed unreasonably expensive. Luckily it's not necessary with build IDs :)

Note that the latest push (to fix Windows) only uses the Go build ID, not the full binary build ID. I think that's not too big of a problem though.

Copy link
Member

@niaow niaow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what is going on with tinyhci right now, but this looks good to me

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 this pull request may close these issues.

4 participants