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

add support for build caching #142

Merged
merged 1 commit into from
Oct 11, 2020
Merged

add support for build caching #142

merged 1 commit into from
Oct 11, 2020

Conversation

mvdan
Copy link
Member

@mvdan mvdan commented Sep 22, 2020

No description provided.

@mvdan
Copy link
Member Author

mvdan commented Sep 22, 2020

Also, this is one of the few PRs I do want to keep as multiple commits, because it includes changes from multiple people. So the idea is to keep the number of commits low (under 5) and rebase-merge at the end.

@pagran
Copy link
Member

pagran commented Sep 22, 2020

needs tests that caching actually works.

Maybe check the garbleMapHeaderName line in the ~/.cache directory?

@mvdan
Copy link
Member Author

mvdan commented Sep 23, 2020

I think it's simpler than that, like I said on Slack - we could just check if garble build -v contains a line for a library. If it does, we're not caching. If it does not, we're caching. The main package should generally always show up if we're linking a binary.

@mvdan
Copy link
Member Author

mvdan commented Sep 27, 2020

This should now properly work with build IDs, including with Go master. Still lacks tests, though, which I'll add in the coming days.

@mvdan mvdan changed the title WIP: add support for build caching add support for build caching Oct 7, 2020
@mvdan
Copy link
Member Author

mvdan commented Oct 7, 2020

Alright, this now includes tests, a better commit message, and is ready for reviews.

pagran
pagran previously approved these changes Oct 11, 2020
Copy link
Member

@pagran pagran left a comment

Choose a reason for hiding this comment

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

Looks good for me ¯_(ツ)_/¯

main.go Show resolved Hide resolved
main.go Show resolved Hide resolved
capnspacehook
capnspacehook previously approved these changes Oct 11, 2020
Copy link
Member

@capnspacehook capnspacehook left a comment

Choose a reason for hiding this comment

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

lgtm, the previous issue of caching only happening every other build is gone, so nice work!

main.go Show resolved Hide resolved
main.go Show resolved Hide resolved
As per the discussion in golang/go#41145, it
turns out that we don't need special support for build caching in
-toolexec. We can simply modify the behavior of "[...]/compile -V=full"
and "[...]/link -V=full" so that they include garble's own version and
options in the printed build ID.

The part of the build ID that matters is the last, since it's the
"content ID" which is used to work out whether there is a need to redo
the action (build) or not. Since cmd/go parses the last word in the
output as "buildID=...", we simply add "+garble buildID=_/_/_/${hash}".
The slashes let us imitate a full binary build ID, but we assume that
the other components such as the action ID are not necessary, since the
only reader here is cmd/go and it only consumes the content ID.

The reported content ID includes the tool's original content ID,
garble's own content ID from the built binary, and the garble options
which modify how we obfuscate code. If any of the three changes, we
should use a different build cache key. GOPRIVATE also affects caching,
since a different GOPRIVATE value means that we might have to garble a
different set of packages.

Include tests, which mainly check that 'garble build -v' prints package
lines when we expect to always need to rebuild packages, and that it
prints nothing when we should be reusing the build cache even when the
built binary is missing.

After this change, 'go test' on Go 1.15.2 stabilizes at about 8s on my
machine, whereas it used to be at around 25s before.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants