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

typeurl: make marshal_test.go compile again and then pass #8

Closed
wants to merge 1 commit into from

Conversation

rsc
Copy link

@rsc rsc commented Jun 27, 2018

The test was importing the old import path instead of the new
import path, so it wasn't testing the code in this directory.
(And the code it was trying to test no longer exists.)

After making the test build, change the expected type
identifiers (per f694355) to make it also pass.

The test was importing the old import path instead of the new
import path, so it wasn't testing the code in this directory.
(And the code it was trying to test no longer exists.)

After making the test build, change the expected type
identifiers (per f694355) to make it also pass.
@crosbymichael
Copy link
Member

I don't see how the protos are not getting registered in the tests now. I added a t.Log(proto.MessageName(&eventsapi.TaskStart{})) at the top of the test and it returns an empty url....

@rsc
Copy link
Author

rsc commented Jun 27, 2018

@crosbymichael Sorry for sending broken code. It worked for me with vgo. It didn't occur to me it would fail with plain go.

The problem is that github.com/containerd/containerd has vendored github.com/gogo/protobuf/proto. Inside containerd/typeurl, the import "github.com/gogo/protobuf/proto" means the actual github.com/gogo/protobuf/proto, but inside containerd/containerd, the same importreally means github.com/containerd/containerd/vendor/github.com/gogo/protobuf/proto - a different copy. The eventsapi.TaskStart is registered with the vendored copy but the call to proto.MessageName is going into the top-level version, where TaskStart is in fact unregistered.

This worked with vgo because it eliminates this ambiguous nonsense: an import path like github.com/gogo/protobuf/proto is restricted to mean the same thing everywhere in the build.

If you want people to be able to import and use containerd/containerd things from other packages (like containerd/typeurl), then the solution is to take gogo/protobuf out of the vendor directory. If you don't expect that, then the solution is to make this test not reach into containerd/containerd for a test proto.

@crosbymichael
Copy link
Member

@rsc makes sense.

Do you think it's ok to generate some protobuf as test data in this repo for testing?

@rsc
Copy link
Author

rsc commented Jun 27, 2018

@crosbymichael Sure, that makes sense to me. I'll leave that for you though.

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

Successfully merging this pull request may close these issues.

None yet

2 participants