Support passing options to plugins though a separate flag

[Monnoroch]

For example buf requires passing it's config through --buf-check-lint_opt instead of -buf-check-lint_out. This aspect doesn't support passing options though a non-out argument.

[aaliddell]

I going to close this as a duplicate of #54, which is blocked on bazelbuild/bazel#8494. You can subscribe to those two, but believe me there’s great desire to see this supported!

You can see there’s been some discussion in those two issues, but fundamentally we are stuck until Bazel itself has support. In the meantime, if you are defining your own plugins, you can specify a static list of options (again see #54 for example).

Also, I’ve not seen buf before, it looks interesting for the linter and breaking change detector. It might be worth adding as a plugin option here 🤔

[Monnoroch]

Sorry, but I was able to do it locally just fine in my $(bazel infor output_base)/external/rules_proto_grpc:

aspect.bzl:

args.add("--{}_out={}".format(plugin_name, out_arg))

+++ if plugin.extra_options:
+++     args.add_all(plugin.extra_options)

And then just piped the extra_options arg from the plugin.

Is this solution going to break anything else I'm not aware of?

[Monnoroch]

Read through the existing issue. I believe that this is not a duplicate because my idea is to define options per plugin, not per target. Although per-target options would be nice as well, but that is indeed a separate issue,

[aaliddell]

Plugins presently support passing in options, such as:

rules_proto_grpc/nodejs/BUILD.bazel

Lines 6 to 9 in 7e0a97a

	options = [
	"import_style=commonjs",
	"binary",
	],

Is this what you need?

[Monnoroch]

As I said in the initial comment, this won't work. Take a close look at the documentation. It requires it's own flag name:

All flags and config are passed as an option to the plugin as JSON. This must be done with the --buf-check-lint_opt flag as opposed to a parameter to --buf_check_lint_out as the option will include the ":" character as part of JSON.

[aaliddell]

Ok, I’ve got you now, sorry. JSON in a command line arg is somewhat nuts...but should be supportable.

In theory we should just pass all options to plugins via *_opt rather than *_out, but when this was trialled previously there was some issue with certain plugins, if I remember correctly. Looking at the protoc cli source there shouldn’t really be a difference from the plugin’s perspective, so we can try flipping it again and see what CI has to say. If it’s still a problem, it could be an opt-in plugin option.

[Monnoroch]

I can confirm that the opt-in plugin option does indeed work on quite complex proto graphs.

However, I found a small issue with the idea of a linter. I use native.existing_rules() to have an auto-generated single linter for all proto_librariys per package and add all of them as deps to the proto_compile with the plugin. It seems that the aspect is not well-prepared to handle multiple deps because I get a ton of warnings like this one:

cp: warning: source directory 'bazel-out/k8-fastbuild/bin/external/com_google_protobuf/descriptor_proto/build_proto_buf_linter_proto_aspect_verb0/_plugin_linter-plugin/.' specified more than once

This is of course for all libraries that are transitive dependencies of more than one library.

Can't say this is absolutely critical, but it's just unnecessary noise.

[Monnoroch]

See attached commit for a prototype implementation.

[Monnoroch]

I feel like output_dirs should be a depset, not a list. That change would also optimize performante.

[aaliddell]

Hi, sorry this has sat so long. Would adding a flag to each plugin that determines whether it sets options via opt or out flag resolve this? If so, the steps in the next releases would be:

• Add flag that defaults to still setting options via out.
• Flip flag to default setting options via opt

I agree re depsets, I'll add this shortly.

[aaliddell]

Added in #84

[Monnoroch]

Thanks Adam, however your implementation still doesn't work because the required flag name is --buf-check-lint_opt, not --buf_check_lint_opt. I want to once again advocate for my design which makes the call site slightly less readable but at the same time doesn't try to be smarter than the user and allows for arbitrary flag names.

UPD: honestly I'd probably also remove the custom code for the _out flag too if it wasn't for backward compatibility.

[aaliddell]

Wow this plugin is really breaking all the rules of protoc. The --plugin=name=path arg to protoc is meant to ensure that a plugin shouldn't care what name it is being referenced as.

Are you passing the plugin into the tool attribute or just relying on it being on $PATH? There's also the protoc_plugin_name attribute on the plugin that lets you force it to reference a plugin by a specific name.

[Monnoroch]

Consider the aforementioned documentation quote:

All flags and config are passed as an option to the plugin as JSON. This must be done with the --buf-check-lint_opt flag as opposed to a parameter to --buf_check_lint_out as the option will include the ":" character as part of JSON.

It mentions:

--buf-check-lint_opt
--buf_check_lint_out

So yes, the plugin is inconsistent with it's naming and thus the protoc_plugin_name solution doesn't work either. And yes, the plugin does seem to be quite loose in how it follows the protoc guidelines. However these are exactly guidelines, not strict requirements and I expect there to be all kinds of plugins that expect non-conventional flag names. It may feel fair to enforce the suggested standard naming convention but I don't think it's a practical choice as lot's of software may depend on this freedom and won't be rewritten to suite this aspect's needs.

Note that I'm in no way affiliated with buf beyond learning about it and wanting to integrate it in my project, so this is purely a user-centric opinion.

[aaliddell]

Adding this in 3.0.0 release as a 'extra_protoc_args' attr on plugins