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

gopackagesdriver skips protobuf well known types #3239

Closed
ribrdb opened this issue Jul 14, 2022 · 8 comments · Fixed by #3240
Closed

gopackagesdriver skips protobuf well known types #3239

ribrdb opened this issue Jul 14, 2022 · 8 comments · Fixed by #3240

Comments

@ribrdb
Copy link

ribrdb commented Jul 14, 2022

When using gopackagesdriver, protobufs mostly work. But any field using one of google's well known types shows up as an error in the editor.

syntax = "proto3";

import "google/protobuf/wrappers.proto";

message Foo {
  string ok = 1;
  google.protobuf.Int32Value not_ok = 2;
}

I noticed that the .pkg.json file isn't getting generated for @org_golang_google_protobuf//types/known/wrapperspb
I believe this dependency comes from the deps of the go_proto_compiler used. So I tried adding "compiler" and "compilers" to the go_pkg_info_aspect attr_aspects, but this didn't make any difference.
If I manually add "@org_golang_google_protobuf//types/known/wrapperspb" as a dep of my go_proto_library it will work, but then gazelle will remove this. Can we either get gazelle to add these implicit deps or get the aspect to understand them?

@fmeum
Copy link
Member

fmeum commented Jul 14, 2022

I have the feeling that making this explicit in BUILD files via gazelle is the more sensible approach. Could you open an issue over at bazel-gazelle so that the proper folks are made aware of this?

@ribrdb
Copy link
Author

ribrdb commented Jul 14, 2022

For reference it seems like gazelle stopped adding these dependencies because different proto compiler plugins have different targets for the well known types, and they didn't want to add that logic to gazelle.
Although if you're already adding a gazelle directive to change the proto compiler, couldn't you also add a directive to resolve well known types differently?
See #944

@fmeum
Copy link
Member

fmeum commented Jul 14, 2022

Well, that reasoning is pretty sound. Let me do a 180 here and say that I now prefer extending the aspect. Thanks for the pointer!

@achew22 What do you think?

@achew22
Copy link
Member

achew22 commented Jul 14, 2022

I think @ribrdb is correct that it caused some trouble with the proto compiler plugins. It might be better to enhance the aspect here to read the list of force included dependencies and force excluded deps. This just feels like a bug/missing feature in gopackagesdriver

@fmeum
Copy link
Member

fmeum commented Jul 15, 2022

@ribrdb I could work on this, but I can't reproduce it.

With your proto, Gazelle generates the following BUILD file for me:

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

proto_library(
    name = "aspect_proto",
    srcs = ["wkt.proto"],
    visibility = ["//visibility:public"],
    deps = ["@com_google_protobuf//:wrappers_proto"],
)

go_proto_library(
    name = "aspect_go_proto",
    importpath = "github.com/bazelbuild/rules_go/aspect",
    proto = ":aspect_proto",
    visibility = ["//visibility:public"],
)

go_library(
    name = "aspect",
    srcs = ["wkt.go"],
    embed = [":aspect_go_proto"],
    importpath = "github.com/bazelbuild/rules_go/aspect",
    visibility = ["//visibility:public"],
)

alias(
    name = "go_default_library",
    actual = ":aspect",
    visibility = ["//visibility:public"],
)

This already has the relevant proto listed as an explicit dependency. Which compiler are you using?

@ribrdb
Copy link
Author

ribrdb commented Jul 15, 2022

It's listed as a dependency on the proto_library, but the go_proto_library doesn't have any deps.
if you do

bazel build --aspects=@io_bazel_rules_go//go/tools/gopackagesdriver:aspect.bzl%go_pkg_info_aspect --output_groups=go_pkg_driver_json_file,go_pkg_driver_stdlib_json_file,go_pkg_driver_srcs aspect_go_proto

It will not generate bazel-bin/external/org_golang_google_protobuf/types/known/wrapperspb/wrapperspb.pkg.json even though the generated go files import google.golang.org/protobuf/types/known/wrapperspb. Add an explicit deps = ["@org_golang_google_protobuf//types/known/wrapperspb"] and the aspect will generate the .pkg.json file.

@fmeum
Copy link
Member

fmeum commented Jul 15, 2022

@ribrdb Thanks for the clear reproducer. Could you check whether #3240 fixes this for you?

@ribrdb
Copy link
Author

ribrdb commented Jul 15, 2022

Yes that seems to fix it. Thanks!

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

Successfully merging a pull request may close this issue.

3 participants