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

go_proto_compiler causes protoc rebuilds by enabling bootstrap_nogo #2999

Closed
fmeum opened this issue Nov 9, 2021 · 3 comments · Fixed by #3108
Closed

go_proto_compiler causes protoc rebuilds by enabling bootstrap_nogo #2999

fmeum opened this issue Nov 9, 2021 · 3 comments · Fixed by #3108

Comments

@fmeum
Copy link
Member

fmeum commented Nov 9, 2021

What version of rules_go are you using?

v0.29.0

What version of gazelle are you using?

v0.24.0

What version of Bazel are you using?

Bazel 5.0.0rc1 (also affects 4.2.1)

Does this issue reproduce with the latest releases of all the above?

Yes

What operating system and processor architecture are you using?

Linux

Any other potentially useful information about your toolchain?

What did you do?

I am using libraries that themselves use go_proto_compiler from rules_go and observer that it causes protoc to be built in multiple configurations that only differ in the value of @io_bazel_rules_go//go/private:bootstrap_nogo. The top-level exec config has this Starlark flag at its default value (false), but the go_reset_target emitted in the go_proto_compiler macro sets it to true through go_reset_transition (see https://github.com/bazelbuild/rules_go/blob/bd7fbccc635af297db7b36f6c81d0e7db7921cca/go/private/rules/transition.bzl#L239).

What did you expect to see?

protoc is only built once, in the top-level exec config.

What did you see instead?

protoc (wastefully) built multiple times.

@fmeum
Copy link
Member Author

fmeum commented Nov 9, 2021

Setting bootstrap_nogo to False fails with a cyclic dependency, so fixing this might require duplicating the rule for use in go_proto_compiler. I am not sure what the best fix is though as I do not fully understand the transition structure around nogo.

Once this issue has been resolved, I could submit a PR that fixes a similar issue with go_proto_library, which depends on protoc via its proto_library deps and does not use the go_reset_transition. Also, go_proto_compiler has a direct dependency on @com_google_protobuf//:protoc, which should also go through a go_reset_target to prevent rebuilds when e.g. a CGo binary depends on a go_proto_library and sets a linkmode.

@robfig
Copy link
Contributor

robfig commented Nov 11, 2021

bootstrap_nogo = True causes any configured nogo to be ignored when building. I'm not 100% sure if this is meant to apply to the protoc code generator (e.g. the go code generators are written in go) or the code generated by protoc or both. This could be helpful to avoid having generated code get flagged for any static analysis violations, and it would be unnecessary processing. I'm not exactly sure why the cyclic dependency exists in this case, but the general idea is that the build tools need to be built without nogo so that they can be used to build nogo itself.

Perhaps the error is that the protoc plugin does need bootstrap_nogo, but protoc itself does not and so it's an unnecessary part of the cache key. I'm not sure how to resolve it, but the first step I would take is to make a reproducer that allows easy experimentation with different options. I'm afraid that I and other rules_go maintainers are stretched far too thin at the moment, so I can't promise when I'll look into it. Any contributions would be most welcome.

@fmeum
Copy link
Member Author

fmeum commented Nov 11, 2021

That makes sense, thanks for explanation. I will figure this out for my codebase and turn it into a PR once I have fully understood the consequences.

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.

2 participants