[cmake] Modify cmake config file to set correct suffix and prefix for library

[mcbarton]

The purpose of this PR is to find the modifications to CppInterOp needed so that the wasm build of xeus-cpp in PR compiler-research/xeus-cpp#14 can pass.

[mcbarton]

@vgvassilev can you activate the CI?

[vgvassilev]

@vgvassilev can you activate the CI?

No, it does not let me... It is probably either because that PR is a draft of we have a mistake in the CI config.

[mcbarton]

@vgvassilev can you activate the CI?

No, it does not let me... It is probably either because that PR is a draft of we have a mistake in the CI config.

@vgvassilev It was a CI config issue. Once the CI runs, i'll check it for any errors, and afterwards i'll cc you in when it is ready to test possible solutions to xeus-cpp wasm build.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.63%. Comparing base (1334558) to head (a071ee6).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #203   +/-   ##
=======================================
  Coverage   78.63%   78.63%           
=======================================
  Files           8        8           
  Lines        3057     3057           
=======================================
  Hits         2404     2404           
  Misses        653      653           

[mcbarton]

@vgvassilev @alexander-penev This PR is now ready for testing solutions to the issue where xeus-cpp was using the wrong library extension for CppInterOp, during the wasm build of xeus-cpp

[mcbarton]

@vgvassilev Could this be the cause of the issue is with it producing a .a when a .so is expected? If you read the configure stage of
https://github.com/compiler-research/CppInterOp/actions/runs/8254377551/job/22578422472#step:13:2
you'll notice multiple messages along the lines of

CMake Warning (dev) at llvm-project/build/lib/cmake/clang/ClangTargets.cmake:375 (add_library):
  ADD_LIBRARY called with SHARED option but the target platform does not
  support dynamic linking.  Building a STATIC library instead.  This may lead
  to problems.

So despite choosing a shared library, we will end up with a static one if I am reading this correctly.

[vgvassilev]

@vgvassilev Could this be the cause of the issue is with it producing a .a when a .so is expected? If you read the configure stage of https://github.com/compiler-research/CppInterOp/actions/runs/8254377551/job/22578422472#step:13:2 you'll notice multiple messages along the lines of

CMake Warning (dev) at llvm-project/build/lib/cmake/clang/ClangTargets.cmake:375 (add_library):
  ADD_LIBRARY called with SHARED option but the target platform does not
  support dynamic linking.  Building a STATIC library instead.  This may lead
  to problems.

So despite choosing a shared library, we will end up with a static one if I am reading this correctly.

Correct. That is because wasm does not support shared libraries and turns them off. We should just do that preemptively in the build and adjust everything else in the config file.

[mcbarton]

@vgvassilev Could this be the cause of the issue is with it producing a .a when a .so is expected? If you read the configure stage of https://github.com/compiler-research/CppInterOp/actions/runs/8254377551/job/22578422472#step:13:2 you'll notice multiple messages along the lines of

CMake Warning (dev) at llvm-project/build/lib/cmake/clang/ClangTargets.cmake:375 (add_library):
  ADD_LIBRARY called with SHARED option but the target platform does not
  support dynamic linking.  Building a STATIC library instead.  This may lead
  to problems.

So despite choosing a shared library, we will end up with a static one if I am reading this correctly.

Correct. That is because wasm does not support shared libraries and turns them off. We should just do that preemptively in the build and adjust everything else in the config file.

@vgvassilev Is the solution in the config file I have done acceptable to you? It appears to work. If yes, i'll put it in as a patch in the Emscripten forge recipe.

[vgvassilev]

Now the config lgtm. Do we want to merge this pr? Or should we test elsewhere first?

[mcbarton]

@vgvassilev This is the patch I have settled on after your suggestions if your happy with it. I decided to fix the library prefix at the same time as the suffix.

diff --git a/cmake/CppInterOp/CppInterOpConfig.cmake.in b/cmake/CppInterOp/CppInterOpConfig.cmake.in
index 6a2a810..60d4914 100644
--- a/cmake/CppInterOp/CppInterOpConfig.cmake.in
+++ b/cmake/CppInterOp/CppInterOpConfig.cmake.in
@@ -10,19 +10,20 @@ get_filename_component(CPPINTEROP_INSTALL_PREFIX "${CPPINTEROP_INSTALL_PREFIX}"
 include(CMakeSystemSpecificInformation)
 
 ### build/install workaround
+if (BUILD_SHARED_LIBS)
+  set(__lib_suffix ${CMAKE_SHARED_LIBRARY_SUFFIX})
+  set(__lib_prefix ${CMAKE_SHARED_LIBRARY_PREFIX})
+else()    
+  set(__lib_suffix ${CMAKE_STATIC_LIBRARY_SUFFIX})
+  set(__lib_prefix ${CMAKE_STATIC_LIBRARY_PREFIX})
+endif(BUILD_SHARED_LIBS)
 
 if (IS_DIRECTORY "${CPPINTEROP_INSTALL_PREFIX}/include")
   set(_include "${CPPINTEROP_INSTALL_PREFIX}/include")
-  set(_libs "${CPPINTEROP_INSTALL_PREFIX}/lib/${CMAKE_SHARED_LIBRARY_PREFIX}clangCppInterOp${CMAKE_SHARED_LIBRARY_SUFFIX}")
+  set(_libs "${CPPINTEROP_INSTALL_PREFIX}/lib/${__lib_prefix}clangCppInterOp${__lib_suffix}")
 else()
   set(_include "@CMAKE_CURRENT_SOURCE_DIR@/include")
-  set(_libs "@CMAKE_CURRENT_BINARY_DIR@/lib/${CMAKE_SHARED_LIBRARY_PREFIX}clangCppInterOp${CMAKE_SHARED_LIBRARY_SUFFIX}")
-endif()
-
-if (IS_DIRECTORY "${CPPINTEROP_INSTALL_PREFIX}/lib/cmake/CppInterOp")
-  set(_cmake "${CPPINTEROP_INSTALL_PREFIX}/lib/cmake/CppInterOp")
-else()
-  set(_cmake "@CMAKE_CURRENT_SOURCE_DIR@/cmake/CppInterOp")
+  set(_libs "@CMAKE_CURRENT_BINARY_DIR@/lib/${__lib_prefix}clangCppInterOp${__lib_suffix}")
 endif()

[mcbarton]

Now the config lgtm. Do we want to merge this pr? Or should we test elsewhere first?

I would suggest this PR gets merged after compiler-research/xeus-cpp#14 is merged , so I can change

CppInterOp/.github/workflows/ci.yml

Line 1276 in 7a38391

git clone https://github.com/alexander-penev/xeus-cpp.git

to reference the compiler research repo.

I will put in the patch to both the conda-forge and emscripten forge shortly for testing.

[mcbarton]

@vgvassilev Just as I was preparing the patch for CppInterOp the build for xeus-clang-repl build failed. It looks like it needs a little refinement. I will work out what the issue is tommorrow.

[mcbarton]

@vgvassilev The build of xeus-clang-repl fails if I use the if (BUILD_SHARED_LIBS) you suggested, but passes if i use if (@BUILD_SHARED_LIBS@ STREQUAL "ON") . Do you have any objections to me using this line instead? I could add a FIXME next to.

[vgvassilev]

Ah, I see. I'd not hardcode BUILD_SHARED_LIBS to ON. It can be TRUE, 1, etc. Doesn't this work:

Suggested change

	if (@BUILD_SHARED_LIBS@ STREQUAL "ON")
	if (@BUILD_SHARED_LIBS@)

In addition we need to fix this line add_library(clangCppInterOp SHARED IMPORTED) to STATIC conditionally.

[mcbarton]

@vgvassilev would you like to me strip out all the ci stuff in this PR, so the patch can be merged now? It will be easy to add the ci stuff back in once compiler-research/xeus-cpp#14 has been merged.

[vgvassilev]

@vgvassilev would you like to me strip out all the ci stuff in this PR, so the patch can be merged now? It will be easy to add back in once compiler-research/xeus-cpp#14 has been merged.

It would be better because we do not know the eta of that PR.

[vgvassilev]

@vgvassilev would you like to me strip out all the ci stuff in this PR, so the patch can be merged now? It will be easy to add back in once compiler-research/xeus-cpp#14 has been merged.

It would be better because we do not know the eta of that PR.

Let's see if the patch works first through the new package, though...

[mcbarton]

@vgvassilev I have modified the PR, so it is now just the patch, after the patch was seen to work. The PR can now be merged.

[vgvassilev]

LGTM! Thank you, @mcbarton!