Add Initial Support for xeus-cpp-lite #199
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #199 +/- ##
==========================================
- Coverage 80.80% 80.72% -0.08%
==========================================
Files 19 19
Lines 974 970 -4
Branches 93 93
==========================================
- Hits 787 783 -4
Misses 187 187
|
| Libraries etc coming out of emscripten's sysroot. Assuming we are still inside build we should do the following | ||
| ```bash | ||
| cp xcpp.data _output/extensions/@jupyterlite/xeus/static | ||
| cp $PREFIX/lib/libclangCppInterOp.so _output/extensions/@jupyterlite/xeus/static |
There was a problem hiding this comment.
Though we would always need to copy xcpp.data to a location that is accessible by Jupyter Lite (either _output/xeus/bin or _output/extensions/@jupyterlite/xeus/static that already have access to xcpp.js and xcpp.wasm .......
The second line here shifting libclangCppInterOp.so would only be required temporarily. This is only because JupyterLite doesn't deal with shared libs as expected as I think it expects static libs. Hence once we shift to emsdk 3.1.73 and can support a static lib (libclangCppInterOp.a) this would no longer be required !
Hence this is temporary.
| # 2) If the above cannot be done, we can use the resource dir provided | ||
| # by llvm on emscripten-forge but would involve adding a dependency. | ||
| # 3) Shift the resource dir and the sysroot to a common location. | ||
| # 4) Preload everything required together. |
There was a problem hiding this comment.
TODO with respect to preloading raised here. Please let me know of any questions you'll have or this needs to be rephrased.
There was a problem hiding this comment.
Technically speaking in the long run we would have a better method to load files (and libraries like clad, xsimd etc) dynamically using mambajs(https://github.com/emscripten-forge/mambajs) that is currently in the works.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| # to be able to deal with arguments present in kernel.json | ||
| # 3) Finally we should fetch the C++ version from the kernel.json file and | ||
| # be able to pass it to our wasm interpreter rather than forcing a version. | ||
| configure_kernel("/share/jupyter/kernels/xcpp20/") |
There was a problem hiding this comment.
TODO with respect to supporting more kernels raised here. We would be going ahead with xcpp20 for now but would like to support all.
Please let me know of any questions you'll have or this needs to be rephrased.
| @@ -84,7 +84,7 @@ if(EMSCRIPTEN) | |||
| set(XEUS_CPP_USE_SHARED_XEUS_CPP OFF) | |||
| set(XEUS_CPP_BUILD_TESTS OFF) | |||
| # ENV (https://github.com/emscripten-core/emscripten/commit/6d9681ad04f60b41ef6345ab06c29bbc9eeb84e0) | |||
| set(EMSCRIPTEN_FEATURES "${EMSCRIPTEN_FEATURES} -s \"EXTRA_EXPORTED_RUNTIME_METHODS=[ENV']\"") | |||
| set(EMSCRIPTEN_FEATURES "${EMSCRIPTEN_FEATURES} -s \"EXPORTED_RUNTIME_METHODS=[ENV']\"") | |||
There was a problem hiding this comment.
EXTRA_EXPORTED_RUNTIME_METHODS had been deprecated in favour of EXPORTED_RUNTIME_METHODS since 2.0.18
| @@ -42,6 +43,7 @@ void* createInterpreter(const Args &ExtraArgs = {}) { | |||
| ClangArgs.push_back("-isystem"); | |||
| ClangArgs.push_back(CxxInclude.c_str()); | |||
| } | |||
| #endif | |||
There was a problem hiding this comment.
These instructions( DetectResourceDir and DetectSystemCompilerIncludePaths ) can never be put to use as they are based on exec which emscripten doesn't plan on supporting now or in future.
There was a problem hiding this comment.
Even if we are okay having errors coming up in the console due to this. They lead to other problems anyways.
src/xinterpreter.cpp
Outdated
| Cpp::AddIncludePath((xeus::prefix_path() + "/include/").c_str()); | ||
| #ifndef EMSCRIPTEN | ||
| Cpp::AddIncludePath((xeus::prefix_path() + "/include/").c_str()); | ||
| #endif |
There was a problem hiding this comment.
This would end up giving up $MAMBA_ROOT_PREFIX/envs/xeus-cpp-wasm-host/include/ which is of no use to us as it's a local path web assembly would just want to ignore.
But even if we go for it, it would lead to path lookup errors for now.
There was a problem hiding this comment.
This can go in createInterpreter as CxxSystemIncludes.push_back(xeus::prefix_path() + "/include/")
There was a problem hiding this comment.
Hmm kinda confused as to why this should be treated as a system include rather than using AddIncludePath
Do we want to have something like -isystem /Users/anutosh491/micromamba/envs/xeus-cpp/include/ while building xeus-cpp for non wasm cases ?
There was a problem hiding this comment.
As we won't be interested in the init_includes method as a whole for the wasm case, we could only have it for the non-wasm case at the top itself
interpreter::interpreter(int argc, const char* const* argv) :
.....
{
.....
init_includes();
init_preamble();
init_magic();
}
We could only enable it for the non wasm case rather than doing this.
void interpreter::init_includes()
{
#ifndef EMSCRIPTEN
Cpp::AddIncludePath((xeus::prefix_path() + "/include/").c_str());
#endif
}
The same applies for init_magics as we have the following (though I strongly believe the file one can be supported in the near future.)
void interpreter::init_magic()
{
#ifndef EMSCRIPTEN
preamble_manager["magics"].get_cast<xmagics_manager>().register_magic("xassist", xassist());
preamble_manager["magics"].get_cast<xmagics_manager>().register_magic("file", writefile());
#endif
}
There was a problem hiding this comment.
Hmm kinda confused as to why this should be treated as a system include rather than using
AddIncludePathDo we want to have something like
-isystem /Users/anutosh491/micromamba/envs/xeus-cpp/include/while building xeus-cpp for non wasm cases ?
Isn’t that folder containing other conda packages?
There was a problem hiding this comment.
Yes, I get it. Let's do one thing. As we don't expect the init_include, init_magic or init_preamble to work for starters.
Hence a place we could start with rather than addressing everything separately is this as what I've done in my latest commit.
#ifndef EMSCRIPTEN
init_includes();
init_preamble();
init_magic();
#endif
And one by one whatever we expect to get working for eg
- introspection like
?std::vector - the file magic
We could start separating those out. And the ones that we don't expect to work for eg xassist depends on curl and curl is something that won't be working for wasm as of now .... so these would still be kept to be undefined in case of a wasm build.
EDIT: Argh, playing with the CI a bit tells me that even the above might not be a great approach, hence reverted the changes. Can't think of a nice way to address this for now. Maybe let's keep it like this for now and make a point to improve in near future ?
There was a problem hiding this comment.
Why not doing what I suggested in the first place?
There was a problem hiding this comment.
My doubt is do we get rid of the init_includes method overall and just shift the functionality to createInterpreter ?
There was a problem hiding this comment.
This can go in
createInterpreterasCxxSystemIncludes.push_back(xeus::prefix_path() + "/include/")
That' what I am asking in this PR.
There was a problem hiding this comment.
That' what I am asking in this PR.
Actually you know what, the init_includes doesn't even play a big part as we think it is !!
That's because it's overlapping with the following in kernel.json.in
xeus-cpp/share/jupyter/kernels/xcpp20/kernel.json.in
Lines 12 to 13 in a7649c0
So the location we are interested in is taken care of anyways as I see the following locally coming out of kernel.json
#include "..." search starts here:
#include <...> search starts here:
/Users/anutosh491/micromamba/envs/xeus-cpp/include
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/c++/v1
/Library/Developer/CommandLineTools/usr/lib/clang/15.0.0/include
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include
/Library/Developer/CommandLineTools/usr/include
/Users/anutosh491/micromamba/envs/xeus-cpp/bin/../include/c++/v1
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks (framework directory)
End of search list.
|
Also some more context. We need a way to store the temporarily generated files ( i) Jupyter Lite uses kind-of a fetch-based FS, except it's an in-browser server proxy. But it acts as if files were coming from a normal Jupyter server. There might be better methods to address this that I shall look into but for now the most straightforward way to do this is using this patch. (https://github.com/emscripten-forge/recipes/blob/main/recipes/recipes_emscripten/llvm/patches/shift_temporary_files_to_tmp_dir.patch) And the discussion with Sam Clegg (maintainer of emscripten) and Martin Renou (maintainer of Jupyter Lite) here Hence to get the above working
|
|
The above would give you the following Screen.Recording.2024-12-18.at.4.13.16.PM.mov |
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
| @@ -355,11 +360,6 @@ __get_cxx_version () | |||
| publish_stream("stderr", s); | |||
| } | |||
|
|
|||
| void interpreter::init_includes() | |||
| { | |||
| Cpp::AddIncludePath((xeus::prefix_path() + "/include/").c_str()); | |||
There was a problem hiding this comment.
This needs to go in createInterpreter as I suggested in previous comment.
There was a problem hiding this comment.
I think we should be able to do without it but let's do the above if you think we would need that !
There was a problem hiding this comment.
We had that functionality before and I do not see anything that explains why we should drop it. I think we should drop it generally and solve the problem with parameters to the kernel. If we want that pleas update the commit message and we can drop.
There was a problem hiding this comment.
Hmm, maybe check this comment (#199 (comment))
So
- we anyways don't need it for the wasm case.
- I do not see any difference including it or not including for the non-wasm cases . I see the following locally with or without it.
"/Users/anutosh491/micromamba/envs/xeus-cpp/bin/xcpp" -cc1 -triple arm64-apple-macosx14.0.0 -Wundef-prefix=TARGET_OS_ -Werror=undef-prefix -Wdeprecated-objc-isa-usage -Werror=deprecated-objc-isa-usage -E -disable-free -clear-ast-before-backend -disable-llvm-verifier -discard-value-names -main-file-name "<<< inputs >>>" -mrelocation-model pic -pic-level 2 -pic-is-pie -mframe-pointer=non-leaf -ffp-contract=on -fno-rounding-math -funwind-tables=1 -target-sdk-version=14.4 -fcompatibility-qualified-id-block-type-checking -fvisibility-inlines-hidden-static-local-var -fbuiltin-headers-in-system-modules -fdefine-target-os-macros -target-cpu apple-m1 -target-feature +zcm -target-feature +zcz -target-feature +v8.4a -target-feature +aes -target-feature +altnzcv -target-feature +ccdp -target-feature +complxnum -target-feature +crc -target-feature +dotprod -target-feature +fp-armv8 -target-feature +fp16fml -target-feature +fptoint -target-feature +fullfp16 -target-feature +jsconv -target-feature +lse -target-feature +neon -target-feature +pauth -target-feature +perfmon -target-feature +predres -target-feature +ras -target-feature +rcpc -target-feature +rdm -target-feature +sb -target-feature +sha2 -target-feature +sha3 -target-feature +specrestrict -target-feature +ssbs -target-abi darwinpcs -debugger-tuning=lldb -fdebug-compilation-dir=/Users/anutosh491/work/xeus-cpp/build5 -target-linker-version 951.9 -v -fcoverage-compilation-dir=/Users/anutosh491/work/xeus-cpp/build5 -resource-dir /Users/anutosh491/micromamba/envs/xeus-cpp/lib/clang/19 -isystem /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/c++/v1 -isystem /Library/Developer/CommandLineTools/usr/lib/clang/15.0.0/include -isystem /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include -isystem /Library/Developer/CommandLineTools/usr/include -isystem "/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks (framework directory)" -include new -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk -I /Users/anutosh491/micromamba/envs/xeus-cpp/include -internal-isystem /Users/anutosh491/micromamba/envs/xeus-cpp/bin/../include/c++/v1 -internal-isystem /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/local/include -internal-isystem /Users/anutosh491/micromamba/envs/xeus-cpp/lib/clang/19/include -internal-externc-isystem /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include -std=c++20 -fdeprecated-macro -ferror-limit 19 -stack-protector 1 -fblocks -fencode-extended-block-signature -fregister-global-dtors-with-atexit -fgnuc-version=4.2.1 -fno-implicit-modules -fskip-odr-check-in-gmf -fcxx-exceptions -fexceptions -fmax-type-align=16 -fcolor-diagnostics -fincremental-extensions -D__GCC_HAVE_DWARF2_CFI_ASM=1 -o - -x c++ "<<< inputs >>>"
clang -cc1 version 19.1.2 based upon LLVM 19.1.2 default target arm64-apple-darwin23.5.0
ignoring nonexistent directory "/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks (framework directory)"
ignoring nonexistent directory "/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/local/include"
ignoring nonexistent directory "/Users/anutosh491/micromamba/envs/xeus-cpp/lib/clang/19/include"
ignoring nonexistent directory "/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/Library/Frameworks"
ignoring duplicate directory "/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include"
#include "..." search starts here:
#include <...> search starts here:
/Users/anutosh491/micromamba/envs/xeus-cpp/include
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/c++/v1
/Library/Developer/CommandLineTools/usr/lib/clang/15.0.0/include
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include
/Library/Developer/CommandLineTools/usr/include
/Users/anutosh491/micromamba/envs/xeus-cpp/bin/../include/c++/v1
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks (framework directory)
End of search list.
Hence I removed it.
There was a problem hiding this comment.
You won't see a difference with that command as the include path is added at a later stage than printing the invocation commands.
Whatever, if somebody wants to re-enable this, we can solve it in a more consistent way by adding a -I in the kernel.json.
Description
Please include a summary of changes, motivation and context for this PR.
Fixes # (issue)
Type of change
Please tick all options which are relevant.