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

RFC: Add "modern" version of closure_template_library #389

Open
Yannic opened this issue Jun 7, 2019 · 3 comments
Open

RFC: Add "modern" version of closure_template_library #389

Yannic opened this issue Jun 7, 2019 · 3 comments

Comments

@Yannic
Copy link
Contributor

Yannic commented Jun 7, 2019

Current situation

closure_template_{java,js}_library has been part of rules_closure for some time.
Since its addition in 2016(?), a lot has changed, both in rules_closure and Bazel, and I think it's time to create something new. Something that looks and feels more like other "modern" Bazel-rules.

The current versions of closure_template_{java,js}_library are implemented as thin (independent) wrappers around genrule. This means that adding dependencies to or removing them from templates requires changing multiple targets, which is rather error-prone. Also, using closure templates from a new language (e.g. from Java if you only used them in JS) requires adding targets for all transitive dependencies.

closure_template_java_library(
    name = "a_java_soy",
    srcs = [
        "a.soy",
    ],
)

closure_template_js_library(
    name = "a_js_soy",
    srcs = [
        "a.soy",
    ],
)

closure_template_java_library(
    name = "b_java_soy",
    srcs = [
        "b.soy",
    ],
    deps = [
        ":a_java_soy",
    ],
)

closure_template_js_library(
    name = "b_js_soy",
    srcs = [
        "b.soy",
    ],
    deps = [
        ":a_js_soy",
    ],
)

New rule

The style of the new rules is inspired by proto_library. Instead of having per-language targets, we will create a dedicated closure_template_library rule that defines the dependencies between individual templates, and multiple closure_template_{language}_library-rules that generate language bindings for the templates.

closure_template_library(
    name = "a_soy",
    srcs = [
        "a.soy",
    ],
)

closure_template_library(
    name = "a_soy",
    srcs = [
        "b.soy",
    ],
    deps = [
        ":a_soy",
    ],
)

closure_template_java_library(
    name = "b_java_soy",
    deps = [
        ":b_soy",
    ],
)

closure_template_js_library(
    name = "b_js_soy",
    deps = [
        ":b_soy",
    ],
)

Internally, the language rules will use aspects to generate the language bindings for transitive dependencies.

Other design goals are:

  1. figure out how integration with closure_proto_library should work (e.g. closure_proto_library: propagate descriptors to create_closure_js_library (like #277) #314, Issues building protos, and using them #388), and
  2. integrate the template generation into ClosureWorker (it currently isn't).
@sgammon
Copy link
Contributor

sgammon commented Jun 7, 2019

this would be really awesome in terms of maintainability, at least the way we structure templates when using rules_closure. obviously i'm only commenting on the API surface here and not what is needed to land this feature, but this is really cool and we'd be happy to test, or help with PRs.

@gkdn
Copy link
Collaborator

gkdn commented Jun 8, 2019

Is there any particular reason that you want to use aspects here? They are complicated and only useful if you like to overlay on top of other rules that you don't have control on (for example closure_js_proto overlaying on top of existing proto rules).

I haven't dealt with closure templates but for this particular case, all you you need seems like a macro:

def closure_template_library(name, deps, ...):
js_deps = [ d + "_js" for d in deps]
closure_template_js_library(name + "_js", js_deps, ...)
java_deps = [ d + "_java" for d in deps]
closure_template_java_library(name + "_java", java_deps, ...)

Then your particular target java or js target depends on foo_java or foo_js.

@Yannic
Copy link
Contributor Author

Yannic commented Jun 13, 2019

The reason why I thought of aspects to implement this is so users don't have to pay the overhead for languages they don't use during analysis, and that it might be slightly easier to add new languages (there is an official generator for python that rules_closure doesn't support). That said, I'm open to implementing this without aspects if they turn out to not be worth the extra effort.

The main reason why I'm proposing this new rule is to fix the limitations of the current genrule-based implementation. They have some issues when crossing workspace-boundaries, (especially the java version) have rather strong opinions on the layout of the workspace, and, in the case of Java, only support the "old" Tofu style.

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

3 participants