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

Build errors when Go modules are enabled #328

Closed
dfawley opened this issue Aug 31, 2018 · 17 comments
Closed

Build errors when Go modules are enabled #328

dfawley opened this issue Aug 31, 2018 · 17 comments

Comments

@dfawley
Copy link

dfawley commented Aug 31, 2018

In grpc/grpc-go#2281, I converted grpc-go to support the new modules in Go. However, without a workaround, staticcheck gave the following error:

$ staticcheck -ignore '
> internal/transport/transport_test.go:SA2002
> benchmark/benchmain/main.go:SA1019
> stats/stats_test.go:SA1019
> test/end2end_test.go:SA1019
> balancer_test.go:SA1019
> balancer.go:SA1019
> clientconn_test.go:SA1019
> internal/transport/handler_server_test.go:SA1019
> internal/transport/handler_server.go:SA1019
> ' ./...
.../go/pkg/mod/github.com/golang/mock@v1.1.1/gomock/controller.go:60:2: could not import golang.org/x/net/context (cannot find package "golang.org/x/net/context" in any of:
	.../go111/go/src/golang.org/x/net/context (from $GOROOT)
	.../go/src/golang.org/x/net/context (from $GOPATH))
couldn't load packages due to errors: google.golang.org/genproto/googleapis/rpc/status, github.com/golang/protobuf/ptypes/any, ./channelz/service and 21 more

The workaround: make a symlink in $GOPATH/src for every package needed from $GOPATH/pkg/mod (see vet.sh immediately before running staticcheck).

I found this related issue: golang/go#26504. However, it was resolved before the 1.11 release and was marked as a release blocker, so my best guess is it was incorporated in the release. The advice in that issue is to switch to golang.org/x/tools/go/packages from go/build. Maybe that will fix all related issues?

@dominikh
Copy link
Owner

dominikh commented Sep 1, 2018

A release with Go modules support is planned, probably within 1-4 weeks. The next branch contains most of the necessary work, but go/packages still has some upstream issues with Go 1.10 that need fixing first before we can merge it into master.

@dfawley
Copy link
Author

dfawley commented Sep 4, 2018

Thanks for the update. FWIW, I tried using next in grpc-go, but found some problems: -ignore no longer works (and the -checks alternative doesn't appear to offer file-level control like -ignore did, which is unfortunate), generated code is now being checked with no way to disable AFAICT, some things are getting flagged as unused even though they are used, and we have some actual problems that we need to address. Module support is working, though -- thanks! Should I file issues against next for any of these other things?

@dominikh
Copy link
Owner

dominikh commented Sep 4, 2018

-ignore is being phased out in favour of linter directives – however, it should still be working in next. I'll check what's going on. Is your code generated from other Go code, or non-Go code? The way //line directives are being interpreted has slightly changed, with us following the directive if it points to another Go file.

There haven't been any changes to the detection of unused objects; however there are some known false positives in edge cases. Have you been running unused before and it is now reporting more issues? Or have you never used it before?

@dfawley
Copy link
Author

dfawley commented Sep 4, 2018

-ignore is being phased out in favour of linter directives – however, it should still be working in next. I'll check what's going on.

Linter directives look like a fine alternative, thanks for the pointer.

I'll check what's going on.

You can see https://github.com/dfawley/grpc-go/tree/next_staticcheck if you need an example; you can run vet.sh or just the staticcheck invocation from it. Note the SA1019 errors that should be excluded by the -ignore flag.

There haven't been any changes to the detection of unused objects; however there are some known false positives in edge cases. Have you been running unused before and it is now reporting more issues? Or have you never used it before?

We were never manually doing any unused checks; we just run staticcheck with its defaults, ignoring the SA1019 (deprecated) warnings -- mostly because we need to test our own deprecated functionality.

Thanks for the help!

@dominikh
Copy link
Owner

dominikh commented Sep 4, 2018

We were never manually doing any unused checks; we just run staticcheck with its defaults, ignoring the SA1019 (deprecated) warnings -- mostly because we need to test our own deprecated functionality.

I see. You probably want staticcheck -checks "SA*" then. On next, staticcheck behaves like the current megacheck. Do note that there will be extensive release notes once next is actually released, explaining all of these things. I'm sorry for throwing you in at the deep end.

I will take a look at next_staticcheck to see what's going on.

As for SA1019, look out for #317 getting fixed.

@dfawley
Copy link
Author

dfawley commented Sep 4, 2018

No worries, I didn't expect everything to be perfect on a development branch. I was just testing to see if we might be able to switch over, and also help beta test your module support. I think we actually will want these other checks; they seem worthwhile. I already have several PRs out to fix some things it flagged.

Also if you only want to see what happened in next_staticcheck you can look at the travis run here:
https://travis-ci.org/dfawley/grpc-go/jobs/424528608#L692 (relevant ignore line)
https://travis-ci.org/dfawley/grpc-go/jobs/424528608#L1010 (error from that file)

I was thinking our ignore syntax might not be correct (it uses newlines instead of spaces), but even after changing that it still doesn't seem to be working.

@dominikh
Copy link
Owner

dominikh commented Sep 4, 2018

Have these ignores ever worked? The file names have to include the package import path, like net/http/server.go – this holds true even when using modules. In your case, something like google.golang.org/grpc/balancer_test.go:SA1019 should work.

@dfawley
Copy link
Author

dfawley commented Sep 4, 2018

Ahhhhhhhhhhh... The ignores used to include the package path, but the hack that I put in to keep things working with staticcheck@master and our module support necessitated their removal. Thanks for the insight.

Edit: it worked! With -checks 'SA*' and the package path added, the next branch works for us now.

@dmitris
Copy link

dmitris commented Sep 21, 2018

I am having a similar issue though with a different package:

$ GOPATH=/tmp/gopath GO111MODULE=on staticcheck ./...

/tmp/gopath/pkg/mod/<custom.git.mirror.server>/golang/net.git@v0.0.0-20180921000356-2f5d2388922f/http2/frame.go:17:2: could not import golang.org/x/net/http/httpguts (cannot find package "golang.org/x/net/http/httpguts" in any of:
	/usr/local/go/src/golang.org/x/net/http/httpguts (from $GOROOT)
	/tmp/gopath/src/golang.org/x/net/http/httpguts (from $GOPATH))
couldn't load packages due to errors: golang.org/x/net/http2

It works fine with the next branch so looking forward to the release! 😄

bpicode added a commit to bpicode/fritzctl that referenced this issue Sep 21, 2018
* Issue #135.
* Unclear on how to proceed with dep. For now we keep the Gopkg.toml around.
* Repopulate vendor folder (some tools still need to work on module support, e.g. [1]).

[1] dominikh/go-tools#328
bpicode added a commit to bpicode/fritzctl that referenced this issue Sep 21, 2018
* Issue #135.
* Unclear on how to proceed with dep. For now we keep the Gopkg.toml around.
* Repopulate vendor folder (some tools still need to work on module support, e.g. [1]).

[1] dominikh/go-tools#328
bpicode added a commit to bpicode/fritzctl that referenced this issue Oct 21, 2018
* Issue #135.
* Unclear on how to proceed with dep. For now we keep the Gopkg.toml around.
* Repopulate vendor folder (some tools still need to work on module support, e.g. [1]).

[1] dominikh/go-tools#328
@bufdev
Copy link

bufdev commented Dec 20, 2018

@dominikh any updates on this?

@dominikh
Copy link
Owner

@peter-edge I will likely merge next into master some time around New Year's, as part of making the release. In the meantime, you can use the next branch.

@bufdev
Copy link

bufdev commented Dec 20, 2018

OK great, thanks for all the work as always. Any chance as part of that, we could get a go.mod/go.sum file on the next branch? I'm not sure if it's required, I can't remember what external dependencies honnef.co/go/tools has, if any.

@dominikh
Copy link
Owner

I will not be adding go.mod to the repository as part of the next release. Our versioning scheme is incompatible with semver, and most of the major releases have backwards incompatible changes to internal yet publicly accessible APIs. I don't see us moving to modules any time soon.

@bufdev
Copy link

bufdev commented Dec 20, 2018

OK, obviously up to you - of course my two cents would be to move to SemVer, perhaps do a 3000.0.0 release, and then if you do do breaking changes, stick with it. But just my opinion - no need to.

@dominikh
Copy link
Owner

Semver is inherently designed for libraries, not applications. Staticcheck is an application (whose internal APIs some people decide to use). The fact that Go ties the major version to the import path doesn't make that any easier.

@bufdev
Copy link

bufdev commented Dec 20, 2018

For the record, github.com/uber/prototool is also an application without a library, and uses SemVer, to good effect IMO. But again just my opinion.

@dominikh
Copy link
Owner

dominikh commented Jan 3, 2019

Staticcheck supports modules now.

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

4 participants