Replace legacy struct providers with modern ones

[comius]

Legacy struct providers have been deprecated by Bazel. Replacing them with modern providers, will make it possible to simplify and remove legacy handling from Bazel

[gkdn]

Since we are introducing proper providers, would it be possible to get rid of this as well?

[comius]

For ClosureExportsInfo, it looks possible to remove it, by adding exports to ClosuresJsLibraryInfo and WebFilesInfo.

ClosureJsLegacyRunfilesInfo looks easier to remove, by using regular runfiles instead.

Both of those removals require some effort and should probably be done by code owners and not as a part of a large cleanup, where we're fixing about 80 other rulesets.

[gkdn]

Ok that's fair.

[gkdn]

@mollyibot Could you do a quick/hacky prototype of required changes to build_defs/internal_do_not_use/j2cl_js_common.bzl to validate this.

[mollyibot]

I did repro build for j2cl/ j2wasm, One minor thing : some deps may still in deprecated struct which means the in operator or index notation would not always work.

[comius]

I did repro build for j2cl/ j2wasm, One minor thing : some deps may still in deprecated struct which means the in operator or index notation would not always work.

We have internal fixes already for that file. They seem independent of rules_closure.

[gkdn]

I did repro build for j2cl/ j2wasm, One minor thing : some deps may still in deprecated struct which means the in operator or index notation would not always work.

We have internal fixes already for that file. They seem independent of rules_closure.

To double check, you are referring to fixes in Bazel, right? That means if we submit the change we cannot use it from older versions?

Edit: more particularly J2CL that is using 5.4.0?

[comius]

We have internal fixes already for that file. They seem independent of rules_closure.

To double check, you are referring to fixes in Bazel, right? That means if we submit the change we cannot use it from older versions?

Edit: more particularly J2CL that is using 5.4.0?

No, not Bazel. The support for new style providers is in Bazel for ages, I believe even before Bazel 4.

I was referring to fixes of j2cl_js_common.bzl, they are done internally.

[mollyibot]

Yes, I agree j2cl_js_common.bzl can be modified to handle special cases too. why not handle special dep cases in rules_closure as well if some the in operator or index notation usages exist in closure/compiler/closure_js_library.bzl and closure/private/defs.bzl?

[mollyibot]

I send a pr for review @comius . The issue is how we handle the deps in rules_closure. not all deps are rule targets and in may not work to access providers . This is snippet shows the deps we passed in to create_closure_js_library
In g3, we do

deps = [d[JsInfo] if type(d) == "Target" else d for d in deps]  

to pass into js_checkable_provider that takes in deps in provider
in this pull request, I see deps is handled as target (for Backward compatibility?)
Feel free to let us know how you think.

[gkdn]

@mollyibot What is the source of non-target deps here?

[mollyibot]

one example srcs :
the srcs are[<generated file external/com_google_j2cl/jre/java/jre.js>]
and deps struct(descriptors = depset([]), has_closure_library = True, ijs = <generated file external/com_google_j2cl/jre/java/javaemul_internal_annotations-j2cl.i.js>, ijs_files = depset([<generated file external/io_bazel_rules_closure/closure/private/base_lib.i.js>, <generated file external/com_google_j2cl/jre/java/javaemul_internal_annotations-j2cl.i.js>]), info = <generated file external/com_google_j2cl/jre/java/javaemul_internal_annotations-j2cl.pbtxt>, infos = depset([<generated file external/io_bazel_rules_closure/closure/private/base_lib.pbtxt>, <generated file external/com_google_j2cl/jre/java/javaemul_internal_annotations-j2cl.pbtxt>]), js_module_roots = depset(["external/com_google_j2cl_samples_helloworld"]), modules = depset(["external/com_google_javascript_closure_library/closure/goog/base"])

[gkdn]

But that's because we haven't migrated J2CL, right? i.e. j2cl is still producing the struct

[mollyibot]

yes, my concern is will this happen for other callers that are not from j2cl, should rules_closure also handle those scenarios?

[gkdn]

But you mentioned that you needed to workaround a problem on J2CL side, I thought it was something else. Could you share me again the changes needed in J2CL?

for other callers that are not from j2cl

We gonna need to be move forward at one point. Maybe we can temporary support both but one can just wait before upgrading since we don't have other important improvement. Anyway we can discuss if that becomes the only problem (your PR is small so it might be a good tradeoff)

[mollyibot]

I had a messy workaround to change the some of the deps (of providers) that we passed in https://github.com/google/j2cl/blob/master/build_defs/internal_do_not_use/j2cl_java_library.bzl#L100-L104 to deps, and do checks for g3 extract providers for js info as before and opensource not extract providers, and for jvm extract java_info everywhere . This workaround has two versions for j2cl_common and j2cl_java_library for g3 and opensource, that is why I prefer to doing the cleaning in rules_closure.

[gkdn]

IIUC your concern is following steps:

1. rules_closure will be changed with this PR.
2. We need to update J2CL to work with both old version and new version (legacy struct and provider) - i.e. the messy workaround
3. J2CL open-source will be updated to new rules_closure version
4. J2CL will be updated again to remove the workaround

And you are proposing:

1. rules_closure changed to support both versions - legacy struct and provider (this PR + your PR)
2. J2CL is updated to work with providers
3. rules_closure support for legacy struct is dropped

And the final state in both approaches will be the same.

Is that accurate or do you have other concerns?

[mollyibot]

yes, apart from i do not plan to drop the legacy struct support, I plan to do 1 and 2 because i am afraid there would be other caller pass in provider(in a struct) rather than target. if we compare g3 version [js_checkable_provider] they support provider in their deps.

I just uploaded a draft to demonstrate my idea to work around the issue https://github.com/google/j2cl/pull/227/files. i am afraid we need to have two versions(g3 and opensource) for these bzl files.

[gkdn]

There shouldn't be any struct passing scenario; we are under full control and that would be the point of the migration. That also shouldn't happen under js_checkable_provider either. I think something might be wrong in this picture if that's not the case.

[mollyibot]

okay, I do not have issues with your plan then.

[gkdn]

Ok plan wise;
let's do a release of rules_closure and point out that this will be the last version to support existing legacy style. I can do this, when I get back from vacation.

Then we can submit this PR and do follow up changes to do further refactoring to get rid of the extra providers discussed in #599 (comment)

Then prepare the J2CL PR that only supports the latest version of rules_closure. If there is any surprising impact; we can do more updates to rules_closure and finalize the next steps.

Does it sound good?

[mollyibot]

Yes, sounds good! Let me know if i can help with the release.