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

Adding 'Options'? #54

Closed
amit-traiana opened this issue Feb 5, 2020 · 5 comments · Fixed by #106
Closed

Adding 'Options'? #54

amit-traiana opened this issue Feb 5, 2020 · 5 comments · Fixed by #106
Labels
enhancement New feature or request resolved-next-release Code to resolve issue is pending release

Comments

@amit-traiana
Copy link

Is there a way to add 'Options' to a plug-in without modifying the project's BUILD file? Passing it as argument when defining the rule? was thinking about something like that:

scala_grpc_compile(
    name = "test",
    options = [
            "java_conversions",
            "grpc",
        ],
    deps = [":testl_proto"],
)

We need to use java_conversions here, but I'm not completely OK with adding it to the scala BUILD file, as while it's very popular, not all user will want to use it.

Thanks.

@aaliddell aaliddell added the enhancement New feature or request label Feb 6, 2020
@aaliddell aaliddell added this to the 1.x.0 milestone Feb 6, 2020
@aaliddell
Copy link
Member

There used to be a similar option in the original rules_proto, but it was removed when moving to aspect based compilation. It's been a while since I looked at exactly why, but IIRC it is to do with aspects only being able to see string arguments. So it would have to become one of the following two options:

  • Pass an options_string parameter to scala_grpc_compile with the options pre-merged by the user.
  • Convert the options list given above into an options string in the scala_grpc_compile wrapper macro, much like we currently do with the verbose option here:
    def scala_proto_compile(**kwargs):
    _rule(
    verbose_string = "{}".format(kwargs.get("verbose", 0)),
    merge_directories = False,
    **{k: v for k, v in kwargs.items() if k != "merge_directories"}
    )

The second would be nicer, as users wouldn't need to know how to merge the options. The current 'recommendation' (which I agree sucks) is to copy and update the plugin definition to your local workspace and then also copy the relevant .bzl file locally to update the plugin path.

@amit-traiana
Copy link
Author

amit-traiana commented Feb 9, 2020

Second option would be nicer indeed. The problem is that unlike verbose_string that has a defined sets of arguments (0, None, 1, 2, 3 ,4), options_string will never have a predefined set of options - because each plug-in supports it own options.

If I'm following the verbose_string example, it accessable in proto_compile_aspect_impl because it's defined in proto_compile_aspect_attrs. I can't define options_string in proto_compile_aspect_attrs, because it need a specific set of values (being an aspect attribute). Got an idea how to pass that information to proto_compile_aspect_impl (the plugin options string is being built there)?

@aaliddell
Copy link
Member

Ahh, that’s the bit I was forgetting and the reason the option was dropped. In which case, there’s not a lot we can do to improve the situation until aspects are allowed more freedom with arguments. See bazelbuild/bazel#8494

You may be able to put the attr on the target, but again there may be issues with that route the I’m not remembering.

@aaliddell aaliddell added the blocked-external Waiting on external issue/release label Feb 9, 2020
@amit-traiana
Copy link
Author

amit-traiana commented Feb 9, 2020

It's a problem only with public attributes. I can easily set _options_string but options_string require values.

You may be able to put the attr on the target

What do you mean by that? the rule() definition? because it means I will be able to access it from proto_compile_impl(), but the plug-in options string is being defined and the plug-in runs on proto_compile_aspect_impl - and I can't access the attr from there.

The current 'recommendation' (which I agree sucks) is to copy and update the plugin definition to your local workspace and then also copy the relevant .bzl file locally to update the plugin path.

It's a bit more complex than that if I understand correctly? the rule is being referenced in the build file like so:

load("@rules_proto_grpc//scala:defs.bzl", "scala_grpc_compile")

It means that I have to own a copy of defs.bzl, modify load(":scala_grpc_compile.bzl", _scala_grpc_compile="scala_grpc_compile") to pin into my own scala_grpc_compile.bzl file, and also create my own scala/BUILD file so get the options from?
I'm considering maybe to Fork instead and just change the 'options'. It will also let me the options to control the dependencies versions, but It means I'll have to constantly merge.

EDIT: I ended up modifying the options = [] manually on my fork, and I will using it for now. It will be hassle to constantly merge from the upstream, but statically holding 3 files in my Bazel project (scala_grpc_compile, defs.bzl, Scala BUILD) is more risky IMO - because it means I won't be getting any updates and wouldn't be able to know any were done. Hopefully Bazel will fix the issue soon so I can come back using the main repo. I will of-course keep contributing to this repo in case I'll do more fixes on the Fork. I really want to get that gRPC Gateway bug fixed, I will try to focus on it next.

Thanks! :-)

@aaliddell
Copy link
Member

Added as part of upcoming 3.0.0: #106

You can now do:

python_proto_compile(
    name = "person_python_proto",
    protos = ["@rules_proto_grpc//example/proto:person_proto"],
    options = {
        "@rules_proto_grpc//python:python_plugin": ["some_opt"],  # <- Applies to specific plugin
    }
)

or

python_proto_compile(
    name = "person_python_proto",
    protos = ["@rules_proto_grpc//example/proto:person_proto"],
    options = {
        "*": ["some_opt"],  # <- Applied to all plugins in rule
    }
)

@aaliddell aaliddell added resolved-next-release Code to resolve issue is pending release and removed blocked-external Waiting on external issue/release labels Feb 14, 2021
@aaliddell aaliddell removed this from the 3.0.0 milestone Feb 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request resolved-next-release Code to resolve issue is pending release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants