search

bazelbuild / rules_closure

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

Replace legacy struct providers with modern ones #599

Closed comius closed 6 months ago

comius commented 7 months ago

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 commented 7 months ago

@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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 6 months ago

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 commented 6 months ago

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

mollyibot commented 6 months ago

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 commented 6 months ago

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

mollyibot commented 6 months ago

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

gkdn commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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

gkdn commented 6 months ago

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 https://github.com/bazelbuild/rules_closure/pull/599#discussion_r1456810215

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 commented 6 months ago

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