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

Incomplete support for go modules #155

Closed
kispaljr opened this issue Oct 17, 2018 · 13 comments
Closed

Incomplete support for go modules #155

kispaljr opened this issue Oct 17, 2018 · 13 comments

Comments

@kispaljr
Copy link

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, because errcheck is looking for them under $GOROOT/$GOPATH, instead of $GOPATH/pkg/mod.

> errcheck -abspath .\...

C:\src\go\pkg\mod\github.com\docker\distribution@v2.7.0-rc.0.0.20181002220433-1cb4180b1a5b+incompatible\blobs.go:13:2: could not import github.com/opencontainers/image-spec/specs-go/v1 (cannot find package "github.com/opencontainers/image-spec/specs-go/v1" in any of:
	C:\Go\src\github.com\opencontainers\image-spec\specs-go\v1 (from $GOROOT)
	C:\src\go\src\github.com\opencontainers\image-spec\specs-go\v1 (from $GOPATH))

I am using the latest commit in the master branch (commit 1787c4b on Aug 7).

@domgreen
Copy link
Contributor

Seems like the PR #148 has been reverted, probably as when checking out the commits it didnt build.

@tschellenbach
Copy link

+1

jsha pushed a commit to letsencrypt/ct-woodpecker that referenced this issue Dec 18, 2018
* 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/).
@bufdev
Copy link

bufdev commented Dec 20, 2018

When can we get this un-reverted? :-)

@dominikh
Copy link
Collaborator

@kisielk

@nmiyake
Copy link
Contributor

nmiyake commented Dec 24, 2018

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.

@coopernurse
Copy link
Contributor

I built errcheck using @domgreen's fork here: https://github.com/domgreen/errcheck/commits/revert_and_fix and can confirm that fixed my issues with Go 1.11 modules.

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?

@dominikh
Copy link
Collaborator

@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.

@bufdev
Copy link

bufdev commented Dec 26, 2018

A lot of us super appreciate the work that @kisielk has done here (and your work too, @dominikh) and we rely on it every day. Anything we can do to get this fixed ASAP would be great, it’s going to become a bigger and bigger issue as more people convert to Golang modules.

@bufdev
Copy link

bufdev commented Dec 26, 2018

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

@dominikh
Copy link
Collaborator

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.

@coopernurse
Copy link
Contributor

Cool, well I just opened this: #159 based on @domgreen's fork. I removed the vendor dir and errcheck binary that he had committed, but otherwise it's unchanged.

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.

@bufdev
Copy link

bufdev commented Dec 26, 2018

I checked out your branch and installed the binary, and tested it against github.com/uber/prototool, checked out outside of my GOPATH, and it works, FYI.

@jstangroome
Copy link

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 golang:1.11 image, but these steps should be easily adjusted for non-Docker build environments:

RUN go mod vendor && \
  mkdir -p /tmp/hack/ && \
  ln -s "${PWD}/vendor" /tmp/hack/src
ENV GOPATH="${GOPATH}:/tmp/hack"

RUN errcheck ./...

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

No branches or pull requests

8 participants