Skip to content
This repository has been archived by the owner on Jun 27, 2023. It is now read-only.

handle golang.org/x/net/context & context import collisions with source mode #156

Closed
Rajczyk opened this issue Mar 4, 2018 · 8 comments · Fixed by #405
Closed

handle golang.org/x/net/context & context import collisions with source mode #156

Rajczyk opened this issue Mar 4, 2018 · 8 comments · Fixed by #405

Comments

@Rajczyk
Copy link

Rajczyk commented Mar 4, 2018

So I've just been testing mock with protocol buffer generated code on a simple .proto file

syntax = "proto3";

package pb;

// A generic empty message
message Empty {

}

// The Add service definition.
service Random {
    // Gets a random number.
    rpc Get (Empty) returns (stream GetReply) {}
}

// The get response contains the random number.
message GetReply {
    int64 v = 1;
    string err = 2;
}

To generate the above you can do

Install proto3
https://github.com/google/protobuf/releases
mv protoc/bin/protoc /usr/local/bin/protoc

Update protoc Go bindings via
go get -u github.com/golang/protobuf/{proto,protoc-gen-go}

protoc random.pb --go_out=plugins=grpc:.;

The important part to notice of this definition is the usage of streams which causes an interface of interfaces to get generated by protoc

type Random_GetClient interface { Recv() (*GetReply, error) grpc.ClientStream }

When using mockgen on this file I get "imported package collision: context imported twice".
Now I understand the need for an error here because its possible to have a naming collision. In this case however the problem is because golang.org/x/net/context & context paths are actually the same package.

I'm not sure of a way to test two imports are the same but I made a less generic fix here: https://github.com/Rajczyk/mock

@mbana
Copy link

mbana commented Apr 1, 2018

Please see this PR: #163.

It documents how you should be using mockgen when using grpc streams.

Summary: use the reflect invocation and that appear should disappear.

linuxuser586 added a commit to linuxuser586/mock that referenced this issue Oct 19, 2018
This addresses the issue where a package imports another package with context
and golang.org/x/net/context in the same included package. This is leagal since
golang.org/x/net/context is an alias for context. We still don't want other
aliases to context to pass. There are tests that varify that only context and
golang.org/x/net/context are compatable for this special case.
jchampio added a commit to jchampio/gpupgrade that referenced this issue Nov 8, 2018
Oops, looks like using gRPC streams triggers a bug in mockgen, which
requires using reflect-mode invocation as a workaround:

    golang/mock#156

Luckily, it looks like we only have two things to mock in this file.
jchampio added a commit to jchampio/gpupgrade that referenced this issue Nov 8, 2018
Oops, looks like using gRPC streams triggers a bug in mockgen, which
requires using reflect-mode invocation as a workaround:

    golang/mock#156

Luckily, it looks like we only have two things to mock in this file.
@stevendanna
Copy link
Contributor

stevendanna commented Feb 14, 2020

We've recently hit what I believe is a similar bug to this when trying to use mockgen on a protobuf generated go file that includes a streaming client. After looking at the code, I believe I know the root cause of the bug:

The inclusion of a streaming client forces mockgen to parse the grpc package for an interface definition that lives in the grpc package:

https://github.com/golang/mock/blob/master/mockgen/parse.go#L259

During this parsing, mockgen fails with an error:

2020/02/14 10:48:33 imported package collision: "backoff" imported twice

The supposed conflict is caused by the fact that the grpc package imports two different packages called backoff:

vendor/google.golang.org/grpc/clientconn.go
39:	"google.golang.org/grpc/internal/backoff"

vendor/google.golang.org/grpc/backoff.go
19:// See internal/backoff package for the backoff implementation. This file is
27:	"google.golang.org/grpc/backoff"

According to the Golang spec, imports are file-scoped, so these are legal imports. However, when parsing the package, mockgen merges all files in the package:

https://github.com/golang/mock/blob/master/mockgen/parse.go#L199

and then calls importsOfFile which does conflict detection as if they were all imported into the same file.

This issue and other related issues have had comments and labels indicating that this is considered a documentation bug and users should be using reflect mode rather than source mode. I can certainly change my local invocations to work around that for now, but given the analysis above I think this should be fixable. I'm happy to look into a solution here if y'all agree this is a bug.

stevendanna added a commit to chef/automate that referenced this issue Feb 17, 2020
This moves us to "reflect-mode" by specifying the package and
interface that we want to mock rather than the source file. This
change is currently required to work around a bug in mockgen's
"source-mode". See

golang/mock#156 (comment)

for more details.

Signed-off-by: Steven Danna <steve@chef.io>
stevendanna added a commit to chef/automate that referenced this issue Feb 17, 2020
This moves us to "reflect-mode" by specifying the package and
interface that we want to mock rather than the source file. This
change is currently required to work around a bug in mockgen's
"source-mode". See

golang/mock#156 (comment)

for more details.

Signed-off-by: Steven Danna <steve@chef.io>
stevendanna added a commit to chef/automate that referenced this issue Feb 17, 2020
This moves us to "reflect-mode" by specifying the package and
interface that we want to mock rather than the source file. This
change is currently required to work around a bug in mockgen's
"source-mode". See

golang/mock#156 (comment)

for more details.

Signed-off-by: Steven Danna <steve@chef.io>
msorens added a commit to chef/automate that referenced this issue Feb 19, 2020
This moves us to "reflect-mode" by specifying the package and
interface that we want to mock rather than the source file. This
change is currently required to work around a bug in mockgen's
"source-mode". See

golang/mock#156 (comment)

for more details.

Signed-off-by: Steven Danna <steve@chef.io>
msorens added a commit to chef/automate that referenced this issue Feb 19, 2020
* Update golang/protobuf package reference to latest

Apparently I had GOFLAGS set  in my env,
which initially caused a failure:

```
$ go get -u github.com/golang/protobuf
go get: disabled by -mod=vendor
```

Unsetting GOFLAGS allowed it to proceed:
```
$ GOFLAGS="" go get -u github.com/golang/protobuf
```

Signed-off-by: michael sorens <msorens@chef.io>

* Revendor

First tried to run this outside of hab studio
but failed due to a difference between BSD and GNU `cp`. The error was:

```
cp: illegal option -- -
usage: cp [-R [-H | -L | -P]] [-fi | -n] [-apvXc] source_file target_file
       cp [-R [-H | -L | -P]] [-fi | -n] [-apvXc] source_file ... target_directory
```

Then went inside hab studio and found `make revendor` taking a very long time.
Steven Danna guided debugging this; his summation:

 * Start `make revendor`
 * Run `ps fauxww` to see what step of the process we were on
 * See that we were on `go mod tidy`
 * Get impatient
 * Use `strace -fp PID_OF_GO_MOD_TIDY` to see what it was doing.
 * Notice a lot of stats to files in node_modules.
 * Kill the revendor process, move the node_modules folder, start at the top.

(I moved node_modules out of automate-ui, chef-ui-library, and automate-workflow-web)

Signed-off-by: michael sorens <msorens@chef.io>

* Update grpc package reference to latest

Apparently the golang/protobuf package had a dependency on the latest
grpc version, so needed to update this too:

$ GOFLAGS="" go get -u google.golang.org/grpc
go: finding google.golang.org/grpc v1.27.1
go: downloading google.golang.org/grpc v1.27.1
go: extracting google.golang.org/grpc v1.27.1
go: finding golang.org/x/net latest
go: finding golang.org/x/text v0.3.2
go: finding google.golang.org/genproto latest
go: finding golang.org/x/sys latest
go: downloading golang.org/x/net v0.0.0-20200202094626-16171245cfb2
go: downloading golang.org/x/sys v0.0.0-20200202164722-d101bd2416d5
go: downloading google.golang.org/genproto v0.0.0-20200211111953-2dc5924e3898
go: extracting golang.org/x/net v0.0.0-20200202094626-16171245cfb2
go: downloading golang.org/x/text v0.3.2
go: extracting google.golang.org/genproto v0.0.0-20200211111953-2dc5924e3898
go: extracting golang.org/x/sys v0.0.0-20200202164722-d101bd2416d5
go: extracting golang.org/x/text v0.3.2

Signed-off-by: michael sorens <msorens@chef.io>

* Revendor

Moved automate-ui and chef-ui-library out of the way,
then in hab studio ran `make revendor`.
Quicker, but still took 3 or 4 minutes total.

Signed-off-by: michael sorens <msorens@chef.io>

* Fix lib/grpc unit tests

Signed-off-by: michael sorens <msorens@chef.io>

* use reflect-mode when compiling cfgmgmt client mock

This moves us to "reflect-mode" by specifying the package and
interface that we want to mock rather than the source file. This
change is currently required to work around a bug in mockgen's
"source-mode". See

golang/mock#156 (comment)

for more details.

Signed-off-by: Steven Danna <steve@chef.io>

* Regenerate for gateway and api

Exited hab studio and cleared results/* and bin/*
Then re-entered hab studio and ran these to use new vendored code:
compile_go_protobuf_component automate-gateway
compile_go_protobuf_component api

Signed-off-by: michael sorens <msorens@chef.io>

* Regenerate for compliance-service

compile_go_protobuf_component compliance-service

Signed-off-by: michael sorens <msorens@chef.io>

* Regenerate for automate-grpc

compile_go_protobuf_component automate-grpc

Signed-off-by: michael sorens <msorens@chef.io>
@Katiyman
Copy link

My protoc version is 3.11.2 and grpc is 1.27.1 but i am facing same issue

@codyoss
Copy link
Member

codyoss commented Feb 22, 2020

@stevendanna I would accept a PR that trys to make this better 😸

@codyoss codyoss changed the title handle golang.org/x/net/context & context import collisions handle golang.org/x/net/context & context import collisions with source mode Feb 22, 2020
@arjunsingri
Copy link

What are the next steps here?

@codyoss
Copy link
Member

codyoss commented Feb 28, 2020

@arjunsingri There has been a PR open to support this. In the meantime, as stated in this thread, you should be able to use reflect mode for now.

@arjunsingri
Copy link

@arjunsingri There has been a PR open to support this. In the meantime, as stated in this thread, you should be able to use reflect mode for now.

@codyoss

Can you please help me out and explain how to use the reflect mode? The doc says "list of symbols", and I wanted to understand what these symbols are.

Reflect mode generates mock interfaces by building a program that uses reflection to understand interfaces. It is enabled by passing two non-flag arguments: an import path, and a comma-separated list of symbols.

You can use "." to refer to the current path's package.

Example:

mockgen database/sql/driver Conn,Driver

# Convenient for `go:generate`.
mockgen . Conn,Driver

@codyoss
Copy link
Member

codyoss commented Mar 31, 2020

Symbols == interface names

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants