Why overwrite CMAKE_MSVC_RUNTIME_LIBRARY #1699
Comments
|
yeah, I made this, trying to match the scons solution in terms of options, but more things are coming out of the woodwork that tells me it wasnt the best solution to the problem and its probably best I leave that upto consumers entirely. Can I please get your feedback for an ideal situation? how woud you like linking to the msvc managed? |
|
In the cmake/windows.cmake file there is a description at the top as to why. Just thinking out loud here: The crux to the situation is that the MSVC_RUNTIME_LIBRARY target property isn't transient, meaning it does not flow to consuming libraries, so whatever is in the CMAKE_MSVC_RUNTIME_LIBRARY gets added to targets, and that by default conflicts with the build flags. The only solution I have is to clobber the CMAKE_MSVC_RUNTIME_LIBRARY so in the simple case targets that rely on godot-cpp will have the right variables set. More advanced users like yourself can reset the contents of CMAKE_MSVC_RUNTIME_LIBRARY before your targets are defined that need something else. Looking over the usecase and code again I can't see an alternative solution. |
|
@Naros this issue might interest you as you also had questions about the MSVC runtime. |
|
@enetheru We can just set I believe, this way you don't have to propagate these flags into the parent project, or even set them in the first place with I strongly suggest looking at other open-source projects to get an idea how it's usually done. And I don't mean to say that you didn't. One of the examples is GLFW: https://github.com/glfw/glfw/blob/master/CMakeLists.txt #--------------------------------------------------------------------
# Apply Microsoft C runtime library option
# This is here because it also applies to tests and examples
#--------------------------------------------------------------------
if (MSVC AND NOT USE_MSVC_RUNTIME_LIBRARY_DLL)
set(CMAKE_MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>")
endif() |
|
This amounts to the same thing no? We would still be overwriting the value of CMAKE_MSVC_RUNTIME_LIBRARY, so it would be set when godot-cpp is included as an submodule either through fetch make available. Is it the cached part that is the problem? I can make it available at the top level scope without caching by using PARENT_SCOPE. but only in its current location. set( CMAKE_MSVC_RUNTIME_LIBRARY "$<1:>" PARENT_SCOPE)It needs to be set before the targets are defined, that includes godot-cpp and any dependent targets. It's just so darn hard to actually use because it doesnt propogate to consumers of the library, where just using the flags does. in both cases, we would be overwriting the value of CMAKE_MSVC_RUNTIME_LIBRARY and anybody who needs a separate value needs to change that. |
|
My only issue with this is the warning I see inside CLion: If the build directories do not exist yet, this does not happen but if they do it does repeatedly. This leads to CLion indicating that there is a warning that needs to be reviewed as shown here:
As someone who has worked in other projects that allow this sort of stuff to manifest, it desensitizes people's awareness and it just becomes the defacto, and ultimately leads to the potential for other warnings or issues to happen that won't be spotted or recognized because you assume it's the above warning when it could also be accompanied by something else. For me, that's why I always look for this, which tells me the reload was successful.
|
|
@enetheru I should point out that the CMAKE command ran by CLion is this: E:\Jetbrains\CLion\bin\cmake\win\x64\bin\cmake.exe -DCMAKE_BUILD_TYPE=Release -DCMAKE_MAKE_PROGRAM=E:/Jetbrains/CLion/bin/ninja/win/x64/ninja.exe -DEXTENDED_TOOLTIPS=TRUE -DCMAKE_CXX_COMPILER_LAUNCHER=ccache -DCMAKE_C_COMPILER_LAUNCHER=ccache -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -G Ninja -S E:\GitHub\godot-orchestrator -B E:\GitHub\godot-orchestrator\cmake-build-releaseI'm not sure whether any of that makes a bit of difference. I suspect not, but thought I'd share it for completeness. |
|
@enetheru Let me correct myself: yes, we'd still be overwriting it, but we'd be using an actually proper value. Imagine scenario set(CMAKE_MSVC_RUNTIME_LIBRARY) # This could be set via an argument to the cmake command
add_subdirectory(godot-cpp) # Sets CMAKE_MSVC_RUNTIME_LIBRARY to empty.
add_subdirectory(somelib) # Adds a target somelib (external lib, not our code, so we can't change it).
# This project doesn't set any runtime flags and doesn't link to godot-cpp,
# and relies purely on the CMAKE_MSVC_RUNTIME_LIBRARY variable.
# As a result, because godot-cpp has overwritten CMAKE_MSVC_RUNTIME_LIBRARY,
# the somelib target will link to whatever is default in CMake (which is /MD).
add_library(ourgdext MODULE)
target_link_libraries(ourgdext godot-cpp somelib) # This will result in conflicts, because
# godot-cpp may be /MT and somelib is /MDTo solve this, I suggest setting |
|
My original statement of why overwrite IMHO, ideally, all projects should expose an option for setting static/dynamic runtime and not rely on the |
|
I get it, took going through your description line by line, attempting to correct mistakes, for me to see it. This paraphrased/re-arranged line is the crux of it I believe?
Which is very enlightening thank you. Looks like I have a bunch of testing to do. |
|
@enetheru Sounds about right. I think I'd suggest that godot-cpp should only care about its own sub-directories (sub-project) and not force specific |
I wish it were that simple, the thing is that we have to match the actual godot executable, if the exe and our dll dont match i believe we have problems. So the I'm working on something that might work. i'll submit a PR soon and see if it satisfies. |
|
Godot executable is compiled completely separately, I don't think it has to do anything with the runtime of godot-cpp. We compile a dll and statically link to godot-cpp. This is complete dll, everything within it must use the same runtime flag. What happens next doesn't matter. I believe this is how it works. But yes, let's see what you'll come up with. |
Previously I eschewed the use of CMAKE_MSVC_RUNTIME_LIBRARY in favour of setting the flags using target_compile_options so that they would propagate to consumers. However, it has been raised that a dependency( independent to godot-cpp ) that doesn't set any runtime flags, which relies purely on the CMAKE_MSVC_RUNTIME_LIBRARY variable will very likely not have the correct msvc runtime flags set. Where MSVC documentation states "All modules passed to a given invocation of the linker must have been compiled with the same runtime library compiler option (/MD, /MT, /LD)." It was also mentioned that messaging as a warning that we are setting the option was not ideal. So I have updated the cmake code to use CMAKE_MSVC_RUNTIME_LIBRARY over target-compile_options. And set it as a CACHE STRING variable so that it can be overridden if desired. We still message consumers, but as a NOTICE. While performing this work I noticed that the various options code being inside functions was unnecessary, and less flexible.
|
Try this PR, it still sets as a cache variable, but at least it doesnt clobber it like it did before. The only code thats really changed is in windows.cmake |
Previously I eschewed the use of CMAKE_MSVC_RUNTIME_LIBRARY in favour of setting the flags using target_compile_options so that they would propagate to consumers. However, it has been raised that a dependency( independent to godot-cpp ) that doesn't set any runtime flags, which relies purely on the CMAKE_MSVC_RUNTIME_LIBRARY variable will very likely not have the correct msvc runtime flags set. Where MSVC documentation states "All modules passed to a given invocation of the linker must have been compiled with the same runtime library compiler option (/MD, /MT, /LD)." It was also mentioned that message type WARNING is not ideal for notifying consumers. So I have updated the cmake code to use CMAKE_MSVC_RUNTIME_LIBRARY over target-compile_options. And set it as a CACHE STRING variable so that it can be overridden if desired. We still message consumers, but as a NOTICE. While performing this work I noticed that the various options code being inside functions was unnecessary, and less flexible.
Previously I eschewed the use of CMAKE_MSVC_RUNTIME_LIBRARY in favour of setting the flags using target_compile_options so that they would propagate to consumers. However, it has been raised that a dependency( independent to godot-cpp ) that doesn't set any runtime flags, which relies purely on the CMAKE_MSVC_RUNTIME_LIBRARY variable will very likely not have the correct msvc runtime flags set. Where MSVC documentation states "All modules passed to a given invocation of the linker must have been compiled with the same runtime library compiler option (/MD, /MT, /LD)." It was also mentioned that message type WARNING is not ideal for notifying consumers. So I have updated the cmake code to use CMAKE_MSVC_RUNTIME_LIBRARY over target-compile_options. And set it as a CACHE STRING variable so that it can be overridden if desired. We still message consumers, but as a NOTICE.
|
I cleaned up the commit to remove all the extra stuff, it can wait for a cleanup PR. |
|
@enetheru Your changes work very well! A few notes:
|
Thank you very much 😄 I Appreciate you lodging this in the first place and the ongoing interactions here, it makes validating the work so much easier.
I changed back to
While writing i noticed that the order of how things are included meant that the options functions were unnecessary abstraction that just got in the way, so I plan a follow up that removes them all entirely. Since I'm using the cache variable again I will put it back.
I did initally think a normal variable was better, but then thought how annoying it might get being unable to override it, or having it only able to propagate up one level from
If you can figure out some if condition that can detect its way through generator expressions then I can add it, but I cant think of one, so a simple STATUS message might be all that gets made. |
Previously I eschewed the use of CMAKE_MSVC_RUNTIME_LIBRARY in favour of setting the flags using target_compile_options so that they would propagate to consumers. However, it has been raised that a dependency( independent to godot-cpp ) that doesn't set any runtime flags, which relies purely on the CMAKE_MSVC_RUNTIME_LIBRARY variable will very likely not have the correct msvc runtime flags set. Where MSVC documentation states "All modules passed to a given invocation of the linker must have been compiled with the same runtime library compiler option (/MD, /MT, /LD)." It was also mentioned that message type WARNING is not ideal for notifying consumers. So I have updated the cmake code to use CMAKE_MSVC_RUNTIME_LIBRARY over target-compile_options. And set it as a CACHE STRING variable so that it can be overridden if desired. We still message consumers, but as a NOTICE.
|
You're right, the generator expressions ^^ I guess, this could be documented somehow in the getting-started docs. Then I would go back to how it was, but put the whole thing into if(NOT CMAKE_MSVC_RUNTIME_LIBRARY)
message(NOTICE "Setting CMAKE_MSVC_RUNTIME_LIBRARY. For more information please read godot-cpp/cmake/windows.cmake")
set(CMAKE_MSVC_RUNTIME_LIBRARY
"MultiThreaded$<IF:$<BOOL:${GODOT_DEBUG_CRT}>,DebugDLL,$<$<NOT:$<BOOL:${GODOT_USE_STATIC_CPP}>>:DLL>>"
CACHE STRING "Select the MSVC runtime library for use by compilers targeting the MSVC ABI.")
endif() |
I think that no matter how its sliced, messaging the user during configure with something, and pointing them to the windows.cmake to discover more information is best. So the |
Previously I eschewed the use of CMAKE_MSVC_RUNTIME_LIBRARY in favour of setting the flags using target_compile_options so that they would propagate to consumers. However, it has been raised that a dependency( independent to godot-cpp ) that doesn't set any runtime flags, which relies purely on the CMAKE_MSVC_RUNTIME_LIBRARY variable will very likely not have the correct msvc runtime flags set. Where MSVC documentation states "All modules passed to a given invocation of the linker must have been compiled with the same runtime library compiler option (/MD, /MT, /LD)." It was also mentioned that message type WARNING is not ideal for notifying consumers. So I have updated the cmake code to use CMAKE_MSVC_RUNTIME_LIBRARY over target-compile_options. And set it as a CACHE STRING variable so that it can be overridden if desired. We still message consumers, but as a NOTICE.
Previously I eschewed the use of CMAKE_MSVC_RUNTIME_LIBRARY in favour of setting the flags using target_compile_options so that they would propagate to consumers. However, it has been raised that a dependency( independent to godot-cpp ) that doesn't set any runtime flags, which relies purely on the CMAKE_MSVC_RUNTIME_LIBRARY variable will very likely not have the correct msvc runtime flags set. Where MSVC documentation states "All modules passed to a given invocation of the linker must have been compiled with the same runtime library compiler option (/MD, /MT, /LD)." It was also mentioned that message type WARNING is not ideal for notifying consumers. So I have updated the cmake code to use CMAKE_MSVC_RUNTIME_LIBRARY over target-compile_options. And set it as a CACHE STRING variable so that it can be overridden if desired. We still message consumers, but as a NOTICE.
CMake: Fix for #1699 msvc runtime selection issues
Godot version
N/A
godot-cpp version
master
System information
N/A
Issue description
I've noticed there has been recently some acitivity regarding MSVC runtime in CMake. With the latest changes I see that windows.cmake overwrites CMAKE_MSVC_RUNTIME_LIBRARY to empty. It feels quite intrusive A project may be compiling other things separate from the app (like dlls or other exes) that rely on this variable.
Steps to reproduce
N/A
Minimal reproduction project
N/A
The text was updated successfully, but these errors were encountered: