Closure js

[juj]

This adds JS version of Closure compiler for users who don't want to ship Java as part of their SDKs.

First I added google-closure-compiler in the tree directly, but after doing that, I realize that there are many dependencies to the closure compiler npm package and so many files to bundle (490) that would make it a bit awkward to put in the repository.

So I settled for a scheme that if one enables the JS closure compiler, it is npm installed on demand. I am not sure if there would exist a minified version of closure-cli that we could easily embed, asked for that in google/closure-compiler#3515.

[juj]

Hmm, oddly npm install on demand fails on the CI.. does it not install node and npm?

shared:CRITICAL: /root/emsdk-master/node/12.9.1_64bit/bin/npm
/usr/bin/env: ‘node’: No such file or directory
Traceback (most recent call last):
  File "/root/project/emcc.py", line 3704, in <module>
    sys.exit(run(sys.argv))
  File "/root/project/emcc.py", line 2512, in run
    optimizer)
  File "/root/project/emcc.py", line 3099, in do_binaryen
    final = run_closure_compiler(final)
  File "/root/project/emcc.py", line 3094, in run_closure_compiler
    extra_closure_args=options.closure_args)
  File "/root/project/tools/shared.py", line 2437, in closure_compiler
    if Settings.USE_JS_CLOSURE_COMPILER and not check_javascript_closure_compiler():
  File "/root/project/tools/shared.py", line 545, in check_javascript_closure_compiler
    run_process([npm, 'install', 'google-closure-compiler@20191111.0.0'], cwd=path_from_root('.'))
  File "/root/project/tools/shared.py", line 177, in run_process
    ret = subprocess.run(cmd, check=check, input=input, *args, **kw)
  File "/usr/lib/python3.6/subprocess.py", line 438, in run
    output=stdout, stderr=stderr)
subprocess.CalledProcessError: Command '['/root/emsdk-master/node/12.9.1_64bit/bin/npm', 'install', 'google-closure-compiler@20191111.0.0']' returned non-zero exit status 127.

[sbc100]

This looks good! It will be great to get rid of our java dependency!

A couple of comments:

1. Rather than try to support both the java and the JS version can we just switch wholesale to the JS version? Are there any advantages to containing to support the java version?

2. Rather than installing on-demand during the compile, why not add it to our package.json file and then make running "npm install" a prerequisite? emsdk users wouldn't need to do it because we could run npm install before shipping out the SDK. git users like us would see a message like "closure compiler not found (did you forget to run npm install)?".

I general I think we should be moving away from on-demand installation, download and compilation if possible.

[juj]

1. Rather than try to support both the java and the JS version can we just switch wholesale to the JS version?   Are there any advantages to containing to support the java version?

The JavaScript version is unfortunately slower - how much slower I haven't actually profiled, but it is likely to be noticeable. So in a video call earlier with Alon the idea was to pursue this kind of dual approach instead.

2\. why not add it to our package.json file and then make running "npm install" a prerequisite?

That would imply that all users prefer the JS version instead of the Java version, although I can certainly place it there.

I general I think we should be moving away from on-demand installation, download and compilation if possible.

Agree, I dislike the approach I had to go with here. The only reason for this was that checking in a large node_modules directory was unwieldy. Ideal solution would be a minified+amalgamated closure-compiler-js-cli.min.js file (google/closure-compiler#3515), but I don't know enough of node/npm world if that is easy to make from the npm repository, or if it requires a custom webpack/rollup/minification solution in place.

[kripken]

It should definitely be possible to build the entire project to a single minified js file, and I think that's the best thing to check in, parallel to the java binary we have today. Does npm build not do that? The output is likely in a subdir like bin/ or node_modules/bin/.

If we need help with this hopefully there are npm experts reading this already, if not we can cc someone.

[sbc100]

@kripken, is there some reason why your prefer to check-in the dependencies into source control? What do you think of the idea of doing npm install (for sdk users this would be part of building the emsdk)?

I think @mathiasbynens was helping is move away from checked-in JS deps in the past but we didn't yet succeed. Maybe he has some advice here?

[juj]

Does npm build not do that?

Tried this out, was not able to get anywhere. I think npm build can be used to build your project to a minified form, but it doesn't grab whatever packages you have automatically into it (a project using closure should not bundle closure as minified into its build..). Trying npm build in google-closure-compiler node_modules directory also gives just an error. So not sure if that kind of minification would exist out of the box.

[kripken]

@sbc100 I think the core issue is that these files almost never change, and they are portable. So it's simpler to just check in a "binary" than to set up a build environment (both on CI and locally; instead, just make the portable binary locally by 1 dev, every few years).

cc @dcodeIO (who I see mentioned on the closure compiler page 😄) about building closure compiler js to a single .js file, maybe we are missing something here...

[dcodeIO]

Looks like the google-closure-compiler npm package includes not only the JS variant, but also the respective Java version and a Windows exe, then picking the fastest one supported by default.

platform = getFirstSupportedPlatform(['native', 'java', 'javascript']);

Suggesting to remove the old Java one and replace it with the npm package incl. dependencies, building it like

mkdir closure-compiler
cd closure-compiler
npm install --force google-closure-compiler

and then zipping the contents of the closure-compiler directory and ship that. There'll be a node_modules/.bin/google-closure-compiler bash file and a node_modules/.bin/google-closure-compiler.bat batch file inside then. Should "just work" considering that node is also being installed by Emsdk.

(Didn't test)

[juj]

Migrating closure to be supplied by emsdk would be fine for me, though that would be something Google people would have to help with, as I cannot upload artifacts to emsdk repository.

[dcodeIO]

Gave this a proper test-run now and it turned out that the native compilers becoming installed depend on the platform installing, so I updated the little build snippet above accordingly. Now forces installation of all the native ones so these exist if the platform building and the platform executing differ. Size of the directory then is 125mb (each native compiler is about 35mb and the Java one is 10mb), all zipped about 45mb.

Also gave building a JS-only one a try but then figured that we'd have to ship the externs it uses to describe various web and node APIs in addition to the bundle anyway. Seems that packaging all of this up is a better approach since it reduces the risk of breakage compared to rolling a custom solution.

[sbc100]

I don't understand. What does "packing all this up" mean? Wouldn't that be the definition of a custom solution? The standard solution seems to be "npm install", no?

I guess I agree with the "packing all this up", I just think that should be done as part of emsdk, not the "git clone" of emscripten.

[dcodeIO]

What I meant there is to ship closure compiler via Emsdk, similar to how node is shipped. What the build snippet above does is to make the npm install that the user would otherwise do, so we can zip it and ship it. Another approach would be to implement something into Emsdk that, after node has been installed, does the npm install directly instead of downloading a premade zip, just wasn't sure how feasible this is since Emsdk currently does only archives. The latter might as well be part of Emscripten since all there is to it is shipping a package.json indicating the closure compiler version and executing npm install (node must be installed for this to work).

[sbc100]

Oh I like that idea. Having a post install step in emsdk that does npm install. I think i'll try that approach. I like the better then zipping up node_modules because node_modules can have arch-specific stuff in to in theory and itsn't designed to be shared between machines (IIUC).

[juj]

Posted PRs emscripten-core/emsdk#403 and #9986 that migrate Closure to be shipped via emsdk. (or if not using that, one can do a npm install and invoke the tool via PATH lookup)

If we decide to go that route, that pair of PRs will obsolete this PR.

[juj]

Superceded by #9989.