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

importpath set only to package name, rather than full path (due to proto go_package option) #124

Closed
ixdy opened this issue Feb 9, 2018 · 4 comments

Comments

@ixdy
Copy link
Contributor

ixdy commented Feb 9, 2018

I'm attempting to update kubernetes/kubernetes to a more recent version of rules_go, and due to the warnings about importpath deprecation, decided to update gazelle as well, from 31ce76e to 683e26c.

Gazelle removes the importpaths outside vendor/, and correctly preserves most of the importpaths in vendor/, but a number are rewritten to include only the package. For example, some of the incorrect diffs look like

diff --git a/vendor/github.com/libopenstorage/openstorage/api/BUILD b/vendor/github.com/libopenstorage/openstorage/api/BUILD
index 2f7f752a40..24627d375e 100644
--- a/vendor/github.com/libopenstorage/openstorage/api/BUILD
+++ b/vendor/github.com/libopenstorage/openstorage/api/BUILD
@@ -13,7 +13,7 @@ go_library(
         "api.pb.go",
         "status.go",
     ],
-    importpath = "github.com/libopenstorage/openstorage/api",
+    importpath = "api",
     visibility = ["//visibility:public"],
     deps = [
         "//vendor/github.com/golang/protobuf/proto:go_default_library",
diff --git a/vendor/google.golang.org/grpc/grpclb/grpc_lb_v1/messages/BUILD b/vendor/google.golang.org/grpc/grpclb/grpc_lb_v1/messages/BUILD
index 06ab31fa94..63e7ea975d 100644
--- a/vendor/google.golang.org/grpc/grpclb/grpc_lb_v1/messages/BUILD
+++ b/vendor/google.golang.org/grpc/grpclb/grpc_lb_v1/messages/BUILD
@@ -9,7 +9,7 @@ filegroup(
 go_library(
     name = "go_default_library",
     srcs = ["messages.pb.go"],
-    importpath = "google.golang.org/grpc/grpclb/grpc_lb_v1/messages",
+    importpath = "messages",
     visibility = ["//visibility:public"],
     deps = ["//vendor/github.com/golang/protobuf/proto:go_default_library"],
 )

full diff: importpath-diff.txt

Note that kubernetes/kubernetes is a little weird, in that we have a staging/ directory which is really vendored code (intended for other kubernetes/ repos), but we symlink to these directories from vendor/ to make go happy. Gazelle basically handles these correctly, though we have a post-gazelle sed to fix the importpaths. I don't think that's affecting what's happening here, though, since these packages are incorrectly filled even without the sed, and it's affecting some of the normal vendored dependencies too.

@ixdy
Copy link
Contributor Author

ixdy commented Feb 9, 2018

It seems like there may be a strong correlation between there being a .proto / .pb.go file and these incorrect importpath attributes.

@ixdy
Copy link
Contributor Author

ixdy commented Feb 9, 2018

Yeah, this appears to be directories that have a .proto file that specify go_package that isn't fully qualified. For example, vendor/github.com/libopenstorage/openstorage/api/api.proto has

option go_package = "api";

It seems like gazelle is now using this verbatim instead of trying to resolve to the full path?

@ixdy ixdy changed the title importpath set only to package name, rather than full path importpath set only to package name, rather than full path (due to proto go_package option) Feb 9, 2018
@ixdy
Copy link
Contributor Author

ixdy commented Feb 9, 2018

oh neat, the recommended value for option go_package has changed over time: golang/protobuf#139

@jayconrod
Copy link
Contributor

Thanks for investigating and fixing this. I didn't know about the package name form. I wish there were better documentation for this on golang/protobuf.

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

2 participants