-
Notifications
You must be signed in to change notification settings - Fork 697
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
Bazel build with LTO fails #807
Comments
Sounds like have more stuff we might want to add the pre-built cache there. |
That makes sense. I believe the LTO builds use LTO-built variants of system libraries. Any pointers on how to add things to the pre built cache? |
I think the bazel SDK is based on the emsdk binaries. emsdk itself ships just common configurations of libraries and does not set FROZEN_CACHE. So there are a few options:
@walkingeyerobot will probably have some idea about which makes sense here. |
Hi @walkingeyerobot , what do you think would be the best path forward here? |
Hey, sorry for the late reply! I think we should go with option 1, adding more configs to the emsdk build. I don't think that will push the emsdk builds to unreasonable sizes, and I think it's intuitive for users to have both LTO and non-LTO builds work. If emsdk maintainers are uncomfortable with this, we can certainly look into the other two options. |
cc @trybka |
That sounds good to me. What are the next steps here? |
Hi @walkingeyerobot @trybka @sbc100, sorry for the ping, is there any way I can help push this forward? |
I think the quickest solution would be to run You could probably get this working today (without landing any change here) by creating your own docker imagine based on the official one that just runs that extra command. This is very simple with docker because of that layering tech. |
Any way to do that with the bazel toolchain? I believe the bazel toolchain downloads its own release bundle to use, so running bazel within docker still wouldn't give you access to the libs built in the container? I guess we could ditch the toolchain for now and work off of the docker image? |
Ah sorry I forgot we were chatting about the bazel toolchain. What happens if you simply remove |
This seems to work but requires having write permissions to |
Do you not have write access to your own home directory? Or is an artifact of the way blaze tries to setup its hermetic environment? |
I think it's related to the hermetic environment, yeah. I think, at least in sandbox mode, that it's run this way intentionally. Haven't 100% confirmed though |
Confirmed. It works with |
What are the next steps to move this issue forward? |
@sbc100 what builds the archives hosted at https://storage.googleapis.com/webassembly/emscripten-release-builds? can that be updated to include lto builds for future emscripten versions? |
The code that builds the releases lives at https://chromium.googlesource.com/emscripten-releases/ and runs on chromium CI infra at: https://ci.chromium.org/p/emscripten-releases The process is still in flux but currently @dschuff (or one of us) has to manually trigger an LTO build for each release we bless. |
Wait, are we talking about building wasm LTO builds with a bazel toolchain, or using LTO-optimized compiler binaries? |
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]>
The first option, so that bazel can compile with lto |
I'm trying to work around this for my Org while we wait for upstream support (ideally as discussed in #971), I've done as you suggested:
How do I turn this docker image into an archive compatible with the bazel toolchain? |
When using the Bazel emscripten toolchain as outlined here, when building and linking with LTO via
--copt="-flto" --linkopt="-flto"
, the linking step seems to fail because of the frozen cache setting in use here.Reproducible so far on MacOS and Linux. This is an example error trace with a bit of internal nomenclature removed.
The text was updated successfully, but these errors were encountered: