-
Notifications
You must be signed in to change notification settings - Fork 23
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
Fix CMake build on macOS #2133
Fix CMake build on macOS #2133
Conversation
CMakeLists.txt
Outdated
@@ -253,6 +253,9 @@ if(WIN32) | |||
add_compile_definitions(WIN32_LEAN_AND_MEAN) | |||
endif() | |||
|
|||
# Ensure dynamic library extension is always .so | |||
set(CMAKE_SHARED_LIBRARY_SUFFIX ".so") |
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.
is this OK for Windows? There is some windows specific code in this file, so I assume we can run the build for Windows using it
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.
I just tried building on Windows using MSYS2, using both the UCRT64 and MINGW64 environments. Both result in Yosys not building (TARGET_PDB_FILE
not supported by linker for UCRT64 and -rdynamic
not supported for MINGW64, probably because CMake can't identify the environment in that latter case). Both errors are unrelated to these changes since it never gets to even build synlig. oss-cad-suite-build
doesn't contain yosys-config.cmake
which this CMakeLists requires to find Yosys, so I'm not sure how to use a pre-built version of Yosys. I'm going to call it quits after spending a couple hours trying to get this to build, so if someone else has a working Windows build environment, go for it.
Regardless, I made this change since it appears Yosys looks for .so files regardless of platform.
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.
thanks for checking this.
I triggered the CI, once it's green I'll merge the PR
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.
I just noticed the following change would fit in better with the existing line above it. I'm testing a build rn if you'd prefer it this way.
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -253,9 +253,6 @@ if(WIN32)
add_compile_definitions(WIN32_LEAN_AND_MEAN)
endif()
-# Ensure dynamic library extension is always .so
-set(CMAKE_SHARED_LIBRARY_SUFFIX ".so")
-
# Put source code here, files that are generated at build time in
# surelog_generated_SRC
set(synlig_SRC
@@ -293,6 +290,7 @@ set_target_properties(
synlig PROPERTIES SOVERSION "${SYNLIG_VERSION_MAJOR}.${SYNLIG_VERSION_MINOR}")
set_target_properties(synlig PROPERTIES OUTPUT_NAME "systemverilog")
set_target_properties(synlig PROPERTIES PREFIX "")
+set_target_properties(synlig PROPERTIES SUFFIX ".so")
# Allow undefined symbols at link time on macOS. These symbols should already be
# present within yosys when the plugin is loaded.
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.
makes sense, the code will be cleaner. Please push it and I'll retrigger the CI
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.
OK, done. Still works on my Mac.
e5bfc05
to
6090723
Compare
The new CMake-based build system almost works on macOS, but fails at the final step when linking the plugin. This is because the default behavior on macOS (for both clang and GCC) is to require all symbols to be defined at link time for a dynamic library. This is not the case for synlig, so it fails to link.
Failing linker output
This PR adds the
-undefined dynamic_lookup
flag when built under macOS to allow undefined symbols at link time, similar to the behavior under Linux.Additionally, yosys looks for plugins with a
.so
extension, so this is enforced instead of the macOS default of.dylib
.Works on macOS 14.1 with clang 15 and CMake 3.27.7