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.

Sign up for GitHub

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 emscripten automated tests #483

Merged
merged 14 commits into from
Mar 14, 2025
Merged

Add emscripten automated tests #483

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
1a597b9
Add automated tests for Emscripten builds
mcbarton Jan 24, 2025
906de42
Merge branch 'main' into wip]-add-automated-wasm-tests
mcbarton Feb 21, 2025
2484e20
Merge branch 'main' into wip]-add-automated-wasm-tests
mcbarton Feb 22, 2025
444c081
Apply PR suggestions
mcbarton Feb 28, 2025
1e124e4
Merge branch 'main' into wip]-add-automated-wasm-tests
mcbarton Feb 28, 2025
0e2d5cc
Merge branch 'main' into wip]-add-automated-wasm-tests
mcbarton Mar 9, 2025
be0c5cd
Update exports.ld
mcbarton Mar 9, 2025
0c80777
Update exports.ld
mcbarton Mar 9, 2025
b77ac02
Merge branch 'main' into wip]-add-automated-wasm-tests
mcbarton Mar 11, 2025
2cdf83f
See if number of undefined symbols reduced
mcbarton Mar 11, 2025
a12e15b
Put back new undefined symbols list
mcbarton Mar 11, 2025
85313c8
Add CPPINTEROP_API to GetOperator. and GetOperatorArity
mcbarton Mar 13, 2025
b6cadd3
Add CPPINTEROP_API in the correct place
mcbarton Mar 13, 2025
57ab2c0
Fix clang format issue
mcbarton Mar 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions .github/workflows/deploy-pages.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,10 +93,11 @@ jobs:
if: ${{ runner.os != 'windows' }}
shell: bash -l {0}
run: |
set -e
./emsdk/emsdk activate ${{matrix.emsdk_ver}}
source ./emsdk/emsdk_env.sh
micromamba create -f environment-wasm.yml --platform=emscripten-wasm32

export SYSROOT_PATH=$PWD/emsdk/upstream/emscripten/cache/sysroot
export PREFIX=$MAMBA_ROOT_PREFIX/envs/CppInterOp-wasm
export CMAKE_PREFIX_PATH=$PREFIX
export CMAKE_SYSTEM_PREFIX_PATH=$PREFIX
Expand Down Expand Up @@ -130,6 +131,7 @@ jobs:
-DCODE_COVERAGE=${{ env.CODE_COVERAGE }} \
-DCMAKE_INSTALL_PREFIX=$PREFIX \
-DCMAKE_FIND_ROOT_PATH_MODE_PACKAGE=ON \
-DSYSROOT_PATH=$SYSROOT_PATH \
../
else
emcmake cmake -DCMAKE_BUILD_TYPE=${{ env.BUILD_TYPE }} \
Expand All @@ -141,13 +143,15 @@ jobs:
-DCODE_COVERAGE=${{ env.CODE_COVERAGE }} \
-DCMAKE_INSTALL_PREFIX=$PREFIX \
-DCMAKE_FIND_ROOT_PATH_MODE_PACKAGE=ON \
-DSYSROOT_PATH=$SYSROOT_PATH \
../
fi

emmake make -j ${{ env.ncpus }} check-cppinterop
emmake make -j ${{ env.ncpus }} install

cd ..


echo "SYSROOT_PATH=$SYSROOT_PATH" >> $GITHUB_ENV
echo "CB_PYTHON_DIR=$CB_PYTHON_DIR" >> $GITHUB_ENV
echo "CPPINTEROP_BUILD_DIR=$CPPINTEROP_BUILD_DIR" >> $GITHUB_ENV
echo "CPPINTEROP_DIR=$CPPINTEROP_DIR" >> $GITHUB_ENV
Expand All @@ -160,7 +164,6 @@ jobs:
run: |
./emsdk/emsdk activate ${{matrix.emsdk_ver}}
source ./emsdk/emsdk_env.sh
export SYSROOT_PATH=$PWD/emsdk/upstream/emscripten/cache/sysroot
micromamba activate CppInterOp-wasm
git clone --depth=1 https://github.com/compiler-research/xeus-cpp.git
cd ./xeus-cpp
Expand All @@ -175,7 +178,7 @@ jobs:
-DXEUS_CPP_EMSCRIPTEN_WASM_BUILD=ON \
-DCMAKE_FIND_ROOT_PATH_MODE_PACKAGE=ON \
-DCppInterOp_DIR="${{ env.CPPINTEROP_BUILD_DIR }}/lib/cmake/CppInterOp" \
-DSYSROOT_PATH=$SYSROOT_PATH \
-DSYSROOT_PATH=${{ env.SYSROOT_PATH }} \
..
emmake make -j ${{ env.ncpus }} install

Expand Down
15 changes: 9 additions & 6 deletions .github/workflows/emscripten.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -548,10 +548,11 @@ jobs:
if: ${{ runner.os != 'windows' }}
shell: bash -l {0}
run: |
set -e
./emsdk/emsdk activate ${{matrix.emsdk_ver}}
source ./emsdk/emsdk_env.sh
micromamba create -f environment-wasm.yml --platform=emscripten-wasm32

export SYSROOT_PATH=$PWD/emsdk/upstream/emscripten/cache/sysroot
export PREFIX=$MAMBA_ROOT_PREFIX/envs/CppInterOp-wasm
export CMAKE_PREFIX_PATH=$PREFIX
export CMAKE_SYSTEM_PREFIX_PATH=$PREFIX
Expand Down Expand Up @@ -585,6 +586,7 @@ jobs:
-DCMAKE_INSTALL_PREFIX=$PREFIX \
-DCMAKE_FIND_ROOT_PATH_MODE_PACKAGE=ON \
-DLLVM_ENABLE_WERROR=On \
-DSYSROOT_PATH=$SYSROOT_PATH \
../
else
emcmake cmake -DCMAKE_BUILD_TYPE=${{ env.BUILD_TYPE }} \
Expand All @@ -597,13 +599,15 @@ jobs:
-DCMAKE_INSTALL_PREFIX=$PREFIX \
-DCMAKE_FIND_ROOT_PATH_MODE_PACKAGE=ON \
-DLLVM_ENABLE_WERROR=On \
-DSYSROOT_PATH=$SYSROOT_PATH \
../
fi

emmake make -j ${{ env.ncpus }} install

emmake make -j ${{ env.ncpus }} check-cppinterop
emmake make -j ${{ env.ncpus }} install
cd ..


echo "SYSROOT_PATH=$SYSROOT_PATH" >> $GITHUB_ENV
echo "CB_PYTHON_DIR=$CB_PYTHON_DIR" >> $GITHUB_ENV
echo "CPPINTEROP_BUILD_DIR=$CPPINTEROP_BUILD_DIR" >> $GITHUB_ENV
echo "CPPINTEROP_DIR=$CPPINTEROP_DIR" >> $GITHUB_ENV
Expand All @@ -616,7 +620,6 @@ jobs:
run: |
./emsdk/emsdk activate ${{matrix.emsdk_ver}}
source ./emsdk/emsdk_env.sh
export SYSROOT_PATH=$PWD/emsdk/upstream/emscripten/cache/sysroot
micromamba activate CppInterOp-wasm
git clone --depth=1 https://github.com/compiler-research/xeus-cpp.git
cd ./xeus-cpp
Expand All @@ -631,6 +634,6 @@ jobs:
-DXEUS_CPP_EMSCRIPTEN_WASM_BUILD=ON \
-DCMAKE_FIND_ROOT_PATH_MODE_PACKAGE=ON \
-DCppInterOp_DIR="${{ env.CPPINTEROP_BUILD_DIR }}/lib/cmake/CppInterOp" \
-DSYSROOT_PATH=$SYSROOT_PATH \
-DSYSROOT_PATH=${{ env.SYSROOT_PATH }} \
..
emmake make -j ${{ env.ncpus }} install
7 changes: 1 addition & 6 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,7 @@ endif()

# Add appropriate flags for GCC
if (LLVM_COMPILER_IS_GCC_COMPATIBLE)
if (APPLE)
if (APPLE OR EMSCRIPTEN)
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fno-common -Woverloaded-virtual -Wcast-qual -fno-strict-aliasing -Wno-long-long -Wall -W -Wno-unused-parameter -Wwrite-strings")
else()
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fno-common -Woverloaded-virtual -Wcast-qual -fno-strict-aliasing -pedantic -Wno-long-long -Wall -W -Wno-unused-parameter -Wwrite-strings")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Among both of these I see that the pedantic flag is whats different between both the "set...." options we have

I don't think pedantic has a huge role here but do you have any specific reason to remove it ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason for removing it is because it is passed through to the emscripten build of gtest, creating a warning, and as we treat all warnings as errors, then the gtest build errors out. Same reason it was remove during the apple build.

Copy link
Collaborator

@anutosh491 anutosh491 Feb 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay if that's the case I am not sure of this change.

This is the global cmake file and making changes here affect not just the build for unittests but also the library.

I see the following CXX_FLAGS are being passed after your change while building libclangCppInterOp.so

CXX_FLAGS =  -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough  -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -Wcast-qual -fno-strict-aliasing -Wno-long-long -Wall -W -Wno-unused-parameter -Wwrite-strings  -Wno-sign-compare -O3 -DNDEBUG -std=c++17 -fPIC -s SIDE_MODULE=1 -UNDEBUG

And this doesn't have pedantic which it used to have originally.

I haven't gotten to the gtest thing yet. My first aim is to ensure the library that we were building already is built without any unncessary changes.

So yeah this might have to play a role for gtest (but if that's the case you try supressing it there to avoid duplication) . Supressing it here changes the cxx_flags for the lib

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vgvassilev On this point I don't agree. This change is necessary. This fix is what we settled on when we encountered the same issue upgrading to the latest gtest for MacOS builds. No idea why its acceptable in the case of native builds, but no Emscripten builds.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fix is what we settled on when we encountered the same issue upgrading to the latest gtest for MacOS builds

Not sure if this was discussed before. Don't have enough context.

Neither do I think pedantic is a game changer here ! But commenting as a reviewer is tough unless I don't know of the error (looks just randomly removed to me)

Maybe paste the error here ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vgvassilev On this point I don't agree. This change is necessary. This fix is what we settled on when we encountered the same issue upgrading to the latest gtest for MacOS builds. No idea why its acceptable in the case of native builds, but no Emscripten builds.

Can we get more context on the error?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anutosh491 @vgvassilev Here is what happens if you keep the pedantic flag ttps://github.com/compiler-research/CppInterOp/actions/runs/13412927760/job/37466965423?pr=483#step:9:250 .

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. In that case can we limit flag just for gtest?

Copy link
Collaborator Author

@mcbarton mcbarton Feb 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the initial approach I took in the gtest PR (here #344 (comment)), but we ended up at the solution we have here (I think to still have consistency between the flags applied to gtest and CppInterOp, but cannot be certain).

Expand Down Expand Up @@ -451,11 +451,6 @@ option(CPPINTEROP_ENABLE_DOXYGEN "Use doxygen to generate CppInterOp interal API
option(CPPINTEROP_ENABLE_SPHINX "Use sphinx to generage CppInterOp user documentation")


if(EMSCRIPTEN)
message("Build with emscripten")
set(CPPINTEROP_ENABLE_TESTING OFF)
endif()

if(MSVC)

set(MSVC_EXPORTLIST
Expand Down
13 changes: 10 additions & 3 deletions Emscripten-build-instructions.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,12 @@ git clone https://github.com/emscripten-core/emsdk.git
./emsdk/emsdk install 3.1.73
```

and activate the emsdk environment
and activate the emsdk environment (we are defining SYSROOT_PATH for use later)

```bash
./emsdk/emsdk activate 3.1.73
source ./emsdk/emsdk_env.sh
export SYSROOT_PATH=$PWD/emsdk/upstream/emscripten/cache/sysroot
```

Now clone the 19.x release of the LLVM project repository and CppInterOp (the building of the emscripten version of llvm can be
Expand Down Expand Up @@ -97,7 +98,7 @@ export CMAKE_PREFIX_PATH=$PREFIX
export CMAKE_SYSTEM_PREFIX_PATH=$PREFIX
```

Now to build CppInterOp execute the following
Now to build and test your Emscripten build of CppInterOp by executing the following

```bash
mkdir build
Expand All @@ -109,7 +110,14 @@ emcmake cmake -DCMAKE_BUILD_TYPE=Release \
-DBUILD_SHARED_LIBS=ON \
-DCMAKE_FIND_ROOT_PATH_MODE_PACKAGE=ON \
-DCMAKE_INSTALL_PREFIX=$PREFIX \
-DSYSROOT_PATH=$SYSROOT_PATH \
../
emmake make -j $(nproc --all) check-cppinterop
```

Assuming it passes all test you can install by executing the following

```bash
emmake make -j $(nproc --all) install
```

Expand All @@ -126,7 +134,6 @@ the CppInterOp build folder, you can build the wasm version of xeus-cpp by execu

```bash
cd ../..
export SYSROOT_PATH=$PWD/emsdk/upstream/emscripten/cache/sysroot
git clone --depth=1 https://github.com/compiler-research/xeus-cpp.git
cd ./xeus-cpp
mkdir build
Expand Down
16 changes: 13 additions & 3 deletions cmake/modules/GoogleTest.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,20 @@ if(WIN32)
-DCMAKE_ARCHIVE_OUTPUT_DIRECTORY_MINSIZEREL:PATH=${_gtest_byproduct_binary_dir}/lib/
-DCMAKE_ARCHIVE_OUTPUT_DIRECTORY_RELEASE:PATH=${_gtest_byproduct_binary_dir}/lib/
-DCMAKE_ARCHIVE_OUTPUT_DIRECTORY_RELWITHDEBINFO:PATH=${_gtest_byproduct_binary_dir}/lib/
-Dgtest_force_shared_crt=ON
BUILD_COMMAND ${CMAKE_COMMAND} --build <BINARY_DIR> --config $<CONFIG>)
-Dgtest_force_shared_crt=ON)
elseif(APPLE)
set(EXTRA_GTEST_OPTS -DCMAKE_OSX_SYSROOT=${CMAKE_OSX_SYSROOT})
endif()

include(ExternalProject)
if (EMSCRIPTEN)
set(config_cmd emcmake cmake)
set(build_cmd emmake make)
else()
set(config_cmd ${CMAKE_COMMAND})
set(build_cmd ${CMAKE_COMMAND} --build ${CMAKE_BINARY_DIR}/unittests/googletest-prefix/src/googletest-build/ --config $<CONFIG>)
endif()

ExternalProject_Add(
googletest
GIT_REPOSITORY https://github.com/google/googletest.git
Expand All @@ -31,7 +38,9 @@ ExternalProject_Add(
# CMAKE_ARGS -DCMAKE_ARCHIVE_OUTPUT_DIRECTORY_DEBUG:PATH=DebugLibs
# -DCMAKE_ARCHIVE_OUTPUT_DIRECTORY_RELEASE:PATH=ReleaseLibs
# -Dgtest_force_shared_crt=ON
CMAKE_ARGS -G ${CMAKE_GENERATOR}
CONFIGURE_COMMAND ${config_cmd} -G ${CMAKE_GENERATOR}
-S ${CMAKE_BINARY_DIR}/unittests/googletest-prefix/src/googletest/
-B ${CMAKE_BINARY_DIR}/unittests/googletest-prefix/src/googletest-build/
-DCMAKE_BUILD_TYPE=$<CONFIG>
-DCMAKE_C_COMPILER=${CMAKE_C_COMPILER}
-DCMAKE_C_FLAGS=${CMAKE_C_FLAGS}
Expand All @@ -40,6 +49,7 @@ ExternalProject_Add(
-DCMAKE_AR=${CMAKE_AR}
-DCMAKE_INSTALL_PREFIX=${CMAKE_INSTALL_PREFIX}
${EXTRA_GTEST_OPTS}
BUILD_COMMAND ${build_cmd}
# Disable install step
INSTALL_COMMAND ""
BUILD_BYPRODUCTS ${_gtest_byproducts}
Expand Down
13 changes: 10 additions & 3 deletions docs/Emscripten-build-instructions.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,13 @@ This can be installed by executing (we only currently support version
git clone https://github.com/emscripten-core/emsdk.git
./emsdk/emsdk install 3.1.73

and activate the emsdk environment
and activate the emsdk environment (we are defining SYSROOT_PATH for use later)

.. code:: bash

./emsdk/emsdk activate 3.1.73
source ./emsdk/emsdk_env.sh
export SYSROOT_PATH=$PWD/emsdk/upstream/emscripten/cache/sysroot

Now clone the 19.x release of the LLVM project repository and CppInterOp
(the building of the emscripten version of llvm can be avoided by
Expand Down Expand Up @@ -115,7 +116,7 @@ You will also want to set a few environment variables
export CMAKE_PREFIX_PATH=$PREFIX
export CMAKE_SYSTEM_PREFIX_PATH=$PREFIX

Now to build CppInterOp execute the following
Now to build and test your Emscripten build of CppInterOp by executing the following

.. code:: bash

Expand All @@ -128,7 +129,14 @@ Now to build CppInterOp execute the following
-DBUILD_SHARED_LIBS=ON \
-DCMAKE_FIND_ROOT_PATH_MODE_PACKAGE=ON \
-DCMAKE_INSTALL_PREFIX=$PREFIX \
-DSYSROOT_PATH=$SYSROOT_PATH \
../
emmake make -j $(nproc --all) check-cppinterop

Assuming it passes all test you can install by executing the following.

.. code:: bash

emmake make -j $(nproc --all) install

Once this finishes building we need to take note of where we built
Expand All @@ -147,7 +155,6 @@ build folder, you can build the wasm version of xeus-cpp by executing
.. code:: bash

cd ../..
export SYSROOT_PATH=$PWD/emsdk/upstream/emscripten/cache/sysroot
git clone --depth=1 https://github.com/compiler-research/xeus-cpp.git
cd ./xeus-cpp
mkdir build
Expand Down
8 changes: 4 additions & 4 deletions include/clang/Interpreter/CppInterOp.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,12 +563,12 @@ namespace Cpp {
TCppIndex_t param_index);

///\returns arity of the operator or kNone
OperatorArity GetOperatorArity(TCppFunction_t op);
CPPINTEROP_API OperatorArity GetOperatorArity(TCppFunction_t op);

///\returns list of operator overloads
void GetOperator(TCppScope_t scope, Operator op,
std::vector<TCppFunction_t>& operators,
OperatorArity kind = kBoth);
CPPINTEROP_API void GetOperator(TCppScope_t scope, Operator op,
std::vector<TCppFunction_t>& operators,
OperatorArity kind = kBoth);

/// Creates an instance of the interpreter we need for the various interop
/// services.
Expand Down
69 changes: 38 additions & 31 deletions lib/Interpreter/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,33 +1,7 @@
if(EMSCRIPTEN)
set_property(GLOBAL PROPERTY TARGET_SUPPORTS_SHARED_LIBS TRUE)
set(CMAKE_SHARED_LIBRARY_CREATE_C_FLAGS "-s SIDE_MODULE=1")
set(CMAKE_SHARED_LIBRARY_CREATE_CXX_FLAGS "-s SIDE_MODULE=1")
set(CMAKE_STRIP FALSE)

add_llvm_library(clangCppInterOp
SHARED

CppInterOp.cpp
CXCppInterOp.cpp
DynamicLibraryManager.cpp
DynamicLibraryManagerSymbol.cpp
Paths.cpp

# Additional libraries from Clang and LLD
LINK_LIBS
clangInterpreter
)
#FIXME: Setting no_soname=1 is needed until https://github.com/emscripten-core/emscripten/blob/ac676d5e437525d15df5fd46bc2c208ec6d376a3/cmake/Modules/Platform/Emscripten.cmake#L36
# is patched out of emsdk, as --soname is not recognised by emscripten. A PR to do this has been done here https://github.com/emscripten-core/emscripten/pull/23453
#FIXME: A patch is needed to llvm to remove -Wl,-z,defs since it is now recognised on emscripten. What needs to be removed is here
# https://github.com/llvm/llvm-project/blob/128e2e446e90c3b1827cfc7d4d19e3c0976beff3/llvm/cmake/modules/HandleLLVMOptions.cmake#L318 . The PR to do try to do this is here
# https://github.com/llvm/llvm-project/pull/123396
set_target_properties(clangCppInterOp
PROPERTIES NO_SONAME 1
)
target_link_options(clangCppInterOp PRIVATE
PUBLIC "SHELL: -s WASM_BIGINT"
)
set(LLVM_LINK_COMPONENTS "")
else()
set(LLVM_LINK_COMPONENTS
${LLVM_TARGETS_TO_BUILD}
Expand All @@ -44,7 +18,7 @@ else()
if ("LLVMOrcDebugging" IN_LIST LLVM_AVAILABLE_LIBS)
list(APPEND LLVM_LINK_COMPONENTS OrcDebugging)
endif()

endif()
set(DLM
DynamicLibraryManager.cpp
DynamicLibraryManagerSymbol.cpp
Expand All @@ -65,6 +39,11 @@ else()
set(cling_clang_interp clangInterpreter)
endif()

if(EMSCRIPTEN)
set(link_libs
${cling_clang_interp}
)
else()
set(link_libs
${cling_clang_interp}
clangAST
Expand All @@ -73,8 +52,9 @@ else()
clangLex
clangSema
)
endif()

if(NOT WIN32)
if(NOT WIN32 AND NOT EMSCRIPTEN)
list(APPEND link_libs dl)
endif()

Expand Down Expand Up @@ -124,7 +104,6 @@ else()
clangStaticAnalyzerCore
)
endif(LLVM_LINK_LLVM_DYLIB)

add_llvm_library(clangCppInterOp
DISABLE_LLVM_LINK_LLVM_DYLIB
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what role this plays for plays for the emscripten build. I remember as we were forcing a shared build ... we were using

  add_llvm_library(clangCppInterOp
    SHARED
    ......

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you elaborate more on what change you want I may be willing to implement, but being honest I couldn't work out what you was suggesting from your comment.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wanted to make sure DISABLE_LLVM_LINK_LLVM_DYLIB plays a part for the emscripten build as we weren't using it earlier. If you're confident we need it, i'm happy

CppInterOp.cpp
Expand All @@ -134,9 +113,37 @@ else()
${link_libs}
)

target_compile_definitions(clangCppInterOp PUBLIC "_CINDEX_LIB_") # workaround for the use of `CINDEX_LINKAGE`

if(EMSCRIPTEN)
# FIXME: When dynamically linking the Emscripten shared library to the
# unit tests main_module you get errors due to undefined symbols. The reading of the file
# below into a SYMBOLS_LIST variable is a temporary workaround that exports the undefined
# symbols from the shared library, until it can be determined why they are not being exported already.
file(READ "${CMAKE_SOURCE_DIR}/lib/Interpreter/exports.ld" SYMBOLS_LIST)

# Replace newlines with spaces
string(REPLACE "\n" " " SYMBOLS_LIST "${SYMBOLS_LIST}")

#FIXME: Setting no_soname=1 is needed until https://github.com/emscripten-core/emscripten/blob/ac676d5e437525d15df5fd46bc2c208ec6d376a3/cmake/Modules/Platform/Emscripten.cmake#L36
# is patched out of emsdk, as --soname is not recognised by emscripten. A PR to do this has been done here https://github.com/emscripten-core/emscripten/pull/23453
#FIXME: A patch is needed to llvm to remove -Wl,-z,defs since it is now recognised on emscripten. What needs to be removed is here
# https://github.com/llvm/llvm-project/blob/128e2e446e90c3b1827cfc7d4d19e3c0976beff3/llvm/cmake/modules/HandleLLVMOptions.cmake#L318 . The PR to do try to do this is here
# https://github.com/llvm/llvm-project/pull/123396
set_target_properties(clangCppInterOp PROPERTIES
NO_SONAME 1
LINK_FLAGS "-s WASM_BIGINT -s SIDE_MODULE=1 ${SYMBOLS_LIST}"
)
if (CPPINTEROP_ENABLE_TESTING)
# When compiling Emscripten tests the shared library it links to is expected to be in the same folder as the compiled Javascript
add_custom_command(TARGET clangCppInterOp POST_BUILD
COMMAND ${CMAKE_COMMAND} -E copy $<TARGET_FILE:clangCppInterOp> ${CMAKE_BINARY_DIR}/unittests/CppInterOp/
)
endif(CPPINTEROP_ENABLE_TESTING)

endif()

target_compile_definitions(clangCppInterOp PUBLIC "_CINDEX_LIB_") # workaround for the use of `CINDEX_LINKAGE`

string(REPLACE ";" "\;" _VER CPPINTEROP_VERSION)
set_source_files_properties(CppInterOp.cpp PROPERTIES COMPILE_DEFINITIONS
"LLVM_BINARY_DIR=\"${LLVM_BINARY_DIR}\";CPPINTEROP_VERSION=\"${_VAR}\""
Expand Down
Loading
Loading