-
Notifications
You must be signed in to change notification settings - Fork 139
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
Incomplete support for go modules #155
Comments
Seems like the PR #148 has been reverted, probably as when checking out the commits it didnt build. |
+1 |
* Fixes the `cttestsrv` API breakage with Trillian master from time source refactoring. * Updates the project/CI to use Go 1.11.x * Configures a `ct-woodpecker` module * Vendors dependencies[0] * Removes `errcheck`[1] from CI * Updates CI to use vendored deps throughout * Fixes a few typos flagged by Goreportcard Resolves #72 and #73 and fixes CI. [0]: Notably this pins Trillian to d0c8d5f - the tip of master at the time of writing. We're using changes that aren't in the latest v1.2.1 tag. Similarly, Trillian at tip of master requires us to pin golang/protobuf at 1d3f30b - the tip of master for that project at the time of writing. Trillian is using changes in this project that aren't in the latest v1.2.0 tag. [1]: Errcheck doesn't work with Go modules and our vendored deps. Adding it back is blocked on kisielk/errcheck#155 Personally I think the stability improvements of vendored dependencies merits removing errcheck from Travis and the cron builds. We do still get `errcheck` coverage per-PR with [GolangCI](https://golangci.com/).
When can we get this un-reverted? :-) |
Can anyone provide context on why the previous PR was reverted/what needs to be done for it to get accepted? I didn't see any information on this in the revert commit. Is the fix noted at #148 (comment) sufficient, or are there larger issues that need to be addressed? I'm interested in seeing this issue fixed and would be happy to contribute work towards doing so, but would like to verify the current state/plan before doing anything to ensure not to duplicate effort. |
I built I don't see an open PR for this branch though. I see this PR was closed: #156 Is there more work to do here? |
@nmiyake the previous PR was reverted because at the time, go/packages wasn't fit for general use, and would cause breakage with certain Go versions. |
Speaking for myself, I have errcheck (and staticcheck, unused @dominikh) commented out in https://github.com/uber/prototool/blob/dev/Makefile which is not a state I want this repository to be in - errcheck and staticcheck specifically have saved me a bunch of times |
I can't speak for @kisielk, but if someone sends a working PR with proper go/packages support for errcheck, it'll likely get merged quickly. Otherwise I can probably take a look at it myself after I've sorted out my own tools. |
Cool, well I just opened this: #159 based on @domgreen's fork. I removed the Do others want to checkout this branch and try it? I only tested it with a single Go project using go 1.11.4, so I'm not sure if there are lurking issues. |
I checked out your branch and installed the binary, and tested it against github.com/uber/prototool, checked out outside of my |
I'm currently working around the lack of Go Modules support in the released errcheck with a small hack which may help others in the short term. I build all Go code inside a Docker build based on the
|
I am trying to use
errcheck
outside of$GOPATH
in a project using go modules, but it reports import problems. More precisely it reports that modules downloaded into$GOPATH/pkg/mod
cannot find their imported dependencies, becauseerrcheck
is looking for them under$GOROOT
/$GOPATH
, instead of$GOPATH/pkg/mod
.I am using the latest commit in the master branch (
commit 1787c4b on Aug 7
).The text was updated successfully, but these errors were encountered: