bazelbuild / rules_closure

Closure rules for Bazel
https://developers.google.com/closure
Apache License 2.0
152 stars 114 forks source link

Make closure_js_library to take deps in provider on top of target #604

Closed mollyibot closed 1 month ago

mollyibot commented 6 months ago

I take some efforts to refactor the code to take only provider, in that approach, we need to change for all providers and collect_runfiles and data , this kind of break the concept of https://github.com/bazelbuild/rules_closure/pull/599 that pass target everywhere. I think for now this small patch should help us to walk around the issue that closure_js_library only handles target(but j2cl pass provider).

gkdn commented 6 months ago

could you do

dep = dep[ClosureJsLibraryInfo] if type(dep) == "Target" else dep

try to keep rest of the code same.

mollyibot commented 6 months ago

could you do

dep = dep[ClosureJsLibraryInfo] if type(dep) == "Target" else dep

try to keep rest of the code same.

for other providers we need to keep target format, for example https://github.com/comius/rules_closure/blob/daf6131a64793e4f684fb9658ce7b6e5726896fb/closure/compiler/closure_js_library.bzl#L168-L182

deps = unfurl(deps, provider = ClosureJsLibraryInfo).exports

    # Collect all the transitive stuff the child rules have propagated. Bazel has
    # a special nested set data structure that makes this efficient.
    @@ -177,7 +179,7 @@ def _closure_js_library_impl(
    # which is a superset of the CSS libraries in its transitive closure.
    stylesheets = []
    for dep in deps:
        if ClosureCssLibraryInfo in dep:
            stylesheets.append(dep.label)

we can do this for closureJsProvider related though.

gkdn commented 6 months ago

could you do

dep = dep[ClosureJsLibraryInfo] if type(dep) == "Target" else dep

try to keep rest of the code same.

for other providers we need to keep target format, for example https://github.com/comius/rules_closure/blob/daf6131a64793e4f684fb9658ce7b6e5726896fb/closure/compiler/closure_js_library.bzl#L168-L182

deps = unfurl(deps, provider = ClosureJsLibraryInfo).exports

    # Collect all the transitive stuff the child rules have propagated. Bazel has
    # a special nested set data structure that makes this efficient.
  @@ -177,7 +179,7 @@ def _closure_js_library_impl(
    # which is a superset of the CSS libraries in its transitive closure.
    stylesheets = []
    for dep in deps:
        if ClosureCssLibraryInfo in dep:
            stylesheets.append(dep.label)

we can do this for closureJsProvider related though.

Can't we do unfurl them in the same way:

    cssDeps = unfurl(deps, provider = ClosureCssLibraryInfo)
    for cssDep in cssDeps:....
mollyibot commented 6 months ago

could you do

dep = dep[ClosureJsLibraryInfo] if type(dep) == "Target" else dep

try to keep rest of the code same.

for other providers we need to keep target format, for example https://github.com/comius/rules_closure/blob/daf6131a64793e4f684fb9658ce7b6e5726896fb/closure/compiler/closure_js_library.bzl#L168-L182

deps = unfurl(deps, provider = ClosureJsLibraryInfo).exports

    # Collect all the transitive stuff the child rules have propagated. Bazel has
    # a special nested set data structure that makes this efficient.
    @@ -177,7 +179,7 @@ def _closure_js_library_impl(
    # which is a superset of the CSS libraries in its transitive closure.
    stylesheets = []
    for dep in deps:
        if ClosureCssLibraryInfo in dep:
            stylesheets.append(dep.label)

we can do this for closureJsProvider related though.

Can't we do unfurl them in the same way:

    cssDeps = unfurl(deps, provider = ClosureCssLibraryInfo)
    for cssDep in cssDeps:....

the cssDep also need to be a target here(plz seecssDep.label ). also there are other providers like WebFilesInfo

deps = unfurl(ctx.attr.deps, provider = WebFilesInfo).exports
for f in dep.files.to_list():
            inputs.append(f)
gkdn commented 6 months ago

The unfurl is overloaded with multiple purposes and that makes a lot of things obscure. I did a refactoring around that, will send for review but first I sent you a cleanup for runfiles handling.