Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build_bazel_rules_nodejs default configuration fails to invoke google-closure-compiler correctly #907

Closed
daveb68 opened this issue Oct 1, 2021 · 0 comments · Fixed by #912

Comments

@daveb68
Copy link

daveb68 commented Oct 1, 2021

There is something wrong with the version of the build_bazel_rules_nodejs repository specified by:

http_archive(
            name = "build_bazel_rules_nodejs",
            sha256 = "0f2de53628e848c1691e5729b515022f5a77369c76a09fbe55611e12731c90e3",
            urls = ["https://github.com/bazelbuild/rules_nodejs/releases/download/2.0.1/rules_nodejs-2.0.1.tar.gz"],
        )

which is the default you get from the emsdk due to its use in the file: emsdk/bazel/deps.bzl

When you try to use the "-closure 1" argument on a production build, this fails due to some problem with how the node binary from bazel processes the symbolic link from google-closure-compiler to ../.../cli.js incorrectly. However, if you update to the latest version of the build_bazel_rules_nodejs repository, the problem goes away.

The new version I tried that resolved my issue was:

http_archive(
    name = "build_bazel_rules_nodejs",
    sha256 = "3635797a96c7bfcd0d265dacd722a07335e64d6ded9834af8d3f1b7ba5a25bba",
    urls = ["https://github.com/bazelbuild/rules_nodejs/releases/download/4.3.0/rules_nodejs-4.3.0.tar.gz"],
)

from the page:

 https://github.com/bazelbuild/rules_nodejs/releases

Of course, if you place the newer version in your WORKSPACE file, bazel respects this and you can get around this issue pretty easily, once you pay the cost of diagnosing it. Fixing this will assist new users not spend time troubleshooting problems like this.

For the record, I verified that the older version of node was not the problem by manually testing the exact command that was failing from within emcc.py against that version of node when used via nvm instead of running from the bazel executable directory. So I have concluded that the issue is something to do with the rules_nodejs-2.0.1.tar.gz' when running on linux.

blueboxd pushed a commit to blueboxd/skia that referenced this issue Oct 13, 2021
This uses the Bazel rule wasm_cc_binary, which is defined
in @emsdk [1]

Note that wasm_cc_binary does not have a linkopts argument
defined, so we instead put any emcc options in the cc_binary
target.

This works around a few bugs in the emsdk Bazel rules:
 - emscripten-core/emsdk#907
 - emscripten-core/emsdk#807

Prior to PS 5, this CL tried a different way to bring in
the toolchain, a more manual way outlined in [2].

A similar approach (modifying the .bazelrc and specifying
the toolchain directly) might be necessary at some point,
but can probably still be done using the @emsdk Bazel rules
and --config=wasm.

To update the version of emscripten used, we just need to
update the parameter in the WORKSPACE call to emsdk_emscripten_deps().

The example/index.html file in this CL does exactly the same
as [3], except the WebGPU calls are made from C++ via WASM.
I made heavy use of these examples [4], [5] while exploring
APIs. What was also useful was looking at the emscripten
source headers [6], [7], [8], [9].

I also learned a lot about WebGPU from [10].

[1] https://github.com/emscripten-core/emsdk/blob/3891e7b04bf8cbb3bc62758e9c575ae096a9a518/bazel/emscripten_toolchain/wasm_cc_binary.bzl
[2] https://hackernoon.com/c-to-webassembly-using-bazel-and-emscripten-4him3ymc
[3] https://github.com/google/skia/blob/206c1f3f7e01b6d7fd2d3aab13ed719ff39d02e4/demos.skia.org/demos/webgpu/index.html
[4] https://github.com/kainino0x/webgpu-cross-platform-demo
[5] https://github.com/Twinklebear/wgpu-cpp-starter
[6] https://github.com/emscripten-core/emscripten/blob/5e6c74153b85fdb3ee3bf1f339d620f4e7ddf705/system/include/emscripten/html5_webgpu.h
[7] https://github.com/emscripten-core/emscripten/blob/5e6c74153b85fdb3ee3bf1f339d620f4e7ddf705/system/include/webgpu/webgpu.h
[8] https://github.com/emscripten-core/emscripten/blob/5e6c74153b85fdb3ee3bf1f339d620f4e7ddf705/system/include/webgpu/webgpu_cpp.h
[9] https://github.com/emscripten-core/emscripten/blob/5e6c74153b85fdb3ee3bf1f339d620f4e7ddf705/src/library_html5_webgpu.js#L24
[10] https://alain.xyz/blog/raw-webgpu

Change-Id: Iff33b72e7265200b2caacbc03e5fcc06a650b56b
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/457396
Reviewed-by: Leandro Lovisolo <[email protected]>
Reviewed-by: Brian Salomon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant