-
-
Notifications
You must be signed in to change notification settings - Fork 570
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
Remove "-rdynamic" flag for static builds #300
Conversation
The CI pipeline shows this fails to build with MSVC due to the number of arguments used with the REGEX command. I have no experience with C development on Windows, and I'm a CMake novice, so I'm not sure how I should address the build failure. |
I'm not a cmake expert either, but I'm sure this isn't the right approach to fix this problem. (If there is a problem: it would be good to have a fuller account of the problem, so we can confirm it.) Probably if something needs to be changed, it's in |
I don't know if this makes a difference, but from what I saw when using CMake with verbosity/debugging, "-rdynamic" is added in based on rules in my systems global CMake configuration files, not something in the cmark build rules.
In the text below, I've gone through the process of generating the faulty build by generating the musl compiler wrapper (musl-gcc) and using the wrapper to compile cmark: Extract musl and generate musl-gcc
Prepare the cmark build
Create the executable
Test the executable
This shows the embedded shared object path which shouldn't be there.
|
@nwellnhof do you have any insight on this issue? |
Seems like a CMake issue. The option probably comes from here: https://github.com/Kitware/CMake/blob/07cfb18f9d29cfc0588ede928846a03ec5599c48/Modules/Platform/Linux-GNU.cmake Maybe setting |
That doesn't make a difference for me:
|
At the time of this writing, it seems that CMake only uses "-rdynamic" on Linux: ``` $ git clone https://github.com/Kitware/CMake.git Cloning into 'CMake'... remote: Enumerating objects: 335, done. remote: Counting objects: 100% (335/335), done. remote: Compressing objects: 100% (254/254), done. remote: Total 272793 (delta 86), reused 125 (delta 79), pack-reused 272458 Receiving objects: 100% (272793/272793), 88.11 MiB | 29.21 MiB/s, done. Resolving deltas: 100% (207174/207174), done. $ cd CMake/Modules/ Modules$ fgrep -r -w rdynamic . ./Platform/Linux-TinyCC-C.cmake:set(CMAKE_EXE_EXPORTS_C_FLAG "-rdynamic ") ./Platform/Linux-NAG-Fortran.cmake:set(CMAKE_SHARED_LIBRARY_LINK_Fortran_FLAGS "-Wl,-rdynamic") ./Platform/Linux-Intel.cmake: set(CMAKE_SHARED_LIBRARY_LINK_${lang}_FLAGS "-rdynamic") ./Platform/Linux-GNU.cmake: set(CMAKE_SHARED_LIBRARY_LINK_${lang}_FLAGS "-rdynamic") ``` So the conditional has been modified accordingly.
Fix typo by changing "STATIC" to "CMARK_STATIC".
The only place I see mention of "rdynamic" in the CMake source is in the modules for Linux, so I've updated the conditional in my patch accordingly, and the build now succeeds on MSVC. |
LINUX is not a CMake platform variable, so the expression has been updated accordingly and the build functionality verified.
- At the time of this writing, "-rdynamic" is only added when using CMake on Linux, so the conditional expression used for patching cmark's build system has been updated to specifically check for Linux. Thanks for Nick Wellnhofer (nwellnhof) for pointing this out in commonmark/cmark#300.
- At the time of this writing, "-rdynamic" is only added when using CMake on Linux, so the conditional expression used for patching cmark's build system has been updated to specifically check for Linux. Thanks to Nick Wellnhofer (nwellnhof) for pointing this out in commonmark/cmark#300.
I think you have to put the final |
I found this post which recommends simply
That should be safe for a static build, right? And then we don't need to worry about regexes. |
That works. I've updated pull request. |
Should we also now remove the check for Linux? If I recall, it was only necessary because of the regex stuff. |
I would argue that it should be scoped to Linux to minimize the chances of collateral damage because that's the only platform where this problem is known to exist since, when I grepped for "rdynamic" in the CMake repositories, the only matches were in the Linux modules directory. However, I'll do whatever's requested to get this merged as long as it fixes the problem for me |
Your argument is reasonable. I don't feel strongly about it either way. If you want to revert the last change, that's fine, I'll merge it either way! |
Thanks, done. |
Thanks! |
- Update cmark to pull in commonmark/cmark#300 which includes a fix to ensure "-rdynamic" is not used for statically linked builds.
I ran into problems trying to build the statically linked cmark executable using musl libc that was caused by the "-rdynamic" flag being implicitly added to the build command. The resulting executable would had references to a musl libc shared object instead of being hermetic as expected. I'm not sure if this has any unintended consequences, but I at least have no problems building a dynamically linked executable with this patch applied on my Linux and macOS machines.