Repository navigation
Add opt-in Emscripten WebGL/OffscreenCanvas support - #505
Alex-PLACET wants to merge 1 commit into
Conversation
|
Woah sounds pretty cool ! Shall go through it soon. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #505 +/- ##
=======================================
Coverage 72.70% 72.70%
=======================================
Files 18 18
Lines 1209 1209
Branches 115 115
=======================================
Hits 879 879
Misses 330 330 🚀 New features to boost your workflow:
|
|
@anutosh491 can you rerun this build please: https://github.com/compiler-research/xeus-cpp/actions/runs/42032211988/job/127306174459?pr=505 ? |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
I think I like most of what I see here. Maybe one thing that comes to my mind is that we can add a cool demo for people to try out here https://github.com/compiler-research/xeus-cpp/tree/main/notebooks Maybe we can just add it in the lite demo if it's just a cell or two. |
|
Here a link to show simple WebGL canva and log the events: https://alex-placet.github.io/canvas-host/lab/index.html |
If we include this notebook in the PR, we would need the webgl turned on in the Github deployment, and the read the docs PR preview build. |
| cd - | ||
| # Run tests in browsers | ||
| # --enable-unsafe-swiftshader lets Chrome fall back to software WebGL in headless CI, which the WebGL runtime tests need. |
There was a problem hiding this comment.
This comment needs moving to the top of the Test Emscripten xeus-cpp in browser step, where all the other options are explained. Also, Chromiums documentation (https://chromium.googlesource.com/chromium/src/+/main/docs/gpu/swiftshader.md) says SwiftShader is deprecated, so do we really want the tests reliant on an option which may suddenly be removed?
There was a problem hiding this comment.
Yes it's depreacted. The alternatives are to use xvfb but it don't works very well on my machine. We may just drop the chrome tests and keep the firefox one
There was a problem hiding this comment.
I do not support dropping the Chrome tests. What exact error do you get if you don't add any flags, so I know what error you are trying to get past?
There was a problem hiding this comment.
I mean the specific tests related to webgl.
I finally modified the CI. The xvfb is working only with Chrome on Linux
Ahh let it be if it's too hard to get that. |
|
clang-tidy review says "All clean, LGTM! 👍" |
2 similar comments
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
I think this is taking good shape now. We should be able to finish merging Matthew's pr with the emsdk update by tomorrow and then land this I guess! |
mcbarton
left a comment
mcbarton
left a comment
There was a problem hiding this comment.
See comments for why I am requesting changes. Also if we absolutely need the option XEUS_CPP_EMSCRIPTEN_WEBGL then it needs including in the deployment workflow, and the read the docs PR preview.
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
@anutosh491 I ask that you do not approve the workflow to run anymore times until #505 (comment) is addressed. The PR is taking the wrong approach, my review comments are being ignored and I do not appreciate you ignoring my comments either by approving the workflow to run. |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
@mcbarton I don't ignore your comments, I'm iterating on the code and try to have green CI builds. I re-request your review only when the PR is in a good shape to avoid to waste your time. |
I'm away from my computer at the moment to try it out, but I suggest you install ubuntu desktop on the Github runner to see if it then has a display (and see how long it takes to install) That is a better representation of what a user who is developing xeus-cpp on Ubuntu will have access to anyway (there is a probably a mimumal ubuntu desktop package but too busy at the moment to look). Most developers won't use a Ubuntu server like OS which has no GUI. |
|
@mcbarton the real problem is that ubuntu needs a real or a virtual display (ubuntu-desktop or not). |
Give the action a go, and see if it gives you a display. |
|
clang-tidy review says "All clean, LGTM! 👍" |
That appears to work in allowing the tests in run (from a quick glance). I'd just have it skips the tests now (rather than fail) if it doesn't detect the support for webgl in the browser. |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
mcbarton
left a comment
mcbarton
left a comment
There was a problem hiding this comment.
The browser arguments were not updated in the documentation. Also it still says that the browser tests are headless in the documentation, which they are not anymore.
| # These options are Emscripten-only; native builds must not receive them. | ||
| if(NOT XEUS_CPP_WASM_EXPORTED_RUNTIME_METHODS) | ||
| message(FATAL_ERROR | ||
| "xeus-cpp: WebGL tests require the kernel's exported runtime methods" | ||
| ) | ||
| endif() |
There was a problem hiding this comment.
This if block doesn't feel necessary.
mcbarton
left a comment
mcbarton
left a comment
There was a problem hiding this comment.
After these suggested changes I think I am happy. I would just like a second opinion on whether xeus_cpp_wasm_append_runtime_method is needed
|
clang-tidy review says "All clean, LGTM! 👍" |
|
@Alex-PLACET can you resolve the merge conflict and squash all the commits into one? |
Enable the Emscripten WebGL2/OffscreenCanvas link options and export Module.GL for xcpp, add browser WebGL tests, run browser tests non-headless with a Linux display, and document the required Chrome flags.
28c7862 to
1f28a73
Compare
mcbarton
left a comment
mcbarton
left a comment
There was a problem hiding this comment.
LGTM. I'll now pass this to @anutosh491 in case he has any comments. I don't know if he still wants an example notebook added to xeus-cpps deployment, and i'd like his opinion on whether the xeus_cpp_wasm_append_runtime_method function is needed.
The read the docs failure looks unrelated from a quick glance at the output.
|
clang-tidy review says "All clean, LGTM! 👍" |
Description
Add XEUS_CPP_EMSCRIPTEN_WEBGL (OFF by default) to build the xcpp kernel with WebGL2 and OffscreenCanvas support. The option enables the Emscripten WebGL2/OffscreenCanvas link flags, keeps the existing exported runtime methods, and adds GL so transferred OffscreenCanvas objects are available through Module.GL.offscreenCanvases.
Wire the option into the wasm CI build and verify that the generated xcpp.js exports Module.GL and offscreenCanvases. Document the option in the wasm build instructions and build-options reference.
Type of change
Please tick all options which are relevant.