Emscripten: Replace Closure compiler with its JS version to remove Java dependency

[saschanaz]

Google just published closure-compiler-js which is available on NPM. Can emscripten get some benefit with it?

[kripken]

Good idea!

[juj]

Oh, yes please! I'd love to stop bundling an embedded Java runtime in emsdk for people to access closure. This would be a great help there. If someone is interested in contributing to this, would be much appreciated.

[citisolo]

Hi , I'm new here but I would like to help , where would I start ?

[kripken]

Great, hi @citisolo!

To get started, I would look at the code that calls closure compiler, a method called closure_compiler in tools/shared.py. That runs compile compiler in java currently. After adding the JS version of closure to the repo (probably at third_party/closure-compiler, which is where the java version currently is), you can replace the call to java with a call using run_js to run the JS version.

Then I would look for where closure compiler is used in the tests (tests/test*.py) and verify they pass after the change.

[citisolo]

Hi @kripken ,
Thanks for pointing me in the right direction . I can get started on that tommorow (in london it's 23:51). Cheers

[citisolo]

I replaced the java version with the js version by adding the package with a driver that is called through run_js but there the following issues I'm finding challenging at the moment ;

1. The package requires installation of node-extern and node-modules directories which are ignored by the repo , so will need to be automatically installed
2. default.test_files is failing due to node running out of memory.
3. other.test_emcc is failing with

"AssertionError: closure minifies the shell, -g1 makes it keep whitespace"

, the main problem is that the js-closure-compiler does not seem to be generating the same code as the java compiler , even with the same flags set, and externs included.

/cc @kripken @SaschaNaz

[saschanaz]

It seems that there is no --formatting parameter support on JS version yet, which is used by -g1.

[kripken]

Thanks @citisolo! Comments on those issues:

1. I actually don't know much about node modules. Is it possible to bundle those somehow? Or is automatic installation what people normally do?
2. The out of memory issue sounds worrying. It makes sense the JS version would use more memory but test_files isn't actually that big... so maybe the JS version uses a lot more memory, or maybe we are hitting a bug in closure-JS or in node.js? If we can't figure this out, maybe it's too risky to use the JS version.
3. Thanks, @SaschaNaz. So, if the JS version doesn't have --formatting, I think that's a problem as it's very useful for debugging closure issues (without whitespace, things are much harder). Perhaps we could find out if they intend to support this soon? If not, then this might make using the JS version too risky.

[citisolo]

@kripken I agree , I believe it's still early days for the js-compiler . Perhaps we should keep this open for sometime and revisit it when all the kinks are sorted out?
/cc @SaschaNaz

[kripken]

Yeah, that sounds good. Thanks again @citisolo for looking into this, and hopefully when the js version is finished we can use it.

[chadaustin]

Curious: is there a build time hit to using closure-compiler-js? Or is its performance comparable to the Java version? At IMVU, Closure was a big part of the build time.

[kripken]

It was significantly slower than the Java version last I checked. We can recheck though, a lot of time passed. But it may matter less now anyhow, as you are probably thinking of using closure on all the code - since you say it was a big part of build time - and we now only use it on the normal JS code, not compiled code.

[chadaustin]

Oh I see. Sounds great then. :)

[stale[bot]]

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 7 days. Feel free to re-open at any time if this issue is still relevant.