google/elemental2

Build failure: `Executing genrule //java/elemental2/dom:w3c_event_patched failed`

niloc132 opened this issue · 11 comments

This commit seems to have broken the build: d15d513

It assumes that google/closure-compiler#3542 has already merged into google/closure-compiler, but this does not appear to be the case. As such, building elemental2 currently fails, since this diff does not apply cleanly.

gkdn commented

Elemental2 automatically syncs in earlier than JsCompiler so this is expected at the head version when a change is submitted.

Is there a rough ETA on the cycle completing? Looks like the lag time (of broken elemental2 and all downstreams) is 16+ hours.

I think closure guys are syncing every day or two days.

Just a heads up, this is still broken even though closure has merged that patch.

This is because w3c_event comes from

extern(
    name = "w3c_event",
    path = "browser/w3c_event.js",
)

and extern(...) uses the com_google_javascript_closure_compiler repository, which is not from git (that would be com_google_closure_compiler, defined here to read from github directly), but from rules_closure which specifies an old release.

This mismatch was discussed in #135 - my understanding was that the conclusion from that was that we should stop using the "master" build, but only use the latest release. Likewise as discussed there, the extern() def does not read from the "master" branch, so I think it was premature to merge this. Would you consider reverting it so we can build this project (and downstream projects) again?

Alternatively, replace the extern(...) invocation above with an alias(...), like in #135?

My last comment were wrong for most of the extern files, I'm sorry.
We use extern files packaged in jscompiler binary (to be sure the api are sync between the externs used by the jscompiler and the interfaces defined in elemental2). So we need to wait that a new binary containing the changes in the externs is released. Then we update rules_closure repo to use this new release and that will fix the build.

We need to switch on a monthly release so people does not need to depend on master.

There is a PR open that will resolve this until the next release is shipped: #138

As elemental2's build instructions direct users to use master (as does the tagged release, even though the tagged build appears to use a tagged version of closure-compiler, which may resolve this), it might make sense to fix this issue - any project following the readme instructions cannot build and hasn't been able to do so for just over a week.

gkdn commented

To clarify;
Internally we use everything from HEAD version so any update goes directly here; I don't know anyway to delay that.
Externally we need to use the externs from the JsCompiler version used by rules_closure.

So there is no way to make head version work always; as things moving and we are looking at two different places plus the patch files.

@jDramaix is that something we could speed that up?

We could think about doing a monthly release (in sync with closure compiler release) so people can use this one instead of depending on master and update the build instructions.
Master will always be instable externally.
Or we could push changes to a branch instead of master and merge to master once this temporary branch is stable.

gkdn commented

Do you expect instability after everything is moved under jscompiler and patches are gone?

The only way to remove patches is to write specific visitors. There is no way to fix everything in the extern file directly. EventListener is an example of api that would need post process to fix the generated java code.
Even if we get rid of the patches, we can have instability if they rename/move extern files (they did that recently and will do it again) and with integer entity files.

Note that we don't have instability with integer entity files as the jsinterop generator runs in non-strict mode in opensource and it emits a warning instead of an error.

So I guess if we can rid of the patches, master branch of elemental should stay stable. Closure guys will rename so extern files in the near future but then it should not happen anymore