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

Allow explicit attrs in aspects that are not strings #13975

Closed
kgreenek opened this issue Sep 11, 2021 · 3 comments
Closed

Allow explicit attrs in aspects that are not strings #13975

kgreenek opened this issue Sep 11, 2021 · 3 comments

Comments

@kgreenek
Copy link

Description of the problem / feature request:

I have implemented starlark rules for flatbuffers.

They work very similarly to the protobuf rules, e.g.

load("@rules_flatbuffers//flatbuffers:flatbuffers_library.bzl", "flatbuffers_library")
load("@rules_flatbuffers//flatbuffers:cc_flatbuffers_library.bzl", "cc_flatbuffers_library")

flatbuffers_library(
    name = "foo_fbs",
    srcs = ["foo.fbs"],
)

cc_flatbuffers_library(
    name = "foo_cc_fbs",
    deps = [":foo_fbs"],
)

The cc_flatbuffers_library rule uses aspects to generate the C++ sources from the fbs schema definitions. It's the perfect use-case for aspects.

I would like to enable users to pass a custom toolchain for the language-specific rules. The flatc compiler offers several useful customizations that users in different situations will want to enable by passing flags to the flatc compiler.

Concretely, I want to enable this type of usage:

load("@rules_flatbuffers//flatbuffers:flatbuffers_library.bzl", "flatbuffers_library")
load("@rules_flatbuffers//flatbuffers:cc_flatbuffers_library.bzl", "cc_flatbuffers_library")
load("@rules_flatbuffers//flatbuffers/toolchains:cc_flatbuffers_toolchain.bzl", "cc_flatbuffers_toolchain")

cc_flatbuffers_toolchain(
    name = "my_custom_cc_toolchain",
    flatc_args = [ ... ],
)   

flatbuffers_library(
    name = "foo_fbs",
    srcs = ["foo.fbs"],
)

cc_flatbuffers_library(
    name = "foo_cc_fbs",
    toolchain = ":my_custom_cc_toolchain",
    deps = [":foo_fbs"],
)

I have already implemented it, but in order to allow a custom toolchain arg to be passed, I need to pass the toolchain to the aspect. Currently this does not appear to be possible.

The toolchain is implemented as a simple rule which returns a FlatbuffersToolchainInfo provider. I need to be able to pass a label with this provider to the cc code generation aspect.

I was almost able to get what I need with a private attr called _toolchain. But when it is defined this way, I don't think the user can pass their own custom toolchain.

The current implementation of the rule, is implemented here for reference:
https://github.com/kgreenek/rules_flatbuffers/blob/7ec99994d4b56a2eb0735871470298e26875c780/flatbuffers/cc_flatbuffers_library.bzl#L101

Feature requests: what underlying problem are you trying to solve with this feature?

Allow non-string attrs for aspects. Specifically, allow labels to be passed as attrs to an aspect.

What operating system are you running Bazel on?

Ubuntu 18.04 & 20.04

What's the output of bazel info release?

release 4.2.1

Have you found anything relevant by searching the web?

Somewhat-related issue: #11246

@benjaminp
Copy link
Collaborator

#8494

@kgreenek
Copy link
Author

kgreenek commented Sep 11, 2021

That is super helpful, thank you! It seems to me like maybe my mental model for how aspects are supposed to be used was a bit wrong.

One idea for my specific use-case: instead of enabling users to pass a toolchain to the cc_flatbuffers_library rule, I could instead require them to set up the toolchain in the WORKSPACE file. That would generate a BUILD file with a toolchain target defined that everything in the workspace would use.

kgreenek added a commit to kgreenek/rules_flatbuffers that referenced this issue Sep 11, 2021
Now there can only be one toolchain per language, and it must be defined
in the WORKSPACE.

For more context, see:
bazelbuild/bazel#13975
@kgreenek
Copy link
Author

I made that change, and now I think it is much cleaner:
https://github.com/kgreenek/rules_flatbuffers/tree/c81acb253637eb28cb4fc4b1dd5f1d9cc6a0ca29

I think it's also more in-line with other starlark rules.

I do think it would still be useful for aspects attrs to be a bit less restrictive, but I will close this issue since the other issue is already tracking it.

This issue was closed.
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

No branches or pull requests

2 participants