Migrate Closure to be located via npm install by juj · Pull Request #9989 · emscripten-core/emscripten

juj · 2019-12-08T13:18:05Z

Removes Closure from tree. Also remove test_sanity.test_closure_compiler, that is somewhat extraneous. The function check_closure_compiler is well enough, and is run whenever one enables closure with --closure 1.

hujiajie · 2019-12-09T02:14:46Z

Is there any plan to migrate other in-tree node modules (third_party/html-minifier, tools/eliminator, etc.) to npm install as well? If yes, does it mean python packages (e.g. third_party/ply) should be shipped via pip install similarly?

sbc100 · 2019-12-09T02:51:53Z

Is there any plan to migrate other in-tree node modules (third_party/html-minifier, tools/eliminator, etc.) to npm install as well? If yes, does it mean python packages (e.g. third_party/ply) should be shipped via pip install similarly?

In the long term yes, I think both these things would be good. One step at time though.

juj · 2019-12-09T07:04:21Z

Is there any plan to migrate other in-tree node modules (third_party/html-minifier

#9990 migrates html-minifier.

kripken · 2019-12-09T20:55:24Z

.circleci/config.yml

+            tar -xf node-v12.13.0-linux-x64.tar.xz
+            export PATH="`pwd`/node-v12.13.0-linux-x64/bin:${PATH}"
+            npm install
      - run:


I don't see a full explanation of the plan here, so I'm not sure who we expect to run npm install, exactly?

From this change to circleCI, it seems like this direction implies all users, even emsdk users, will need to install npm and run npm install? If so that worries me.

No, emsdk users will get this as a part of post-install.

Direct git users will need to run npm install, but we can prompt them to do this if they forget. We will of course need to document this clearly and mention both on the mailing list and in the ChangeLog. One nice thing about doing it with closure first is that it only effects our users who use closure, so it will be a good way to test the waters without effecting all users right away.

Another nice thing about making this a post-install step and not trying to bundle it is that the npm install can do all kind of platform specific things such as building native executables if needs be. With a bundled version we wouldn't have that option (unless we want commit bundle N different platform bundles)

I think we should talk about this suggestion to use npm in a more general way, as I still don't think I see the big picture. I see @juj posted a long comment in emscripten-core/emsdk#404 (comment) - maybe we should consolidate the discussion there?

As to that last point, though - why would native executables matter for closure or other JS things? Or is there some future use of native executables you envision?

Replying to this in emsdk/404

tools/settings_template.py

tools/shared.py

juj · 2019-12-10T18:59:25Z

Hmm, the PR is now in a state where it passes locally on my Windows, Linux and Mac computers. However on the CI it is still failing with saying node is not found when running closure compiler. I am not at all sure what is going on there, since node is added to PATH https://github.com/emscripten-core/emscripten/pull/9989/files#diff-1d37e48f9ceff6d8030570cd36286a61R96 , the next line installs closure compiler, so that PATH addition must be taking effect as well?

sbc100 · 2019-12-10T19:14:06Z

.circleci/config.yml

+            wget https://nodejs.org/dist/v12.13.0/node-v12.13.0-linux-x64.tar.xz
+            tar -xf node-v12.13.0-linux-x64.tar.xz
+            export PATH="`pwd`/node-v12.13.0-linux-x64/bin:${PATH}"
+            npm install


We already have node installed as part of emsdk above, right?

Hopefully we can do something like:

source emsdk_master/emsdk_env.sh NODE_DIR=$(dirname $EMSDK_NODE) export PATH=$NODE_DIR:$PATH

Hmm, not sure at all. Changed to that. I do see emsdk in https://github.com/emscripten-core/emscripten/blob/incoming/.circleci/config.yml#L23 , but that does not seem to help. Switched to using the above in 2d6fc43 but it is not locating emsdk there.

Sorry.. that should be "emsdk-master" with a hyphen

Lets see if we can land this part separately. We already run npm install ws in test-upstream-sockets-chrome.. we should probably just move that code here and remove the ws so it follows the package.json.

I tried to split this off here: #10254.. hope you don't mind me being proactive here.

Ok this part has landed now.

Revert this?

juj · 2020-01-09T18:37:07Z

Ping @kripken on this - after emscripten-core/emsdk#404 lands, this should pass the suite and be good to land as well.

kripken · 2020-01-09T18:40:01Z

Same thoughts as the other issue - if you and @sbc100 are confident in being able to handle fallout from landing these things now, then lgtm.

sbc100 · 2020-01-09T18:47:20Z

The good thing about landing the emsdk change first, and holding back the emscripten-side change, is that we can see if emsdk user report issues and revert if needed.

I suggest we wait a couple of weeks after the emsdk change lands before depending on it.

sbc100 · 2020-01-21T17:36:05Z

Before we land this can we first transition the existing checked-in content of node_modules at the top level? Currently I think it just hold ws. So I think it would just be questions of git rm -r node_modules. I'd rather not be in a situation where we have both npm and git managing files in that location.

sbc100 · 2020-01-21T17:36:22Z

I'm happy to propose that change if you like?

sbc100 · 2020-01-21T21:40:38Z

Sorry, ignore those last two comments. Looks like I was wrong and we have never had node_modules in git at the top level. Not sure why I believed that to be true..

tests/runner.py

tools/shared.py

sbc100 · 2020-01-21T21:52:58Z

tools/shared.py

+  except Exception as e:
+    warning('Closure compiler ("%s --version") did not execute properly!' % CLOSURE_COMPILER)
+    warning(str(e))
+  return False


I think maybe we should switch this over to only checking on demand, rather than on startup/sanity, but thats a separate change.

sbc100 · 2020-01-21T21:56:15Z

tools/shared.py


  if CLOSURE_COMPILER is None:
-    CLOSURE_COMPILER = path_from_root('third_party', 'closure-compiler', 'compiler.jar')
+    CLOSURE_COMPILER = path_from_root('node_modules', '.bin', 'google-closure-compiler' + ('.cmd' if WINDOWS else ''))


I feel like we should be able to use "npx google-closure-compiler" here to avoid the internal knowledge of the layout.

npx has many more moving pieces, such as install-on-demand if the package does not exist. I like this explicit behavior better. Since we are not dealing with randomly updating package, and Closure is quite stable, I don't think this is much of an issue.

OK, I agree that maybe npx is more than we want. But once this lands i would like to find a better solution. IIUC pocking directly into the .bin directory is not recommend as that is npm/node implementation detail.

I did a little research and foudn this: https://2ality.com/2016/01/locally-installed-npm-executables.html

Seems like maybe npm bin is the way to get access to that path.

sbc100 · 2020-01-21T21:57:13Z

tools/settings_template.py


-# CLOSURE_COMPILER = '..' # define this to not use the bundled version
+# To install Closure compiler, run 'npm install' in Emscripten root directory
+CLOSURE_COMPILER = '{{{ CLOSURE_COMPILER }}}'


Should we just remove this line completely from the config? Do we want to support non-standard locations/configurations for closure compiler? I'm tempted to say no.

I don't want to break existing users. No idea how that came to be, but I'll leave that for later, if someone wants to drop this. (no idea if someone might break if that is removed, but I think that's for separate topic)

What about leaving this alone then? Any reason not to leave it as a comment? Seems like this line could just be reverted.

emsdk will run npm install in the version of emscripten that it downloads, but we override with our own version from git and we need to run `npm install` locally. Previously we only did this for the socket tests which are currently the only parts of emscripten that actually depend on `npm install` having been run. However we have plans to extend that. See #9989.

juj · 2020-01-23T15:20:30Z

Rebased to master, let's see how the CI likes this now.

sbc100

lgtm with a couple of last comments.

Excited to see this happen!

package.json

sbc100 · 2020-01-23T19:13:46Z

tools/settings_template.py


-# CLOSURE_COMPILER = '..' # define this to not use the bundled version
+# To install Closure compiler, run 'npm install' in Emscripten root directory
+CLOSURE_COMPILER = '{{{ CLOSURE_COMPILER }}}'


What about leaving this alone then? Any reason not to leave it as a comment? Seems like this line could just be reverted.

sbc100 · 2020-01-23T19:16:49Z

tools/shared.py


  if CLOSURE_COMPILER is None:
-    CLOSURE_COMPILER = path_from_root('third_party', 'closure-compiler', 'compiler.jar')
+    CLOSURE_COMPILER = path_from_root('node_modules', '.bin', 'google-closure-compiler' + ('.cmd' if WINDOWS else ''))


OK, I agree that maybe npx is more than we want. But once this lands i would like to find a better solution. IIUC pocking directly into the .bin directory is not recommend as that is npm/node implementation detail.

I did a little research and foudn this: https://2ality.com/2016/01/locally-installed-npm-executables.html

Seems like maybe npm bin is the way to get access to that path.

sbc100 · 2020-01-23T19:17:06Z

.circleci/config.yml

+            wget https://nodejs.org/dist/v12.13.0/node-v12.13.0-linux-x64.tar.xz
+            tar -xf node-v12.13.0-linux-x64.tar.xz
+            export PATH="`pwd`/node-v12.13.0-linux-x64/bin:${PATH}"
+            npm install


Revert this?

sbc100 · 2020-01-23T19:18:35Z

tests/runner.py

+    if os.environ.get('NODE_PATH'):
+      os.environ['NODE_PATH'] = os.environ['NODE_PATH'] + os.pathsep + path_from_root('node_modules')
+    else:
+      os.environ['NODE_PATH'] = path_from_root('node_modules')


I just moved this code to test_sockets.py in #10266. My apologies for the churn, I probably should have waited until you landed this change.

kripken · 2020-01-27T17:11:49Z

This appears to have broken the chromium CI roll,

https://ci.chromium.org/p/emscripten-releases/builders/try/linux/b8890100283367924880

due to errors like

shared:WARNING: Closure compiler (['/b/s/w/ir/cache/builder/emscripten-releases/node-v12.9.1-linux-x64/bin/node', '--max_old_space_size=8192', '/b/s/w/ir/k/install/emscripten/node_modules/.bin/google-closure-compiler']) does not exist, check the paths in /b/s/w/ir/k/install/emscripten_config_upstream. To install Closure compiler, run "npm install" in Emscripten root directory.

It looks like closure was not installed or not installed successfully using the new method?

cc @juj @sbc100

A followup on #9989. The only two calls sites call exit_with_error if this function fails so just make all errors here fatal.

sbc100 · 2020-01-27T22:12:55Z

Fix ed in WebAssembly/waterfall#618

A followup on #9989. The only two calls sites call exit_with_error if this function fails so just make all errors here fatal.

This should have been part of #9989 but we somehow forgot to add it.

This workaround was added in emscripten-core#9989, I assume because one or more tests failed without.

This workaround was added in #9989, I assume because one or more tests failed without.

juj force-pushed the closure_via_npm_install branch from 0f3dbaf to 2a5155b Compare December 8, 2019 13:39

This was referenced Dec 8, 2019

Migrate Closure to be shipped via emsdk or manually installed via npm. #9986

Closed

Closure js #9962

Closed

html-minifier-terser #9990

Merged

kripken reviewed Dec 9, 2019

View reviewed changes

juj mentioned this pull request Dec 10, 2019

Add npm install step as post-build to Emscripten emscripten-core/emsdk#404

Merged

sbc100 reviewed Dec 10, 2019

View reviewed changes

tools/settings_template.py Outdated Show resolved Hide resolved

tools/shared.py Outdated Show resolved Hide resolved

juj force-pushed the closure_via_npm_install branch from fdc0346 to 337a554 Compare December 10, 2019 17:34

sbc100 reviewed Dec 10, 2019

View reviewed changes

juj force-pushed the closure_via_npm_install branch 2 times, most recently from 97d85f3 to 7bd783e Compare December 15, 2019 14:20

juj force-pushed the closure_via_npm_install branch from 7bd783e to fc9ebeb Compare January 10, 2020 20:13

juj changed the base branch from incoming to master January 14, 2020 18:57

juj force-pushed the closure_via_npm_install branch from fc9ebeb to b29140f Compare January 18, 2020 12:07

sbc100 reviewed Jan 21, 2020

View reviewed changes

tests/runner.py Outdated Show resolved Hide resolved

sbc100 reviewed Jan 21, 2020

View reviewed changes

sbc100 mentioned this pull request Jan 21, 2020

Perform npm install after setting up emsdk #10254

Merged

juj force-pushed the closure_via_npm_install branch from 180fffb to c59ad64 Compare January 23, 2020 15:20

sbc100 approved these changes Jan 23, 2020

View reviewed changes

juj force-pushed the closure_via_npm_install branch 2 times, most recently from 5b3585a to 7656639 Compare January 25, 2020 18:25

juj added 6 commits January 26, 2020 13:18

Migrate Closure to be located via npm install

295dd95

Add node to PATH for Closure

6dff12e

Remove littering source map file

cdeeb44

Update package-lock.json

42cd7e6

Add more heap space to Closure invocation

b0787d3

Work around Closure bug(?)

834d3b5

juj force-pushed the closure_via_npm_install branch from 0ccb42b to 04e921f Compare January 26, 2020 11:18

Skip asm3.test_files because it runs out of memory on CI

e7d1cde

juj force-pushed the closure_via_npm_install branch from 04e921f to e7d1cde Compare January 26, 2020 12:30

juj merged commit b5ec625 into emscripten-core:master Jan 26, 2020

sbc100 added a commit that referenced this pull request Jan 27, 2020

Make check_closure_compiler error fatal

5219daf

A followup on #9989. The only two calls sites call exit_with_error if this function fails so just make all errors here fatal.

sbc100 mentioned this pull request Jan 27, 2020

Make check_closure_compiler error fatal #10287

Merged

sbc100 added a commit that referenced this pull request Jan 27, 2020

Make check_closure_compiler error fatal (#10287)

84fb08e

A followup on #9989. The only two calls sites call exit_with_error if this function fails so just make all errors here fatal.

This was referenced Jan 29, 2020

Closure compiler breakage on windows emscripten-releases builder #10304

Closed

[WIP] Google closure compiler js #5464

Closed

sbc100 added a commit that referenced this pull request Feb 14, 2020

Document npm install post-checkout requirement in changelog

1e9f18d

This should have been part of #9989 but we somehow forgot to add it.

sbc100 mentioned this pull request Feb 14, 2020

Document npm install post-checkout requirement in changelog #10442

Merged

sbc100 added a commit that referenced this pull request Feb 14, 2020

Document npm install post-checkout requirement in changelog (#10442)

b17ec45

This should have been part of #9989 but we somehow forgot to add it.

sbc100 added a commit to sbc100/emscripten that referenced this pull request Sep 18, 2024

Remove a no-longer-needed closure workaround. NFC

c03d6df

This workaround was added in emscripten-core#9989, I assume because one or more tests failed without.

sbc100 mentioned this pull request Sep 18, 2024

Remove a no-longer-needed closure workaround. NFC #22584

Merged

sbc100 added a commit that referenced this pull request Sep 24, 2024

Remove a no-longer-needed closure workaround. NFC (#22584)

c104419

This workaround was added in #9989, I assume because one or more tests failed without.

Conversation

juj commented Dec 8, 2019

Uh oh!

hujiajie commented Dec 9, 2019

Uh oh!

sbc100 commented Dec 9, 2019

Uh oh!

juj commented Dec 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

juj commented Dec 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juj commented Jan 9, 2020

Uh oh!

kripken commented Jan 9, 2020

Uh oh!

sbc100 commented Jan 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbc100 commented Jan 21, 2020

Uh oh!

sbc100 commented Jan 21, 2020

Uh oh!

sbc100 commented Jan 21, 2020

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juj commented Jan 23, 2020

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbc100 commented Jan 9, 2020 •

edited

Loading