Skip to content

Conversation

@mollyibot
Copy link
Collaborator

@mollyibot mollyibot commented Mar 6, 2024

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 #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).

@mollyibot mollyibot marked this pull request as ready for review March 6, 2024 19:13
@gkdn
Copy link
Collaborator

gkdn commented Mar 6, 2024

could you do

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

try to keep rest of the code same.

@mollyibot
Copy link
Collaborator Author

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
Copy link
Collaborator

gkdn commented Mar 6, 2024

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:....

if type(dep) == "Target":
if not provider or provider in dep:
res.append(dep)
if type(provider) == "Provider" and provider in dep and hasattr(dep[provider], "exports"):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we ever pass non provider as a provider here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, the default parameter is "", the function signature is

def unfurl(deps, provider = ""):

sometimes there is only one parameter without provider.

@mollyibot
Copy link
Collaborator Author

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
Copy link
Collaborator

gkdn commented Mar 7, 2024

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.

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

Successfully merging this pull request may close these issues.

2 participants