-
Notifications
You must be signed in to change notification settings - Fork 24
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
Initial merge with xeus-clang-repl #14
base: main
Are you sure you want to change the base?
Initial merge with xeus-clang-repl #14
Conversation
0997c40
to
4dc43f7
Compare
667951b
to
9b353e7
Compare
I would like to discuss the use of CppInterOp before getting this in. |
@JohanMabille and @anutosh491 have discussed that at length over IM. I am happy to do so. |
ad5f1b4
to
9b353e7
Compare
@SylvainCorlay and I discussed the plans on CppInterOp including this PR. We decided we should move forward with this. @alexander-penev can you rebase the PR and see what's missing? I suspect we will need a wasm build of CppInterOp. That's already discussed at compiler-research/CppInterOp#183. |
ff6f77f
to
79f2a4e
Compare
As the recipe for the wasm build of CppInterOp here emscripten-forge/recipes#819 uses llvm 17 then it looks #17 will need fixed , and merged first for this PR to be able to pass, once CppInterOp is available in emscripten forge. |
cfbecfd
to
18f39be
Compare
@vgvassilev @alexander-penev For the wasm build, you need to define the extension for the CppInterOp library to .a . Otherwise it looks like it tries to link to the library with the default extension for the operating system (the ci is showing it trying to link to libclangCppInterOp.so li in the wasm build). |
I'm not 100% certain, but I believe the osx Github runners doesn't have conda installed by default (that is why I believe your getting the conda: command not found error in the ci runs). In the CppInterOp CI the easiest way I found of installing it was using miniforge https://github.com/compiler-research/CppInterOp/blob/bd72e91a5f34a8a9507801b6302e91b82b1aa1f8/.github/workflows/ci.yml#L721C1-L723C56 , since it doesn't appear to be available in homebrew. |
Yes, that's the problem I am looking how to solve because emscripten overwrites our shared library mode. However, here we are hardcoding the extension: https://github.com/compiler-research/CppInterOp/blob/bd72e91a5f34a8a9507801b6302e91b82b1aa1f8/cmake/CppInterOp/CppInterOpConfig.cmake.in#L19 I cannot find a canonical cmake use how this should be done properly... |
Where do you see this error? |
(https://github.com/compiler-research/xeus-cpp/actions/runs/8238668637/job/22530744956?pr=14#step:9:14) |
@vgvassilev It looks like you can use CMAKE_FIND_LIBRARY_SUFFIXES https://cmake.org/cmake/help/latest/variable/CMAKE_FIND_LIBRARY_SUFFIXES.html to override the ${CMAKE_SHARED_LIBRARY_SUFFIX} it uses when using the config file. |
I thought it was not harmful but maybe it is. Honestly I am stuck at that stage with the failing |
If you have a particular idea how to fix it maybe we should try to apply it as a patch at the wasm package and see if that works. |
I don't think a patch is needed for CppInterOp. I believe the line set(CMAKE_FIND_LIBRARY_SUFFIXES ".a") is needed in the xeus-cpp CMakeLists.txt if using emscripten, before the line Line 172 in d104fdd
|
It seems that does not work but even if it did it's a little weird. I'd propose for the emscripten package to set |
Probably something like this one: https://github.com/cvc5/cvc5/blob/20e739f7c615f8be8988d22f840b1f71605aad05/cmake/FindCLN.cmake#L79 |
We set BUILD_SHARED_LIBS=Off for Windows builds, so that would cause problems later on. Maybe we should introduce a patch with a CPPINTEROP_EMSCRIPTEN_WASM_BUILD variable like xeus-cpp with its XEUS_CPP_EMSCRIPTEN_WASM_BUILD. We then base CppInterOpConfig.cmake based on that. |
I am not opposed to introducing a new variable. However, if we set |
I can attempt your approach, but it won't be until later in the week. If it needs done sooner, then you or @alexander-penev may need to come up with the patch. |
Unless we resolve the all problems on the other platforms, I don't think we need it quicker... |
7da6402
to
0e655ef
Compare
@@ -74,14 +74,14 @@ jobs: | |||
shell: bash -l {0} | |||
run: | | |||
cd build/test | |||
export CPLUS_INCLUDE_PATH=$CONDA_PREFIX/include:$CONDA_BUILD_SYSROOT:$CONDA_BUILD_SYSROOT/..:$CONDA_BUILD_SYSROOT/../x86_64-conda-linux-gnu/include/c++/12.3.0:$CONDA_BUILD_SYSROOT/../x86_64-conda-linux-gnu/include/c++/12.3.0/include:$CONDA_BUILD_SYSROOT/../include:$CONDA_BUILD_SYSROOT/usr/include:/home/runner/micromamba-root/envs/xeus-cpp/bin/../lib/gcc/x86_64-conda-linux-gnu/12.3.0/include:/home/runner/micromamba-root/envs/xeus-cpp/bin/../lib/gcc/x86_64-conda-linux-gnu/12.3.0/include-fixed:/home/runner/micromamba-root/envs/xeus-cpp/bin/../lib/gcc/x86_64-conda-linux-gnu/12.3.0/../../../../x86_64-conda-linux-gnu/include:/home/runner/micromamba-root/envs/xeus-cpp/bin/../x86_64-conda-linux-gnu/sysroot/usr/include |
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.
Just curious to know what is this export being used for !
interpreter_ptr interp_ptr = interpreter_ptr(new xcpp::interpreter(interpreter_args.size(), interpreter_args.data())); | ||
return interp_ptr; | ||
} | ||
|
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.
These functions have been defined here (https://github.com/compiler-research/xeus-cpp/blob/main/src/xutils.cpp) and don't need to be redefined here
No description provided.