-
Notifications
You must be signed in to change notification settings - Fork 33
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
Specify needed shared libs in wasm kernel spec #221
Conversation
Quite some things won't be required for the wasm kernel as mentioned here (#185) I shall make the changes in |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #221 +/- ##
==========================================
+ Coverage 80.72% 81.03% +0.30%
==========================================
Files 19 19
Lines 970 970
Branches 93 93
==========================================
+ Hits 783 786 +3
+ Misses 187 184 -3 |
As compared to the native kernel, the wasm kernel spec won't be having
This is because even if we want to access an environment variables i.e. our wasm module needs something, it needs to end up in our js file (xcpp.js) before the wasm module is initialized and that is done through a
This is because, we can't use resource dir this way through a flag. For accessing the resource dir, first we want CppInterOp to provide a resource dir that needs to be preloaded along side the sysroot using --preload-file.
Again we can't access a local path like this After running jupyter lite build, we can see the _output directory would fetch all the required kernel packages (the tar.gz files) and create an
This basically leads to unpacking contents from these packages in the browser before we create the interpreter. ![]() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Just a small question
"metadata": { | ||
"debugger": false, | ||
"shared": { | ||
"libclangCppInterOp.so": "lib/libclangCppInterOp.so" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌🏽
@@ -71,12 +71,14 @@ jobs: | |||
- name: Jupyter Lite integration | |||
shell: bash -l {0} | |||
run: | | |||
micromamba create -n xeus-lite-host jupyterlite-core | |||
micromamba create -n xeus-lite-host jupyterlite-core jupyter_server notebook |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is notebook really required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really but I think was added earlier by a contributor as an alternative to lab, hence the change is present there. Keeping it for now and can be removed subsequently if not adding value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as an alternative to lab
Installing notebook will have no impact on this concerning JupyterLite.
It's the jupyter lite build --apps notebook --apps lab
which impacts which kinds of UIs are installed.
JupyterLite already defaults to providing the notebook UI actually, you'll only need to point to a different URL. e.g. for xeus-r you can go on https://jupyter-xeus.github.io/xeus-r/tree instead of https://jupyter-xeus.github.io/xeus-r/lab
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh okay wow, I didn't know we could frame the url like this. Getting rid of it !
cp $PREFIX/bin/xcpp.data _output/extensions/@jupyterlite/xeus/static | ||
cp $PREFIX/lib/libclangCppInterOp.so _output/extensions/@jupyterlite/xeus/static |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to see this being removed :D
Thanks for the reviews. Merging ! |
Fixes #219
Built on top of #220 and should be rebased once that goes in