Use non-symlinked version of google-closure-compiler #16640

kjlubick · 2022-03-31T13:21:34Z

When trying to compile using emscripten/emsdk on Bazel's remote executors, I ran into an error like:

Linking ... failed: (Exit 1): emcc_link.sh failed: error executing command external/emsdk/emscripten_toolchain/emcc_link.sh @bazel-out/wasm-opt-ST-d9bb9c985c3e/bin/...-2.params
node:internal/modules/cjs/loader:936
  throw err;
  ^

Error: Cannot find module './lib/utils'
Require stack:
- /b/f/w/external/emscripten_bin_linux/emscripten/node_modules/.bin/google-closure-compiler
    at Function.Module._resolveFilename (node:internal/modules/cjs/loader:933:15)
    at Function.Module._load (node:internal/modules/cjs/loader:778:27)
    at Module.require (node:internal/modules/cjs/loader:1005:19)
    at require (node:internal/modules/cjs/helpers:94:18)
    at Object.<anonymous> (/b/f/w/external/emscripten_bin_linux/emscripten/node_modules/.bin/google-closure-compiler:20:57)
    at Module._compile (node:internal/modules/cjs/loader:1101:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1153:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:79:12) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/b/f/w/external/emscripten_bin_linux/emscripten/node_modules/.bin/google-closure-compiler'
  ]
}
node:internal/modules/cjs/loader:936
  throw err;
  ^

By looking at the remote execution logs (e.g. appending --execution_log_json_file=/tmp/execlog.json to my Bazel command), I was able to figure out that Bazel was not preserving symlinks that were created by npm install, but rather uploading the linked file in place of the symlink.

Instead of uploading

.../node_modules/.bin/google-closure-compiler # symlink to ../google-closure-compiler/cli.js
.../node_modules/google-closure-compiler/cli.js
.../node_modules/google-closure-compiler/lib/utils.js

it was uploading

.../node_modules/.bin/google-closure-compiler # copy of ../google-closure-compiler/cli.js
.../node_modules/google-closure-compiler/cli.js
.../node_modules/google-closure-compiler/lib/utils.js

Then, when executing node_modules/.bin/google-closure-compiler, it would try to load node_modules/.bin/lib/utils.js and not be able to find it.

To work around this issue, we can bypass the symlink and run the google-closure-compiler/cli.js ourselves, which is successfully able to load the lib/utils.js and all other dependent files. This shouldn't impact non-Bazel clients.

emscripten-core/emsdk#883 is possibly a related issue

kripken · 2022-04-04T19:40:37Z

I don't know enough about the bazel support - is this a bug there?

cc @walkingeyerobot

kjlubick · 2022-04-04T19:45:01Z

I don't think it's an issue with the emsdk Bazel rules, but I could be wrong. I think it's just how Bazel implements remote execution and symlink resolution

walkingeyerobot · 2022-04-04T22:25:45Z

cc @trybka

walkingeyerobot · 2022-04-06T18:14:35Z

This looks good to me. @kripken can you please also approve since this isn't specifically a bazel change?

tools/shared.py

kripken

General question: Why is this an issue for closure compiler but not any other node tool that we use?

kripken · 2022-04-07T22:14:31Z

tools/building.py

+    # may not work correctly in all environments (e.g. Bazel remote execution). Instead,
+    # we go to what that symlink would be pointing to directly.
+    cmd = shared.get_npm_cmd(os.path.join('node_modules', 'google-closure-compiler', 'cli.js'),
+                             relative_to_node_bin=False)


I'm still not sure I follow this. relative_no_node_bin is now false, so I'd expect an absolute path. But the path is node_modules/.../...js, which is relative to something. But relative to what?

This path is now relative to the emscripten root, which is where node_modules is created. Bazel doesn't work well with absolute paths, because in order for things to be hermetic, they need to be contained in the Bazel cache (also known as the external folder)

I see, thanks. Sorry for all the questions, I'm not very familiar with this area.

kjlubick · 2022-04-08T11:20:26Z

Why is this an issue for closure compiler but not any other node tool that we use?
It is likely a lurking issue for other tools that will be used in RBE. I have only run into this for closure, so this only fixes that.

kripken

Ideally this would wait for @sbc100 to be available as I don't think anyone else really knows the npm install process in the emsdk that well. What worries me is that in this repo we don't test that - those tests are in the emsdk repo. So any issue would only be noticed the next time we prepare to do a release.

That wouldn't be the end of the world, though. Is this urgent to land? If so we probably can.

tools/shared.py

kjlubick · 2022-04-08T17:06:52Z

We can wait for @sbc100. It's not a huge rush.

sbc100 · 2022-04-12T20:23:41Z

This seems a little too invasive to me. Ideally we could find a way to do this without touch emscripten.. since this is more of a bazel bug IMHO.

If we have to do something here in emscripten I think maybe we can do something smaller. Perhaps a mapping withing get_npm_cmd that means its external API doesn't need to change?

kjlubick · 2022-04-13T12:11:12Z

I looked through the Bazel CLI and took a few shots in the dark: --noremote_allow_symlink_upload and --noremote_allow_symlink_upload. Neither seemed to fix the issue.

If this is a Bazel bug (and not WAI), I'm not sure where to even start trying to find and fix that bug.

Another option might be to let the emsdk Bazel rules set config.CLOSURE_COMPILER, right? How would that look?

sbc100 · 2022-04-13T16:46:13Z

Yes setting CLOSURE_COMPILER could be an option.

sbc100 · 2022-04-13T16:47:33Z

BTW I don't really think its a bazel bug, but its a bazel issue that probably warrents a workaound in the emsdk bazel toolchain rather than upstream in emscripten (if possible).

kjlubick · 2022-04-13T17:03:25Z

@walkingeyerobot Do you have any suggestions for how to set that in the emsdk Bazel rules?

walkingeyerobot · 2022-04-14T19:29:23Z

I'm not entirely sure, but I feel like there must be a way. @trybka might have some ideas. I think the closure compiler is used by bazel with js rules; it might be worth investigating if they've run into this issue.

sbc100 · 2022-04-14T19:47:02Z

I think we are talking about when emcc runs closure compiler (via the --closure=1 argument).

I think the solution would be to somehow patch the .emscripten config file used by bazel to override CLOSURE_COMPILER.

kjlubick · 2022-04-18T14:56:23Z

This PR is obsoleted by emscripten-core/emsdk#1037

kjlubick added 2 commits March 31, 2022 09:09

Use non-symlinked version of google-closure-compiler

151da28

Try different approach for windows

50c1cdb

walkingeyerobot approved these changes Apr 6, 2022

View reviewed changes

walkingeyerobot requested a review from kripken April 6, 2022 18:13

kripken reviewed Apr 7, 2022

View reviewed changes

tools/shared.py Outdated Show resolved Hide resolved

invert flag

eb0c4cc

kripken reviewed Apr 7, 2022

View reviewed changes

kripken reviewed Apr 8, 2022

View reviewed changes

tools/shared.py Show resolved Hide resolved

more clarification

7b66629

kjlubick closed this Apr 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use non-symlinked version of google-closure-compiler #16640

Use non-symlinked version of google-closure-compiler #16640

kjlubick commented Mar 31, 2022

kripken commented Apr 4, 2022

kjlubick commented Apr 4, 2022

walkingeyerobot commented Apr 4, 2022

walkingeyerobot commented Apr 6, 2022

kripken left a comment

kripken Apr 7, 2022

kjlubick Apr 8, 2022 •

edited

Loading

kripken Apr 8, 2022

kjlubick commented Apr 8, 2022

kripken left a comment

kjlubick commented Apr 8, 2022

sbc100 commented Apr 12, 2022

kjlubick commented Apr 13, 2022

sbc100 commented Apr 13, 2022

sbc100 commented Apr 13, 2022

kjlubick commented Apr 13, 2022

walkingeyerobot commented Apr 14, 2022

sbc100 commented Apr 14, 2022

kjlubick commented Apr 18, 2022

Use non-symlinked version of google-closure-compiler #16640

Use non-symlinked version of google-closure-compiler #16640

Conversation

kjlubick commented Mar 31, 2022

kripken commented Apr 4, 2022

kjlubick commented Apr 4, 2022

walkingeyerobot commented Apr 4, 2022

walkingeyerobot commented Apr 6, 2022

kripken left a comment

Choose a reason for hiding this comment

kripken Apr 7, 2022

Choose a reason for hiding this comment

kjlubick Apr 8, 2022 • edited Loading

Choose a reason for hiding this comment

kripken Apr 8, 2022

Choose a reason for hiding this comment

kjlubick commented Apr 8, 2022

kripken left a comment

Choose a reason for hiding this comment

kjlubick commented Apr 8, 2022

sbc100 commented Apr 12, 2022

kjlubick commented Apr 13, 2022

sbc100 commented Apr 13, 2022

sbc100 commented Apr 13, 2022

kjlubick commented Apr 13, 2022

walkingeyerobot commented Apr 14, 2022

sbc100 commented Apr 14, 2022

kjlubick commented Apr 18, 2022

kjlubick Apr 8, 2022 •

edited

Loading