compile out gofuzz from prod binaries#92491
Conversation
|
Here's a quick prototype of eliminating the fuzzer from our binaries, closing the oldest open SIG testing bug in this repo. This sort of thing is also unfortunately not available under bazel, we probably care most about this in releases anyhow. I have not yet audited where else we need to do this, I plan to do that once the approach is cleared. We do use a similar approach already for the providerless build KEP and dockershim-less building. We could alternatively have a positive tag, and require our tests enable it, but that's a breaking change for external consumers of types like this. This way the change only affects our binaries and doesn't break bazel, similar to the opt-in "compile out cloud providers" |
476ec6c to
de11720
Compare
|
/retest |
|
if we want this to stick we need a check that the built binaries don't include gofuzz symbols |
|
I'm not very familiar with why gofuzz is problematic to have built into binaries... does it actually cause problems or is this just a representative "should be test-only" dependency? if the latter, do we want fuzz-specific build tags or do we really want a "notest" build tag? |
|
As far as I can tell just because it should be test, it's harder than other
test code because for a type to define fuzzing for itself it has to
implement an interface method involving a concrete gofuzz type parameter.
I'm not sure that we have something else like this with other packages.
I'm also not sure how important it is that this stick. I beleive this is
just about avoiding the smell / binary bloat of linking in the fuzzer
runtime used only at (unit) test time in prod.
…On Thu, Jun 25, 2020, 07:08 Jordan Liggitt ***@***.***> wrote:
I'm not very familiar with why gofuzz is problematic to have built into
binaries... does it actually cause problems or is this just a
representative "should be test-only" dependency? if the latter, do we want
fuzz-specific build tags or do we really want a "notest" build tag?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#92491 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAHADK4C4IIV5HKC72GNXXLRYNK6BANCNFSM4OHNBCPA>
.
|
thockin
left a comment
There was a problem hiding this comment.
The reason the bug was filed was definitely "we shouldn't compile test code into prod" binaries. How important is that? I don't know. There are other places where the FakeFoo is in the same file or same packages as the Foo, too.
I don't hate the pattern. I'm flummoxed why Go doesn't have this handled by default. The natural approach would be to decorate these as +build testonly but that requires anyone who runs go test to get it right. Given that releases are well-controlled, that seems like the right path, so i concur with your approach.
I like the idea of generalizing this to something like not-for-prod, so maybe +build !for_prod or something?
There was a problem hiding this comment.
I'm used to seeing this RIGHT at the top of the file - I would not think to look for it anywhere else.
I don't have super strong opinions on this, but I was thinking the only reason anyone else should toggle this is if they're consuming this code from another repo and either want access to the fuzzing behavior, or want to avoid the dependency. FWIW
I should have mentioned, I actually looked into this but it's been brought up in go and shot down, repeatedly. golang/go#21360 golang/go#14668 |
de11720 to
42acb51
Compare
|
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
|
/retest |
|
/retest |
|
this looks reasonable to me |
|
/priority backlog |
|
/assign @thockin |
thockin
left a comment
There was a problem hiding this comment.
I still don't know why Go doesn't have a test build-tag built in.
/lgtm
/approve
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: BenTheElder, thockin The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #14460
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: