Skip to content
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

Open
wants to merge 69 commits into
base: main
Choose a base branch
from

Conversation

alexander-penev
Copy link
Collaborator

No description provided.

@alexander-penev alexander-penev force-pushed the MergeXeusClangRepl branch 2 times, most recently from 0997c40 to 4dc43f7 Compare October 2, 2023 13:43
@alexander-penev alexander-penev force-pushed the MergeXeusClangRepl branch 2 times, most recently from 667951b to 9b353e7 Compare October 30, 2023 07:29
@SylvainCorlay
Copy link
Collaborator

I would like to discuss the use of CppInterOp before getting this in.

@vgvassilev
Copy link
Contributor

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.

@vgvassilev
Copy link
Contributor

I would like to discuss the use of CppInterOp before getting this in.

@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.

@mcbarton
Copy link
Collaborator

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.

CMakeLists.txt Outdated Show resolved Hide resolved
@mcbarton
Copy link
Collaborator

mcbarton commented Mar 11, 2024

@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).

@mcbarton
Copy link
Collaborator

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.

@vgvassilev
Copy link
Contributor

@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).

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...

@vgvassilev
Copy link
Contributor

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.

Where do you see this error?

@mcbarton
Copy link
Collaborator

mcbarton commented Mar 11, 2024

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.

Where do you see this error?

(https://github.com/compiler-research/xeus-cpp/actions/runs/8238668637/job/22530744956?pr=14#step:9:14)
3rd line of this section for both osx builds.

@mcbarton
Copy link
Collaborator

@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).

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...

@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.

@vgvassilev
Copy link
Contributor

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.

Where do you see this error?

(https://github.com/compiler-research/xeus-cpp/actions/runs/8238668637/job/22530744956?pr=14#step:9:14) 3rd line of this section for both osx builds.

I thought it was not harmful but maybe it is. Honestly I am stuck at that stage with the failing test step... And of course the fact that we need to fix the CppInterOpConfig.cmake...

@vgvassilev
Copy link
Contributor

@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).

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...

@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.

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.

@mcbarton
Copy link
Collaborator

mcbarton commented Mar 11, 2024

@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).

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...

@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.

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

find_package(CppInterOp REQUIRED CONFIG PATHS "${CMAKE_PREFIX_PATH}" "${CMAKE_PREFIX_PATH}/lib" "${CPPINTEROP_DIR}" "${CPPINTEROP_DIR}/lib")

@vgvassilev
Copy link
Contributor

@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).

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...

@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.

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

find_package(CppInterOp REQUIRED CONFIG PATHS "${CMAKE_PREFIX_PATH}" "${CMAKE_PREFIX_PATH}/lib" "${CPPINTEROP_DIR}" "${CPPINTEROP_DIR}/lib")

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 -DBUILD_SHARED_LIBS=Off and then based on that option to adjust out CppInterOpConfig.cmake?

@vgvassilev
Copy link
Contributor

@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).

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...

@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.

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

find_package(CppInterOp REQUIRED CONFIG PATHS "${CMAKE_PREFIX_PATH}" "${CMAKE_PREFIX_PATH}/lib" "${CPPINTEROP_DIR}" "${CPPINTEROP_DIR}/lib")

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 -DBUILD_SHARED_LIBS=Off and then based on that option to adjust out CppInterOpConfig.cmake?

Probably something like this one: https://github.com/cvc5/cvc5/blob/20e739f7c615f8be8988d22f840b1f71605aad05/cmake/FindCLN.cmake#L79

@mcbarton
Copy link
Collaborator

@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).

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...

@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.

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

find_package(CppInterOp REQUIRED CONFIG PATHS "${CMAKE_PREFIX_PATH}" "${CMAKE_PREFIX_PATH}/lib" "${CPPINTEROP_DIR}" "${CPPINTEROP_DIR}/lib")

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 -DBUILD_SHARED_LIBS=Off and then based on that option to adjust out CppInterOpConfig.cmake?

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.

@vgvassilev
Copy link
Contributor

@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).

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...

@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.

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

find_package(CppInterOp REQUIRED CONFIG PATHS "${CMAKE_PREFIX_PATH}" "${CMAKE_PREFIX_PATH}/lib" "${CPPINTEROP_DIR}" "${CPPINTEROP_DIR}/lib")

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 -DBUILD_SHARED_LIBS=Off and then based on that option to adjust out CppInterOpConfig.cmake?

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 BUILD_SHARED_LIBS=Off, wouldn't that mean that on Windows we also want static libraries. That is, my approach will make things more consistent on Windows?

@mcbarton
Copy link
Collaborator

mcbarton commented Mar 11, 2024

@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).

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...

@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.

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

find_package(CppInterOp REQUIRED CONFIG PATHS "${CMAKE_PREFIX_PATH}" "${CMAKE_PREFIX_PATH}/lib" "${CPPINTEROP_DIR}" "${CPPINTEROP_DIR}/lib")

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 -DBUILD_SHARED_LIBS=Off and then based on that option to adjust out CppInterOpConfig.cmake?

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 BUILD_SHARED_LIBS=Off, wouldn't that mean that on Windows we also want static libraries. That is, my approach will make things more consistent on Windows?

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.

@vgvassilev
Copy link
Contributor

@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).

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...

@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.

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

find_package(CppInterOp REQUIRED CONFIG PATHS "${CMAKE_PREFIX_PATH}" "${CMAKE_PREFIX_PATH}/lib" "${CPPINTEROP_DIR}" "${CPPINTEROP_DIR}/lib")

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 -DBUILD_SHARED_LIBS=Off and then based on that option to adjust out CppInterOpConfig.cmake?

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 BUILD_SHARED_LIBS=Off, wouldn't that mean that on Windows we also want static libraries. That is, my approach will make things more consistent on Windows?

I can attempt your approach, but it won't be for a few days. 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...

@@ -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
Copy link
Collaborator

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;
}

Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants