Add protocopt option to protoc go compiler

[prestonvanloon]

What type of PR is this?

Feature

What does this PR do? Why is it needed?

This PR allows for downstream projects to define a go proto compiler that requires additional protoc arguments.

Example usage:

go_proto_compiler(
    name = "go_cast_grpc",
    options = [
        "plugins=grpc",
    ],
    plugin = ":protoc-gen-go-cast",
    visibility = ["//visibility:public"],
    deps = [
        "@com_github_golang_protobuf//proto:go_default_library",
        "@org_golang_google_grpc//:go_default_library",
        "@org_golang_google_grpc//codes:go_default_library",
        "@org_golang_google_grpc//status:go_default_library",
        "@org_golang_google_protobuf//reflect/protoreflect:go_default_library",
        "@org_golang_google_protobuf//runtime/protoimpl:go_default_library",
    ],
    protocopts = ["--experimental_allow_proto3_optional"], # Additional requirement for this compiler to use proto3 optionals
)

Which issues(s) does this PR fix?

Fixes #2673

Other notes for review

Opening as a draft to solicit early review.
This is missing any form of tests would like to receive some feedback from maintainers before proceeding.

Ideally, this functionality would be better implemented if the --protocopt flag was exposed via the starlark API, i.e. --protocopt=--experimental_allow_proto3_optional in the command line, however, it does not seem possible to access this flag in starlark. So, I have taken this approach to allow the arguments to be provided as target attributes on the go_proto_compiler rule.

[jayconrod]

@bazelbuild/go-maintainers Please comment if you have any thoughts on this PR.

[achew22]

LGTM, would it be possible to add a testcase that exercises this?

[prestonvanloon]

Added test scenario for this PR. Thanks!

[achew22]

Hrm... I triggered re-run on the windows break 3 times just to get more data. I have absolutely NO idea what that means...

[prestonvanloon]

Maybe something to do with the update in protoc. I didn't test anything for windows :(

[achew22]

Yeah, I don't really know what to do here. I don't have a windows box to test on. @jayconrod do you have a windows box for debugging?

[jayconrod]

I think this is protocolbuffers/protobuf#8049: protoc 3.14 doesn't build with MinGW on Windows.

We probably need to exclude this test on Windows in .bazelci/presubmit.yml (both in the build and test) sections. The other new targets can have tags = ["manual"] to prevent //... from matching them.

[achew22]

@jayconrod I've seen a couple of projects tag as nowindows and then put a test filter in their CI config. Is there a reason to exclude the target in the CI config over setting up a tag to handle this generically?

[jayconrod]

No strong opinion, but if a target fundamentally doesn't work on Windows (for example, if it tests linkmode = "plugin" which isn't supported on Windows), I think it's fine to tag the target itself with something that prevents it from being tested on Windows. If a target should work on Windows but doesn't due to some external issue, then I think we should (hopefully temporarily) disable it in CI.

At the moment, we basically don't have test coverage for go_proto_library on Windows, which is... not good. I hope we can fix that in the future and re-enable these tests.

[ah-edg]

This PR should no longer be required for field presence support since 3.15 (where "optional" is enabled by default).
It still looks like a sensible addition. The recently released protoc 3.15.1 contains the Windows build fix, might help with the CI without disabling tests on Windows...

[prestonvanloon]

@ah-edg Excellent! Let's close this. Users of proto3 optional can update to protoc 3.5.0 or later.

[prestonvanloon]

Just to clarify, I decided to close this since the experimental protoc flag was the use case to support this functionality.
If anyone else has any use case for this feature, I would be happy to push it forward. However, I would not want to add this feature and it never be used.