preliminary Go modules support #274
Conversation
#### Summary The heart of the change is very small -- we stop overwriting the user's value for the `GO111MODULE` env var in two places. This means we end up respecting whatever the user has set for `GO111MODULE`, and relying on `cmd/go` to interpret the meaning of an unset `GO111MODULE` (which defaults to `auto` for Go 1.11-1.13, and likely will default to `on` at some point during Go 1.14 development). In addition, there are some comments added, as well as an explicit check for `GOFLAGS=-mod=vendor`, which is not supported. The tests exercises 'replace' directives, v2+ modules, GO111MODULE set to on|off|auto, running inside GOPATH, running outside GOPATH, the mechanisms by which go-fuzz/go-fuzz-dep is found, as well as vendoring (which is not supported, but it tests that a sensible error is emitted). #### Additional Details Historically, go-fuzz has set up a clean GOPATH environment in a temp directory, instrumented the code for fuzzing coverage, and built the instrumented code in the temp directory. The current approach is to keep doing that (and not set up a full-blown modules-based environment for building), but use modules to find the code to instrument. v2+ modules are potentially problematic due to Semantic Import Versioning (with the major version in the import path). However, it turns out to not be too bad to handle it. For a v2+ module created following the 'major branch' approach, the /vN is materialized into a physical directory. For example, the github.com/russross/blackfriday/v2 module ends up in the /tmp/blah/gopath/src/github.com/russross/blackfriday/v2 directory. This should be a valid layout, and people use this layout today (including when manually switching to a 'major subdirectory' approach for a v2+ module, which involves creating a /v2 or /vN directory so that pre-modules toolchains follow a /v2 or /vN import path and find an on-disk layout that they expect without any knowledge of modules). The approach does not copy go.mod files at all. (Before starting, I was thinking I would need to copy go.mod files to trigger "minimal module awareness", which is a transitional mechanism within cmd/go, but after some experimentation / thinking, I concluded it was not needed based on materializing the /v2). For tests, I am using the script-driven test facility that the core Go team created for testing cmd/go, which is well tuned to testing the various permutations of modules, including it lets you relatively easily set up go.mod files and source files, run commands and check results. (Some more details of the capability here: https://github.com/golang/go/blob/master/src/cmd/go/testdata/script/README).
|
There had been a few moving parts for this:
Given item 1 is now addressed and items 2 and 3 both affect all the recent PRs equally, I think this PR is ready for review as a viable candidate. |
|
Figured this might be worth trying out and I hit a wall pretty quickly. I cloned https://github.com/thepudds/go-fuzz, added a go.mod like this: Build it by doing In my app I did this to ensure I'd get the module aware version: Then when I try to build I get this: |
|
@frioux thanks for reporting! can you please create a small github repo so we can reproduce this correctly? |
|
frioux/leatherman@2c7fe8f reproduces it in an OSS repo; just clone the repo, switch to branch fuzz, and run |
|
Hello @frioux , thanks for the extra eyes here. I really appreciate the extra scrutiny. I'm not sure though exactly what steps you were following when you were testing. You appear to have made go-fuzz itself into a module, which is not what this change supports. (We are getting progressively closer to go-fuzz being a module itself, but that is still a future issue; this change is just about fuzzing modules). Also, what branch did you use when building github.com/thepudds/go-fuzz? The changes in this PR are in the dev-modules-2 branch there. Separately, I'm not sure that FWIW, maybe I made a mistake when trying this, but these steps seemed to work for me to fuzz the module in your fuzz branch of frioux/leatherman, where the starting point was a clean machine with Go 1.13.1: which outputs: |
|
@thepudds thanks for the quick response. You're right, I tried to convert to a module, mostly because that felt easier than using GOPATH at this point, but that was surely a mistake. Even still, I tried following your steps and was unsuccessful: In case it's helpful, here's my Also, just for kicks, I did a go fuzz build, keeping the work dir. Here's the tree: |
|
@frioux I don't know that this is it, but could you remove the In other words, remove these two lines from your module's go.mod:
Also, can you send |
|
@frioux Also, this is probably not it, but could you explain the As a sanity check, could you do |
|
Ok, removed the fuzz related stuff from my module files (see frioux/leatherman@4ce327c.) The rm was to make absolutely sure I had no old version of Just to rebuild and everything, here is a removal and fresh install: |
|
@frioux Are you doing anything special with GOROOT? Are you setting it somewhere? I wonder if it is correct here, or maybe not being handled correctly. According to the error message you are seeing, GOROOT seems to be pointing to /home/frew/sdk/go1.13 when However, it probably should be pointing to /run/shm/go-fuzz-build[...]/goroot at that point? According to your tree output in #274 (comment), it appears For a new workdir, could you do: |
|
For anyone else following this PR, I would be curious if anyone else can reproduce the issue that @frioux encountered (e.g., following the steps I outlined in #274 (comment)). |
|
Here's the tops of those files: As for GOROOT; I don't explicitly set it, but it is set by the go binary itself to |
|
hey @frioux :) can you send instructions with a clean docker image i.e something like that: I think this will make a reproducible environment for everyone |
|
@frioux If you are installing the go command via For example, if I use a normally installed However, using the binary from Can you try instead with a normally installed |
|
Bypassing the wrapper ( |
|
FWIW, I believe the issue @frioux hit has nothing to do this the changes in this PR. I believe it would affect current master in the same way. (That's what I would think in theory, as well as confirmed master breaks in the same way with a quick test). |
Copy. The delay here is simply me taking the time to read and understand this thoroughly. |
|
One other quick comment in case it wasn't clear -- the actual code-level changes in my PR are very small. The only material code change is to stop setting GO111MODULE in two spots (plus some comments, and one sanity check to get a slightly friendlier error if someone tries to force vendoring to be on.... which I'd be happy to drop because its not super important). The bigger volume by far is the tests, but don't feel too obligated to spend an enormous amount of time reading them, especially if short on time. |
|
Good to know. I'm afraid merging #271 has caused a merge conflict here. Sorry. :( Can I bother you to update, at your leisure? |
|
For next steps, I didn’t quite follow what you were saying. Are you saying I should change my PR to stop respecting GO111MODULE by default, and only use the new GOFUZZ111MODULE? Or you saying my PR should respect GO111MODULE by default, but treat GOFUZZ111MODULE as an override? (If you are saying the latter, then that's what I don't quite follow -- the current escape hatch in my current PR is the user can set |
This was what I had in mind. I see now that it doesn't necessarily make a lot of sense. Sorry. Two options for fixing the merge conflict:
I'm open to either. This is what I get for merging #271 without taking the time to understand everything. |
|
I'm all for option 1; the override is undocumented and temporary. Good gofuzz users who really want modules support are likely pinning the gofuzz version, anyway. |
|
I resolved the conflict by removing GOFUZZ111MODULE. (Full disclosure: that was while waiting at the doctor's office, but the diff looks right and travis passed). One other comment is the current GOFUZZ111MODULE implementation can fail with confusing error messages for some modules depending on how you specify your pkg and fuzz func on the command line. (I think it might be a |
|
If it's of any help to move things along, I've started forcing module-mode fuzz builds with |
…e vendor test (#3) * travis: attempt to install cmd/testscript * testing: initial check of 'go-fuzz-build -h' output to see if any testscripts work * testing: change name of first test script * travis: check if the 'testscript' cmd seems to function at all. * testing: use the exec command to invoke 'go-fuzz-build -h' * testing: create initially failing test case for v2 modules * travis: run testscript for v2 modules * testing: remember to use 'exec' to invoke go-fuzz-build * testing: adjust comments in v2 module testscript * go-fuzz-build: preliminary module support * testing: flip from an expected failure to an expected pass for a module test * go-fuzz-build: update comments for preliminary modules support * go-fuzz: preliminary modules support for go-fuzz itself * travis: go-fuzz-defs sanity check * travis: additional temp sanity check * travis: add more comments, some additional sanity checks in case we need to debug in future * travis: s/go-fuzz-deps/go-fuzz-dep/ * testing: validate behavior of v2 modules * testing: renamed fuzz_help.txt * testing: update travis to use the renamed testscripts * travis: conditionally set GO111MODULE=auto, and test Go 1.13 rather than 1.11 * testing: validate modules outside GOPATH * testing: validate modules inside GOPATH * testing: invoke timeout properly * testing: preliminary test for vendored modules (not yet supported) * testing: validate how go-fuzz-dep and go-fuzz-defs are found * travis: run the latest testscripts for modules testing * testing: fix typo in mod_outside_gopath.txt file name * travis: fix typo in file name * go-fuzz-build: detect -mod=vendor * testing: validate we get a proper error for -mod=vendor * readme: describe preliminary modules support * readme: correct typo in modules support * readme: adjust modules support section * readme: tweak wording based on PR review * go-fuzz: tweak comment wording by removing "preliminary modules support" * go-fuzz-build: tweak comment wording by removing "preliminary modules support" * mod_vendor.txt: avoid triggering Go 1.14 auto detection of a vendor directory
|
@josharian @mvdan I attempted to address or respond to each of your comments/questions. PTAL when convenient. Thanks for the feedback! |
|
My comments were more suggestions than concerns per se. All are resolved. |
|
LGTM then. I'll merge as soon as there are no conflicts and CI is green. |
|
The prior CI failure looks like travis flaked out on Mac for Go 1.13. (That VM failed to boot; other versions on Mac passed). Re-triggered travis and they all seem to have passed. Thanks very much @josharian and @mvdan for the review!! |
|
I feel like we need to throw a go-fuzz module support party now :) |
|
Woohoo! Can't wait to fuzz Caddy 2 😄 Thanks for this everyone! |
Summary
The heart of the change is very small -- we stop overwriting the user's value for the
GO111MODULEenv var in two places. This means we end up respecting whatever the user has set forGO111MODULE, and rely oncmd/goto interpret the meaning of an unsetGO111MODULE(which defaults toautofor Go 1.11-1.13, and likely will default toonat some point during Go 1.14 development).In addition, there are some comments added, as well as an explicit check for
GOFLAGS=-mod=vendor, which is not supported.The tests exercise 'replace' directives, v2+ modules, GO111MODULE set to on|off|auto, running inside GOPATH, running outside GOPATH, specifying pkg/func arguments to go-fuzz-build/go-fuzz, not specifying those arguments, the mechanisms by which go-fuzz/go-fuzz-dep is found, as well as vendoring (which is not supported, but it tests that a sensible error is emitted).
Fixes #195
Additional Details
Historically, go-fuzz has set up a clean GOPATH environment in a temp directory, instrumented the code for fuzzing coverage, and built the instrumented code in the temp directory.
The current approach is to keep doing that (and not set up a full-blown modules-based environment for building), but use modules to find the code to instrument.
v2+ modules are potentially problematic due to Semantic Import Versioning (where the major version is placed in the import path, such as github.com/my/mod/v2). However, it turns out to not be too bad to handle it. For a v2+ module created following the 'major branch' approach, the /vN is materialized into a physical directory. For example, the github.com/russross/blackfriday/v2 module ends up in the /tmp/blah/gopath/src/github.com/russross/blackfriday/v2 directory. This should be a valid layout, and people use this layout today (which is called the 'major subdirectory' approach for a v2+ module).
The approach does not copy go.mod files at all. (Before starting, I was thinking I would need to copy go.mod files to trigger "minimal module awareness", which is a transitional mechanism within cmd/go, but after some experimentation / thinking, I concluded it was not needed based on materializing the /vN).
For tests, I am using the script-driven test facility that the core Go team created for testing cmd/go, which is well tuned to testing the various permutations of modules, including it lets you relatively easily set up go.mod files and source files, run commands and check results. (Some more details of the capability here: https://github.com/golang/go/blob/master/src/cmd/go/testdata/script/README).