#gazelle:proto file” directive isn’t usable unless option go_package is explicitly set in each proto_library about bazel-gazelle HOT 18 OPEN

@jeromep-stripe I guess what you suggested is the best solution. Can you make the following changes?

1. send a pull request to rules_go to add protos attribute to go_proto_library
2. send a pull request to Gazelle to add all proto_library targets that produce the same Go package to the protos attribute of the same go_proto_library. Since the current behavior is a bug, we may not need a feature flag to gate this bug fix. @wolfd, @pcj, @danny-skydio, @Michaelhobo, @jvolkman, @ggirelli what do you think?

from bazel-gazelle.

@linzhp, for 1, protos is already a known attribute in go_proto_library, right? 2 makes sense.

from bazel-gazelle.

It's sounds like a bug if gazelle generates unbuildable targets. @wolfd, @pcj, @danny-skydio, @Michaelhobo, @jvolkman, @ggirelli did you run into this issue?

from bazel-gazelle.

I tend to be using https://github.com/stackb/rules_proto for gazelle proto support, which does use the language/proto and proto_library generation, but does not use the go_proto_library rule.

I'm not saying the OP should switch to that, but I did encounter this issue when designing that alternative rule for golang: https://github.com/stackb/rules_proto/blob/9fffedbc7cd8a01ecbf7bf7d00a4d573f085bca7/pkg/rule/rules_go/go_library.go#L201.

In that solution, if you have a package with N proto files and gazelle:proto file, there are N proto_library rules and N proto_compile rules, but a single go_library (and therefore just one importpath) that names the outputs of all N proto_compile rules. Basically, the same scenario that @jeromep-stripe describes.

In some ways this defeats the purpose of gazelle:proto file for golang since you end up taking a dependency on a larger go_library than your code might need, but can be desirable to have gazelle:proto file for other languages being generated alongside go.

from bazel-gazelle.

In some ways this defeats the purpose of gazelle:proto file for golang since you end up taking a dependency on a larger go_library than your code might need, but can be desirable to have gazelle:proto file for other languages being generated alongside go.

That's exactly the situation that we find ourselves in -- we would like the individual proto_library targets exposed for non-golang purposes.

I think ideally the gazelle go language extension would bundle the proto_library targets with the same importpaths together into the output go_proto_library, regardless of what gazelle:proto mode you're in. Not doing that means generating go_proto_library targets that 1) make gazelle resolution of those importpaths ambiguous, and 2) are impossible to build together, which are both things that Gazelle should avoid at all costs, IMO.

But I recognize that, pragmatically, this probably has to exist as a separate mode, at least initially, to avoid breaking folks.

(Let me know if I'm wrong or missing obvious things here -- totally possible, we're relatively new to gazelle & go codegen.)

from bazel-gazelle.

That sounds about right to me.

I'm not an ex-googler so I don't know how things are inside G, but I think I saw some hint somewhere from @adonovan that golang within google can use bazel labels for import statements in .go files. Alternatively, perhaps that was an initial language design goal that was never realized.

If that were true generally, gazelle:proto file would make sense for golang, but for practical purposes I think it is just fundamentally incompatible with this gazelle mode.

from bazel-gazelle.

I agree that compiling multiple proto_library targets into one go_proto_library or go_library target defeats the purpose of gazelle:proto file. Suffixing the import path with proto file name, and generate one Go package per proto file when gazelle:proto file?

So example.com/foo/bar.proto will produce a go_proto_library with importpath = "example.com/foo/bar".

from bazel-gazelle.

What I do is typically one go_proto_library per proto_library with the same importpath. Not sure how gazelle is handling this though.

proto_library(
    name = "foo_proto",
    srcs = ["foo.proto"],
)

go_proto_library(
    name = "foo_pb2_go",
    compilers = [
        "@io_bazel_rules_go//proto:go_proto",
    ],
    importpath = "github.com/something/package",
    proto = ":foo_proto",
)

proto_library(
    name = "bar_proto",
    srcs = ["bar.proto"],
)

go_proto_library(
    name = "bar_pb2_go",
    compilers = [
        "@io_bazel_rules_go//proto:go_proto",
    ],
    importpath = "github.com/something/package",
    proto = ":bar_proto",
)

BUT I am indeed specifying the go_package option in every protobuf file with:

option go_package = "package";

Which is OK, imho.

from bazel-gazelle.

The other approach is to have gazelle generate what @jayconrod suggested here

from bazel-gazelle.

@ggirelli I've tried this, but it typically fails to build if foo.proto imports bar.proto

from bazel-gazelle.

@jeromep-stripe gotcha. I generally avoid importing between proto files in the same package so I have not hit that issue.

from bazel-gazelle.

Actually, if foo.bar imports bar.proto then foo_proto should depend on bar_proto. I am wondering whether foo_pb2_go's build would end up including both automatically then. Have you tried?

from bazel-gazelle.

@ggirelli So I just set up a foo.proto and bar.proto in a directory, where foo.proto imports bar.proto. I used #gazelle:proto file, and here's the generated BUILD file

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

# gazelle:proto file

proto_library(
    name = "bar_proto",
    srcs = ["bar.proto"],
    visibility = ["//visibility:public"],
)

proto_library(
    name = "foo_proto",
    srcs = ["foo.proto"],
    visibility = ["//visibility:public"],
    deps = [":bar_proto"],
)

go_proto_library(
    name = "bar_go_proto",
    importpath = "git.corp.stripe.com/stripe-internal/gocode/example",
    proto = ":bar_proto",
    visibility = ["//visibility:public"],
)

go_proto_library(
    name = "foo_go_proto",
    importpath = "git.corp.stripe.com/stripe-internal/gocode/example",
    proto = ":foo_proto",
    visibility = ["//visibility:public"],
    deps = [":go_default_library"],
)

go_library(
    name = "go_default_library",
    embed = [":bar_go_proto"],
    importpath = "git.corp.stripe.com/stripe-internal/gocode/example",
    visibility = ["//visibility:public"],
)

It's a bit weird that foo_go_proto depends on :go_default_library. I tried building foo_go_proto and got:

> bazel build //example:foo_go_proto 
...
ERROR: /Users/jeromep/stripe/gocode/example/BUILD.bazel:27:17: GoCompilePkg example/foo_go_proto.a failed: (Exit 1): builder failed: error executing GoCompilePkg command (from target //example:foo_go_proto) bazel-out/darwin_x86_64-opt-exec-ST-13d3ddad9198/bin/external/go_sdk/builder_reset/builder compilepkg -sdk external/go_sdk -installsuffix darwin_amd64 -src ... (remaining 59 arguments skipped)

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
bazel-out/darwin_x86_64-dbg/bin/example/foo_go_proto_/git.corp.stripe.com/stripe-internal/gocode/example/foo.pb.go:29:7: undefined: Bar
bazel-out/darwin_x86_64-dbg/bin/example/foo_go_proto_/git.corp.stripe.com/stripe-internal/gocode/example/foo.pb.go:71:25: undefined: Bar
bazel-out/darwin_x86_64-dbg/bin/example/foo_go_proto_/git.corp.stripe.com/stripe-internal/gocode/example/foo.pb.go:108:4: undefined: Bar
bazel-out/darwin_x86_64-dbg/bin/example/foo_go_proto_/git.corp.stripe.com/stripe-internal/gocode/example/foo.pb.go:124:2: undefined: file_example_bar_proto_init
compilepkg: error running subcommand external/go_sdk/pkg/tool/darwin_amd64/compile: exit status 2
Target //example:foo_go_proto failed to build
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 0.758s, Critical Path: 0.28s
INFO: 2 processes: 2 internal.
ERROR: Build did NOT complete successfully

I get the same outcome when I change foo_go_proto's deps to be [":bar_go_proto"].

from bazel-gazelle.

Interesting. Are you using

option go_package = "example";

in your proto files? Also, what is the package in your proto files?

Can you try

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

# gazelle:proto file

proto_library(
    name = "bar_proto",
    srcs = ["bar.proto"],
    visibility = ["//visibility:public"],
)

proto_library(
    name = "foo_proto",
    srcs = ["foo.proto"],
    visibility = ["//visibility:public"],
    deps = [":bar_proto"],
)

go_proto_library(
    name = "bar_go_proto",
    importpath = "git.corp.stripe.com/stripe-internal/gocode/example",
    proto = ":bar_proto",
    visibility = ["//visibility:public"],
)

go_proto_library(
    name = "foo_go_proto",
    importpath = "git.corp.stripe.com/stripe-internal/gocode/example",
    proto = ":foo_proto",
    visibility = ["//visibility:public"],
    deps = [":bar_go_proto"],
)

go_library(
    name = "go_default_library",
    embed = [":bar_go_proto", ":foo_go_proto"],
    importpath = "git.corp.stripe.com/stripe-internal/gocode/example",
    visibility = ["//visibility:public"],
)

(in the above I changed :foo_go_proto's deps and :go_default_library's embed)

and then run

bazel build //example:go_default_library --verbose_failures 

?

But, generally, I would really advise against imports between proto files in the same package. If you are importing they should either be in different packages or in the same proto file. If you are splitting them in two files because you reuse some of the messages, you should really move them somewhere else instead.

from bazel-gazelle.

@ggirelli
Earlier, I didn't set the option go_package in either .proto file (this isn't really an easy thing to enforce for our repo). But I tried it out just now, and it seems that setting option go_package = "example" makes no difference in theimportpath (it's set to "git.corp.stripe.com/stripe-internal/gocode/example" regardless) . Here are the file contents I'm using:

// example/bar.proto
syntax = "proto3";

package com.stripe.gocode.example;

message Bar {
  string name = 1;
}

// example/foo.proto
syntax = "proto3";

package com.stripe.gocode.example;
import "example/bar.proto";

message Foo {
  string name = 1;
  Bar bar = 2;
}

I tried using your BUILD file contents, and bazel build //example:go_default_library --verbose_failures does build successfully, but bazel build //example:foo_go_proto still fails with the same issue I mentioned earlier (I'm surprised that this is the case, since go_default_library embeds foo_go_proto).

What does work and build successfully is having a single go_proto_library, sort of like this

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

# gazelle:proto file

proto_library(
    name = "bar_proto",
    srcs = ["bar.proto"],
    visibility = ["//visibility:public"],
)

proto_library(
    name = "foo_proto",
    srcs = ["foo.proto"],
    visibility = ["//visibility:public"],
    deps = [":bar_proto"],
)

go_proto_library(
    name = "foobar_go_proto",
    importpath = "git.corp.stripe.com/stripe-internal/gocode/example",
    protos = [":foo_proto", ":bar_proto"],
    visibility = ["//visibility:public"],
)

go_library(
    name = "go_default_library",
    embed = [":foobar_go_proto"],
    importpath = "git.corp.stripe.com/stripe-internal/gocode/example",
    visibility = ["//visibility:public"],
)

from bazel-gazelle.

@linzhp Question about requirements: On these lines, gazelle ensures that in # gazelle:proto package mode, a go_library target (go_default_library) does not get generated unless there is a .go file in the package. The same is not true in # gazelle:proto file mode (a go_default_library gets generated even without any .go files). I feel that, just like with package mode, we should not be generating the go_default_library in file mode. This can be done by changing the check to

if pkg != nil && (pcMode == proto.PackageMode || pcMode == proto.FileMode) && pkg.firstGoFile() == "" {

But this might create many changes in repos that currently use gazelle (with file mode). Should we continue generating the go_library target or not in the PR?

from bazel-gazelle.

I think we should not generate go_library in file mode. There is no such thing as "default go library" when we are in package mode or file mode, because these modes allows more than one Go package in the same directory by design.

from bazel-gazelle.

@linzhp I was discussing with @ouguoc2-stripe whether generating a go_library makes sense in file mode. In a situation where there are multiple importpaths, we can't have a default go_library. But when only a single importpath exists in the package (e.g., multiple go_proto_library targets with the same importpath), it might make sense to generate a default go_library. What do you think about generating a go_default_library in cases where it's unambiguous to do so?

from bazel-gazelle.