Gazelle doesn't know how to handle vendored proto deps about bazel-gazelle HOT 15 OPEN

Okay, I dug into this further, and I have a more crisp analysis now. When importing a 3rd party lib through go_repository:

• Adding a build directive to gazelle:exclude **/**.proto does half of what I want -- it removes the problematic rules as the repo is being analyzed.
• Setting build_file_proto_mode = "disable_global" does the other half of what I want -- it forces 3rd-party-to-3rd-party deps to be Go lib deps rather than Proto deps.

However-- the two don't work together. Setting disable_global short-circuits both proto/generate, and go/generateProto, which means my explicit gazelle:exclude **/**.proto not actually remove any rules. When protos are enabled, the exclusion will remove the rules.

So at the moment, I believe I'm still stuck with a fork here.

from bazel-gazelle.

Could you tell me a bit more?

• How did you run Gazelle?
• What went wrong?
• What did you expect?

When I ran Gazelle in that directory, I got this:

load("@io_bazel_rules_go//go:def.bzl", "go_library")
load("@io_bazel_rules_go//proto:def.bzl", "go_proto_library")

proto_library(
    name = "annotations_proto",
    srcs = [
        "annotations.proto",
        "http.proto",
    ],
    visibility = ["//visibility:public"],
    deps = [
        "//google/api:api_proto",
        "@com_google_protobuf//:descriptor_proto",
    ],
)

go_proto_library(
    name = "annotations_go_proto",
    importpath = "github.com/google/googleapis/third_party/googleapis/google/api",
    proto = ":annotations_proto",
    visibility = ["//visibility:public"],
    deps = [
        "//google/api:go_default_library",
        "@com_github_golang_protobuf//protoc-gen-go/descriptor:go_default_library",
    ],
)

go_library(
    name = "go_default_library",
    embed = [":annotations_go_proto"],
    importpath = "github.com/google/googleapis/third_party/googleapis/google/api",
    visibility = ["//visibility:public"],
)

Some problems jump out:

• In annotations_proto, the dependency //google/api:api_proto is unusable. That's from the imported file though, and I don't think we can set include paths (they're supported to be relative to the repository root). There's a similar issue in annotations_go_proto.
• In annotations_go_proto and go_default_library, the importpath is wrong. Should be google.golang.org/genproto/googleapis/api/annotations from the go_package directive.

Anything else?

from bazel-gazelle.

I think I am also hitting this in kubernetes/test-infra.

kubernetes/test-infra#6199 (comment)
https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/test-infra/6199/pull-test-infra-bazel/11864/

Reproduce by cloning this branch and run:

bazel query --keep_going --noshow_progress "kind(.*_binary, rdeps(//..., //...)) except attr('tags', 'manual', //...)"

If I update the proto rules bazel query complains about, eg.

proto_library(
    name = "v1_proto",
    srcs = ["generated.proto"],
    deps = [
        "//k8s.io/apimachinery/pkg/runtime:runtime_proto",
        "//k8s.io/apimachinery/pkg/runtime/schema:schema_proto",
        "//k8s.io/apimachinery/pkg/util/intstr:intstr_proto",
    ],
)

to

proto_library(
    name = "v1_proto",
    srcs = ["generated.proto"],
    deps = [
        "//vendor/k8s.io/apimachinery/pkg/runtime:runtime_proto",
        "//vendor/k8s.io/apimachinery/pkg/runtime/schema:schema_proto",
        "//vendor/k8s.io/apimachinery/pkg/util/intstr:intstr_proto",
    ],
)

bazel query is happy but when I run gazelle, it rewrites the rule back to the original. gazellle runs as part of hack/update-bazel.sh.

from bazel-gazelle.

bazel query is happy but when I run gazelle, it rewrites the rule back to the original. gazellle runs as part of hack/update-bazel.sh.

FWIW, I verified the rewritting is by gazelle by commenting out kazel that runs in the end of update-bazel.sh.

from bazel-gazelle.

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.

from bazel-gazelle.

I am running in a similar issue using import "google/rpc/status.proto";:

Gazelle incorrectly adds the lines
//google/rpc:rpc_proto and //google/rpc:go_default_library below, where-as the correct imports are @com_github_googleapis_googleapis//:status_proto (specified in the WORKSPACE as a new_http_archive with custom BUILD files) and @org_golang_google_genproto//googleapis/rpc/status:go_default_library (normal go_repository) respectively.

See the complete targets:

proto_library(
    name = "users_proto",
    srcs = ["users.proto"],
    visibility = ["//visibility:public"],
    deps = [
        "//google/rpc:rpc_proto",
        "@com_github_googleapis_googleapis//:status_proto",  # keep
        "@com_google_protobuf//:wrappers_proto",
    ],
)

go_proto_library(
    name = "users_go_proto",
    compilers = ["@io_bazel_rules_go//proto:go_grpc"],
    importpath = "gitlab.com/company/project/users/proto/users",
    proto = ":users_proto",
    visibility = ["//visibility:public"],
    deps = [
        "//google/rpc:go_default_library",
        "@com_github_golang_protobuf//ptypes/wrappers:go_default_library",
        "@org_golang_google_genproto//googleapis/rpc/status:go_default_library",  # keep
    ],
)
``

Any way to tell gazelle to not add those incorrect deps apart from putting a `# keep` in front of the whole rule?

from bazel-gazelle.

FYI, I disabled proto rule generation in vendor by default in #78 since vendored protos are almost never buildable.

Thanks, this does the trick for me.

from bazel-gazelle.

@Globegitter Until Gazelle is capable of indexing libraries in other repositories, you'll need to put # keep on the entire rule and maintain it manually. Sorry that's not working yet.

from bazel-gazelle.

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.

@jayconrod Could this be why we see the following error when attempting to bazel build @com_github_prometheus_prometheus//cmd/promtool?

DEBUG: /private/var/tmp/_bazel_marius/c39204db2cb7e0a79ba2096a64d88d76/external/bazel_gazelle/internal/go_repository.bzl:184:13: gazelle: gazelle: rule //vendor/github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger/genswagger:go_default_library imports "github.com/grpc-ecosystem/grpc-gateway/internal" which matches multiple rules: //vendor/github.com/grpc-ecosystem/grpc-gateway/internal:internal_go_proto and //vendor/github.com/grpc-ecosystem/grpc-gateway/internal:go_default_library. # gazelle:resolve may be used to disambiguate
gazelle: rule //vendor/github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger/genswagger:go_default_library imports "github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger/options" which matches multiple rules: //vendor/github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger/options:go_default_library and //vendor/github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger/options:options_go_proto. # gazelle:resolve may be used to disambiguate
gazelle: rule //vendor/github.com/grpc-ecosystem/grpc-gateway/runtime:go_default_library imports "github.com/grpc-ecosystem/grpc-gateway/internal" which matches multiple rules: //vendor/github.com/grpc-ecosystem/grpc-gateway/internal:internal_go_proto and //vendor/github.com/grpc-ecosystem/grpc-gateway/internal:go_default_library. # gazelle:resolve may be used to disambiguate
compilepkg: missing strict dependencies:
	/private/var/tmp/_bazel_marius/c39204db2cb7e0a79ba2096a64d88d76/sandbox/darwin-sandbox/1/execroot/platform_bootstrap/external/com_github_prometheus_prometheus/vendor/github.com/grpc-ecosystem/grpc-gateway/runtime/handler.go: import of "github.com/grpc-ecosystem/grpc-gateway/internal"
No dependencies were provided.
Check that imports in Go sources match importpath attributes in deps.

We use build_file_proto_mode = "disable_global" in the repository rule but still seeing proto rules in the generated build file. Possibly because grpc-ecosystem already has a build file with proto rules? https://github.com/grpc-ecosystem/grpc-gateway/blob/master/internal/BUILD.bazel

The repository is defined as follows:

go_repository(
    name = "com_github_prometheus_prometheus",
    importpath = "github.com/prometheus/prometheus",
    urls = ["https://github.com/prometheus/prometheus/archive/v{}.tar.gz".format(PROMETHEUS_VERSION),],
    strip_prefix = "prometheus-{}".format(PROMETHEUS_VERSION),
    sha256 = "180ce60faae413308db6bb21d83104ed345797a7a3998966869e564717b73347",
    build_file_proto_mode = "disable_global",
)

from bazel-gazelle.

@mariusgrigoriu disable_global won't cause Gazelle to delete existing rules. Gazelle can't distinguish these from manually written rules, and we avoid deleting manually written stuff when possible.

Also disable_global will only apply to the repository where it's written. Depending on what you need to do, you may need to set it on grpc-ecosystem. You may also need to set build_file_generation = "on" (to force Gazelle to run when build files are already present), and you might need to set patches and related attributes to make additional modifications.

from bazel-gazelle.

@jayconrod we ran into this exact issue with github.com/grpc-ecosystem/[email protected]. The problem is that particular repository has an existing https://github.com/grpc-ecosystem/grpc-gateway/blob/master/internal/BUILD.bazel that already contains embedded proto rules. Even if set it up this way:

    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.

from bazel-gazelle.

@dragonsinth For repositories that already have their own build files, Gazelle probably shouldn't run; they should be fetched using http_archive or go_repository with build file generation disabled, and patches can be applied to fix them up as needed.

It's very difficult for Gazelle to decide the right thing to do here. I definitely don't think removing all proto_library and go_proto_library targets in Fix is correct though. At minimum, it ignores # keep comments and blows away manual changes.

from bazel-gazelle.

@dragonsinth For repositories that already have their own build files, Gazelle probably shouldn't run; they should be fetched using http_archive or go_repository with build file generation disabled, and patches can be applied to fix them up as needed.

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 @io_bazel_rules_go//proto/wkt:any_go_proto and @org_golang_google_protobuf//proto try to pass a library to the linker.

That's why I'm explicitly forcing build file generation and proto disabling in the go_repository rule.

It's very difficult for Gazelle to decide the right thing to do here. I definitely don't think removing all proto_library and go_proto_library targets in Fix is correct though. At minimum, it ignores # keep comments and blows away manual changes.

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 test rule when you delete the last _test.go from a package. Where should I be looking in the code?

from bazel-gazelle.

That's why I'm explicitly forcing build file generation and proto disabling in the go_repository rule.

My approach for repos like this would be to check out the repo somewhere, copy the directory, run Gazelle and make manual changes, then diff -urN the two directories to get a patch that can be used with http_archive.

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.

I want to make this work exactly the same way as gazelle would remove a test rule when you delete the last _test.go from a package. Where should I be looking in the code?

Gazelle should usually delete rules by returning them through the Empty list in GenerateRules. If they're empty after merging, they'll be deleted.

Fix was intended to transform build files following old conventions, usually only if the fix command is used. For example, rules_go used to require separate go_default_test and go_default_xtest targets if a package had both internal and external tests. There's a Fix transformation that squashes the two targets, but since it can delete targets that might be mentioned in a test_suite or something, that transformation is conditionally enabled.

from bazel-gazelle.

My approach for repos like this would be to check out the repo somewhere, copy the directory, run Gazelle and make manual changes, then diff -urN the two directories to get a patch that can be used with http_archive.

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.

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.

Gazelle should usually delete rules by returning them through the Empty list in GenerateRules. If they're empty after merging, they'll be deleted.

Fix was intended to transform build files following old conventions, usually only if the fix command is used. For example, rules_go used to require separate go_default_test and go_default_xtest targets if a package had both internal and external tests. There's a Fix transformation that squashes the two targets, but since it can delete targets that might be mentioned in a test_suite or something, that transformation is conditionally enabled.

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.

from bazel-gazelle.