-
Notifications
You must be signed in to change notification settings - Fork 28
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
Add Initial Support for xeus-cpp-lite #199
Changes from all commits
665dff7
7b92a8f
83f6d17
ce35626
b04f641
29e82ee
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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']\"") | ||
endif() | ||
|
||
# Dependencies | ||
|
@@ -152,8 +152,22 @@ function(configure_kernel kernel) | |
endfunction() | ||
|
||
message("Configure kernels: ...") | ||
configure_kernel("/share/jupyter/kernels/xcpp17/") | ||
configure_kernel("/share/jupyter/kernels/xcpp20/") | ||
if(EMSCRIPTEN) | ||
# TODO: Currently jupyterlite-xeus and xeus-lite do not provide | ||
# methods to fetch information from the arguments present in the | ||
# generated emscripten kernel. | ||
# The following needs to be done here : | ||
# 1) We need to configure the kernel properly | ||
# Check issue https://github.com/compiler-research/xeus-cpp/issues/185. | ||
# 2) Once the above is done we need to add support in jupyterlite-xeus & xeus-lite | ||
# 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TODO with respect to supporting more kernels raised here. We would be going ahead with xcpp20 for now but would like to support all. |
||
else() | ||
configure_kernel("/share/jupyter/kernels/xcpp17/") | ||
configure_kernel("/share/jupyter/kernels/xcpp20/") | ||
endif() | ||
|
||
# Source files | ||
# ============ | ||
|
@@ -401,8 +415,24 @@ if(EMSCRIPTEN) | |
xeus_cpp_set_kernel_options(xcpp) | ||
xeus_wasm_compile_options(xcpp) | ||
xeus_wasm_link_options(xcpp "web,worker") | ||
# TODO: Remove the exported runtime methods | ||
# after the next xeus release. | ||
target_link_options(xcpp PUBLIC | ||
-sEXPORTED_RUNTIME_METHODS=FS,PATH,ERRNO_CODES | ||
anutosh491 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
# add sysroot location here | ||
--preload-file ${SYSROOT_PATH}/include@/include | ||
) | ||
# TODO: Emscripten supports preloading files just once before it generates | ||
# the xcpp.data file (containing the binary representation of the file(s) we | ||
# want to include in our application). | ||
# Hence although we are adding support for Standard Headers, Libraries etc | ||
# through emscripten's sysroot for now, we need to do the following: | ||
# 1) Enable CppInterOp to provide us with a resource dir. | ||
# 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Technically speaking in the long run we would have a better method to load files (and libraries like clad, xsimd etc) dynamically using |
||
endif() | ||
|
||
# Tests | ||
# ===== | ||
|
||
|
@@ -492,6 +522,7 @@ if(EMSCRIPTEN) | |
install(FILES | ||
"$<TARGET_FILE_DIR:xcpp>/xcpp.js" | ||
"$<TARGET_FILE_DIR:xcpp>/xcpp.wasm" | ||
"$<TARGET_FILE_DIR:xcpp>/xcpp.data" | ||
DESTINATION ${CMAKE_INSTALL_BINDIR}) | ||
endif () | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -87,13 +87,15 @@ pushd build | |
export PREFIX=$MAMBA_ROOT_PREFIX/envs/xeus-cpp-wasm-host | ||
export CMAKE_PREFIX_PATH=$PREFIX | ||
export CMAKE_SYSTEM_PREFIX_PATH=$PREFIX | ||
export SYSROOT_PATH=$HOME/emsdk/upstream/emscripten/cache/sysroot | ||
|
||
emcmake cmake \ | ||
-DCMAKE_BUILD_TYPE=Release \ | ||
-DCMAKE_PREFIX_PATH=$PREFIX \ | ||
-DCMAKE_INSTALL_PREFIX=$PREFIX \ | ||
-DXEUS_CPP_EMSCRIPTEN_WASM_BUILD=ON \ | ||
-DCMAKE_FIND_ROOT_PATH_MODE_PACKAGE=ON \ | ||
-DSYSROOT_PATH=$SYSROOT_PATH \ | ||
.. | ||
emmake make install | ||
``` | ||
|
@@ -105,12 +107,20 @@ micromamba activate xeus-lite-host | |
python -m pip install jupyterlite-xeus | ||
jupyter lite build --XeusAddon.prefix=$PREFIX | ||
``` | ||
|
||
We now need to shift necessary files like `xcpp.data` which contains the binary representation of the file(s) | ||
we want to include in our application. As of now this would contain all important files like Standard Headers, | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Though we would always need to copy The second line here shifting Hence this is temporary. |
||
``` | ||
|
||
Once the Jupyter Lite site has built you can test the website locally by executing | ||
```bash | ||
jupyter lite serve --XeusAddon.prefix=$PREFIX | ||
``` | ||
|
||
|
||
## Trying it online | ||
|
||
To try out xeus-cpp interactively in your web browser, just click on the binder link: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,6 +28,9 @@ using Args = std::vector<const char*>; | |
|
||
void* createInterpreter(const Args &ExtraArgs = {}) { | ||
Args ClangArgs = {/*"-xc++"*/"-v"}; // ? {"-Xclang", "-emit-llvm-only", "-Xclang", "-diagnostic-log-file", "-Xclang", "-", "-xc++"}; | ||
#ifdef EMSCRIPTEN | ||
ClangArgs.push_back("-std=c++20"); | ||
#else | ||
if (std::find_if(ExtraArgs.begin(), ExtraArgs.end(), [](const std::string& s) { | ||
return s == "-resource-dir";}) == ExtraArgs.end()) { | ||
std::string resource_dir = Cpp::DetectResourceDir(); | ||
|
@@ -42,6 +45,7 @@ void* createInterpreter(const Args &ExtraArgs = {}) { | |
ClangArgs.push_back("-isystem"); | ||
ClangArgs.push_back(CxxInclude.c_str()); | ||
} | ||
#endif | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These instructions( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
ClangArgs.insert(ClangArgs.end(), ExtraArgs.begin(), ExtraArgs.end()); | ||
// FIXME: We should process the kernel input options and conditionally pass | ||
// the gpu args here. | ||
|
@@ -73,6 +77,7 @@ namespace xcpp | |
static std::string get_stdopt() | ||
{ | ||
// We need to find what's the C++ version the interpreter runs with. | ||
#ifndef EMSCRIPTEN | ||
const char* code = R"( | ||
int __get_cxx_version () { | ||
#if __cplusplus > 202302L | ||
|
@@ -93,12 +98,13 @@ int __get_cxx_version () { | |
} | ||
__get_cxx_version () | ||
)"; | ||
|
||
auto cxx_version = Cpp::Evaluate(code); | ||
return std::to_string(cxx_version); | ||
#else | ||
return "20"; | ||
#endif | ||
anutosh491 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
|
||
interpreter::interpreter(int argc, const char* const* argv) : | ||
xmagics() | ||
, p_cout_strbuf(nullptr) | ||
|
@@ -110,7 +116,6 @@ __get_cxx_version () | |
createInterpreter(Args(argv ? argv + 1 : argv, argv + argc)); | ||
m_version = get_stdopt(); | ||
redirect_output(); | ||
init_includes(); | ||
init_preamble(); | ||
init_magic(); | ||
} | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs to go in createInterpreter as I suggested in previous comment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, maybe check this comment (#199 (comment)) So
Hence I removed it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
} | ||
|
||
void interpreter::init_preamble() | ||
{ | ||
//NOLINTBEGIN(cppcoreguidelines-owning-memory) | ||
|
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.
EXTRA_EXPORTED_RUNTIME_METHODS
had been deprecated in favour ofEXPORTED_RUNTIME_METHODS
since2.0.18