containers / virtcontainers

A Go package for building hardware virtualized container runtimes
Apache License 2.0
139 stars 43 forks source link

build: cannot build in a user fork repo #642

Closed grahamwhaley closed 6 years ago

grahamwhaley commented 6 years ago

I can build vc in its master checked out repo, but not in my user fork repo:

$~/gopath/src/github.com/grahamwhaley/containers/virtcontainers$ make
     GOBUILD  build
# github.com/grahamwhaley/containers/virtcontainers
./cni.go:87: cannot use result (type "github.com/containers/virtcontainers/vendor/github.com/containernetworking/cni/pkg/types".Result) as type "github.com/grahamwhaley/containers/virtcontainers/vendor/github.com/containernetworking/cni/pkg/types".Result in argument to convertCNIResult:
        "github.com/containers/virtcontainers/vendor/github.com/containernetworking/cni/pkg/types".Result does not implement "github.com/grahamwhaley/containers/virtcontainers/vendor/github.com/containernetworking/cni/pkg/types".Result (wrong type for GetAsVersion method)
                have GetAsVersion(string) ("github.com/containers/virtcontainers/vendor/github.com/containernetworking/cni/pkg/types".Result, error)
                want GetAsVersion(string) ("github.com/grahamwhaley/containers/virtcontainers/vendor/github.com/containernetworking/cni/pkg/types".Result, error)
Makefile:29: recipe for target 'build' failed
make: *** [build] Error 2

I believe this is a known issue - if somebody could add to this ticket any details of exactly what the problem with the CNI types are and why/how they are tied to the repo path, and any suggestions on how we effectively fix this - much appreciated.

grahamwhaley commented 6 years ago

OK, maybe now I understand the core issue, and a potential workaround.

Fundamentally, it is tied to golang packages, import paths and forks on github. We can see here: https://github.com/containers/virtcontainers/blob/master/cni.go#L25

that we directly import one of our own subdir packages:

cniPlugin "github.com/containers/virtcontainers/pkg/cni"

and, of course when you fork and do a normal go get or git clone from github if your fork, then the path where you are now building does not match that import path - and, hence the 'type clash' build failure.

One fix to get stuff to build is to rewrite all the imports to point at your new path (maybe using something like https://godoc.org/golang.org/x/tools/cmd/gomvpkg, or a script as found in the thread at https://stackoverflow.com/questions/14323872/using-forked-package-import-in-go), but that would then make a complete mess of your git diff and trying to commit patches. A better solution (also on that stackoverflow page), is probably:

Anybody want to confirm this, and ack that adding a remote repo is the best workflow method? If so, I'll add it to the developer docs.

I will also likely add info on how to pin the vendoring in cc-runtime dep files so you can test virtcontainers changes with cc-runtime, unless somebody speaks up.

sboeuf commented 6 years ago

@grahamwhaley I think this is the way to go. I don't use this for virtcontainers since I push directly onto the main repo (I know it's wrong...), but I do that for Kata Containers and it works great. The Kata agent cannot build if you call into make from your forked repo, so I develop on the main one, and I push to a new branch on my forked one.

Adding this to the doc is the best thing to do since I am not sure there is something simpler we can do about it (as you've detailed it).

amshinde commented 6 years ago

@grahamwhaley Adding a remote repo is the best workflow method. I usually clone the master/upstream repo and then continue with 2 remotes "upstream" (to point to the upstream repo) and "origin"(to point to my forked repo).

grahamwhaley commented 6 years ago

Right. My 'normal' workflow is to have those same two remotes, but to work in my forked repo :-) For vc, we need to flip that over. I'll PR a doc update.