-
Notifications
You must be signed in to change notification settings - Fork 378
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
Gazelle doesn't know how to handle vendored proto deps #33
Comments
Could you tell me a bit more?
When I ran Gazelle in that directory, I got this:
Some problems jump out:
Anything else? |
I think I am also hitting this in kubernetes/test-infra. kubernetes/test-infra#6199 (comment) Reproduce by cloning this branch and run:
If I update the proto rules
to
|
FWIW, I verified the rewritting is by |
FYI, I disabled proto rule generation in vendor by default in #78 since vendored protos are almost never buildable. Proto rules that were generated by Gazelle earlier aren't removed yet, but I'll probably put in a fix that removes them before the next version is tagged. |
I am running in a similar issue using Gazelle incorrectly adds the lines See the complete targets:
|
Thanks, this does the trick for me. |
@Globegitter Until Gazelle is capable of indexing libraries in other repositories, you'll need to put |
@jayconrod Could this be why we see the following error when attempting to
We use The repository is defined as follows:
|
@mariusgrigoriu Also |
@jayconrod we ran into this exact issue with go_repository(
name = "com_github_grpc_ecosystem_grpc_gateway",
build_directives = ["gazelle:exclude **/**_test.go", "gazelle:exclude examples"],
build_file_generation = "on",
build_file_name = "BUILD.bazel",
build_file_proto_mode = "disable_global",
importpath = "github.com/grpc-ecosystem/grpc-gateway",
sum = "h1:YuM9SXYy583fxvSOkzCDyBPCtY+/IMSHEG1dKFMLZsA=",
version = "v1.14.1",
) gazelle doesn't actually overwrite the embedded BUILD.bazel, or remove those rules. We had to patch gazelle to remove those problematic rules during the go_repository processing step: diff --git a/language/go/fix.go b/language/go/fix.go
index 0cadfdc5a6..8f81873844 100644
--- a/language/go/fix.go
+++ b/language/go/fix.go
@@ -33,6 +33,7 @@ func (_ *goLang) Fix(c *config.Config, f *rule.File) {
removeLegacyProto(c, f)
removeLegacyGazelle(c, f)
migrateNamingConvention(c, f)
+ removeGoProto(c, f)
}
// migrateNamingConvention renames rules according to go_naming_convention
@@ -361,6 +362,26 @@ func removeLegacyGazelle(c *config.Config, f *rule.File) {
}
}
+// removeGoProto removes any go_proto_library rules if protos are disabled.
+func removeGoProto(c *config.Config, f *rule.File) {
+ // Don't fix if the proto mode was set to something other than the default.
+ if pcMode := getProtoMode(c); pcMode != proto.DisableMode && pcMode != proto.DisableGlobalMode {
+ return
+ }
+
+ // Scan for definitions to delete.
+ for _, l := range f.Loads {
+ if l.Name() == "@io_bazel_rules_go//proto:def.bzl" {
+ l.Delete()
+ }
+ }
+ for _, r := range f.Rules {
+ if r.Kind() == "go_proto_library" {
+ r.Delete()
+ }
+ }
+}
+
func isGoRule(kind string) bool {
return kind == "go_library" ||
kind == "go_binary" ||
diff --git a/language/proto/fix.go b/language/proto/fix.go
index c8e67bf8fb..c24d88c358 100644
--- a/language/proto/fix.go
+++ b/language/proto/fix.go
@@ -21,4 +21,25 @@ import (
)
func (_ *protoLang) Fix(c *config.Config, f *rule.File) {
+ removeProto(c, f)
+}
+
+// removeProto removes any proto_library rules if protos are disabled.
+func removeProto(c *config.Config, f *rule.File) {
+ // Don't fix if the proto mode was set to something other than the default.
+ if pcMode := GetProtoConfig(c).Mode; pcMode != DisableMode && pcMode != DisableGlobalMode {
+ return
+ }
+
+ // Scan for definitions to delete.
+ for _, l := range f.Loads {
+ if l.Name() == "@rules_proto//proto:defs.bzl" {
+ l.Delete()
+ }
+ }
+ for _, r := range f.Rules {
+ if r.Kind() == "proto_library" {
+ r.Delete()
+ }
+ }
} I'd be happy to open a PR if you think some variant of our patch would be accepted. |
@dragonsinth For repositories that already have their own build files, Gazelle probably shouldn't run; they should be fetched using It's very difficult for Gazelle to decide the right thing to do here. I definitely don't think removing all |
Generally yes... but I can't get a clean build without taking draconian measures to prevent any go proto rules from being in the dependency graph.. otherwise I get the well-known, well-known-type package conflict where both That's why I'm explicitly forcing build file generation and proto disabling in the go_repository rule.
Great feedback here-- I'm sure that my patch is not correct. Basically-- I want to make this work exactly the same way as gazelle would remove a |
My approach for repos like this would be to check out the repo somewhere, copy the directory, run Gazelle and make manual changes, then If that's equivalent to forcing build file generation and running Gazelle, no need to take the extra steps. Just another option for more control.
Gazelle should usually delete rules by returning them through the
|
Thanks! I have come close to digging into the patch feature a couple times but shied away because it seemed hard to main and refresh patching when updating. Maybe this isn't so bad.
Makes sense, thanks for the overview. I'll take a look and see if I can't formulate this in a much more sane way. |
Okay, I dug into this further, and I have a more crisp analysis now. When importing a 3rd party lib through go_repository:
However-- the two don't work together. Setting So at the moment, I believe I'm still stuck with a fork here. |
This is where I ran into it
https://github.com/grpc-ecosystem/grpc-gateway/tree/master/third_party/googleapis
We save a subset of the googleapi protos in third_party to insulate us from ustream changes.
The text was updated successfully, but these errors were encountered: