Added fixes which caused issues building on Apple Silicon #170
Conversation
| @@ -6,6 +6,13 @@ get_filename_component(CPPINTEROP_INSTALL_PREFIX "${CPPINTEROP_INSTALL_PREFIX}" | |||
| get_filename_component(CPPINTEROP_INSTALL_PREFIX "${CPPINTEROP_INSTALL_PREFIX}" PATH) | |||
| get_filename_component(CPPINTEROP_INSTALL_PREFIX "${CPPINTEROP_INSTALL_PREFIX}" PATH) | |||
|
|
|||
| # Determine CMAKE_SHARED_LIBRARY_SUFFIX based on operating system | |||
| if (${CMAKE_SYSTEM_NAME} MATCHES "Darwin") | |||
| set(CMAKE_SHARED_LIBRARY_SUFFIX ".dylib") | |||
There was a problem hiding this comment.
Are the changes introduced in Is 4eac1ca not sufficient?
There was a problem hiding this comment.
I didn't find it to be sufficient on my machine.
There was a problem hiding this comment.
@mcbarton, there is some discussion about .so vs .dylib: https://gitlab.kitware.com/cmake/cmake/-/issues/21189. They say that macOS distinguishes between shared libraries and loadable modules. CMAKE_SHARED_LIBRARY_SUFFIX is set to .dylib for shared libraries. CMAKE_SHARED_MODULE_SUFFIX is set to .so for loadable modules (e.g. dlopen). See the [add_library](https://cmake.org/cmake/help/v3.18/command/add_library.html#normal-libraries) command's SHARED and MODULE options..
CppInterOp is meant to be dlopened thus it means that we should keep probably the .so extension. We can do that by adding the MODULE option to https://github.com/compiler-research/CppInterOp/blob/4eac1caa49d86ce9e51da1d46bfe38675f57444c/lib/Interpreter/CMakeLists.txt#L87C1-L93
There was a problem hiding this comment.
@vgvassilev I tried your suggestion and it didn't work on my machine. Adding the MODULE option caused the following to occur
- The following error is produced during cmake configure stage
CMake Error at unittests/CppInterOp/CMakeLists.txt:16 (target_link_libraries):
Target "clangCppInterOp" of type MODULE_LIBRARY may not be linked into
another target. One may link only to INTERFACE, OBJECT, STATIC or SHARED
libraries, or to executables with the ENABLE_EXPORTS property set.
CMake Error at unittests/CppInterOp/CMakeLists.txt:26 (target_link_libraries):
Target "clangCppInterOp" of type MODULE_LIBRARY may not be linked into
another target. One may link only to INTERFACE, OBJECT, STATIC or SHARED
libraries, or to executables with the ENABLE_EXPORTS property set.
-
dylib extension stayed (determined by turning off building of unittests)
-
libclangCppInterop.dylib got renamed to clangCppInterop.dylib (determined by turning off building of unittests)
Any other suggestions?
There was a problem hiding this comment.
Ok, then we are back to your original patch. The part I do not understand is why then the CMAKE_SHARED_LIBRARY_SUFFIX variable is not set. I'd rather have include(CMakeSystemSpecificInformation) than setting it by hand. I checked on my platform and that does set the right suffix. Can we try that?
There was a problem hiding this comment.
@vgvassilev I just performed a quick check on my system and this new solution works. I'll push this solution to my fork sometime this evening
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #170 +/- ##
=======================================
Coverage 72.75% 72.75%
=======================================
Files 8 8
Lines 2885 2885
=======================================
Hits 2099 2099
Misses 786 786
|
Stopped Cuda library been loaded when on Apple sets CMAKE_SHARED_LIBRARY_SUFFIX without hardcoding
git-clang-format command run. changed file lib/Interpreter/Compatibility.h
|
@vgvassilev I have squashed both commits into one, and then performed the command git-clang-format HEAD~, and committed this. I think you meant me to do this in such a way as to reduce everything to a single commit, but was unsure. Will squash both commits of this pull request into 1 once you confirm. |
|
clang-tidy review says "All clean, LGTM! 👍" |
Now I can do it on my end. |
Stopped Cuda library been loaded when on Apple
Add .dylib option for when on MacOS