-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Fixes for building on MSVC #1649
Conversation
@paulharris, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ligfx, @jzakrzewski and @billhoffman to be potential reviewers. |
Note that postfixes on the binaries are required if you use MSVC to "install" it to a folder. The headers are all identical, but the binaries differ (of course). |
The perl part is already worked on separately in #1600 |
option(BUILD_RELEASE_DEBUG_DIRS "Set OFF to build each configuration to a separate directory" OFF) | ||
mark_as_advanced(BUILD_RELEASE_DEBUG_DIRS) | ||
endif() | ||
|
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 don't understand. Why is this section is removed?
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.
Because I could not see anywhere that the flag was used.
Its not a standard CMake flag (as far as I know), and its not mentioned anywhere else in the Curl code.
DEBUG_POSTFIX "-d" | ||
# Note: no postfix for release variants, let user choose what style of release he wants | ||
# MINSIZEREL_POSTFIX "-z" | ||
# RELWITHDEBINFO_POSTFIX "-g" |
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.
What's MINSIZEREL_POSTFIX
and RELWITHDEBINFO_POSTFIX
? I don't think I understand this comment.
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'll give you some background first, and then my opinion.
Many libraries do it the way I've done it, but it took me a while to learn why.
Here is my knowledge dump for you.
Background
Variants
CMake has multiple build variants, they differ whether they have:
- NDEBUG defined (ie asserts enabled)
- What optimisations used (eg for GCC, its -O3 for speed or -Os for size)
- Whether symbols are stripped or not
The variants are:
- Debug (without NDEBUG defined, no optimisations, symbols kept)
- Release (NDEBUG defined, optimise for speed, symbols stripped)
- MinSizeRel (ie Minimum Size Release - NDEBUG defined, optimise for size, symbols stripped)
- RelWithDebInfo (ie Release with Debug Info - NDEBUG defined, optimise for speed, symbols kept)
Note that in MSVC, it is handy to use RelWithDebInfo by default because the .dlls and .exes do NOT have the symbols embedded. They are kept in .pdb files and are not shipped, so program users do not have access to all your symbols, but, you CAN still get a core dump and inspect the backtraces (although with limited success because optimisations can shorten the code paths a lot).
The results (for libcurl) will be a curl library, and a curl exe.
You want to name the debug curl library differently to a release curl library. It is important that it is not mistaken for a release library.
For the 3 release variants, it doesn't matter which one is linked into another exe, they are interchangeable.
The reasons why are explained below in the postscript.
Build Folders
In Linux-CMake, you can only build 1 of those variants in a build folder.
You can reconfigure the build dir to choose a different variant, the result can be left-over binaries left lying around and ignored, so you typically try not to do that because you may end up linking to last week's library.
In Windows-CMake, you do not choose the variant when configuring the build folder.
Instead, you can switch between the variants in MSVC when building.
It doesn't make sense to have separate build directories in Windows because there is a lot of extra context in the MSVC Project that you will lose when you switch back and forth between Release and Debug.
CMake will automatically keep all of the intermediate build files and binaries in separate subdirs within the build dir, until it comes time to install.
Then, typically, all the files are thrown into the same folders (include, lib, bin).
So instead of overwriting the final DLL/EXE binaries, you use different postfixes on the binaries and they live side-by-side.
eg zlib has zlib.dll and zlibd.dll
Opinion: which postfix should be used
I have chosen to ONLY use a postfix on the debug variant, because, for me, my other projects will only be linking to either the debug or one of the release variants.
IF I want to go for small binary sizes, then I'll be building each of the libraries and binaries in the MinSizeRel.
IF I want just one library to be very fast, then that one library will be build with Release or RelWithDebInfo.
The release libraries are interchangeable, so no need to add extra complexity by adding postfixes for them.
And that is partly because of the role msvcrt.dll plays. I discuss below.
Postscript: On the subject of linked libraries and msvcrt
I have noticed that libcurl (debug variant) links to zlib.dll.
This is not ideal, and it should really be trying to link to zlibd.dll.
We get away with it in zlib's case (as long as people don't want to use the static msvcrt aka static C runtime), but other sub-libraries could cause real trouble.
In the MSVC world, a dll that uses ANY of the Microsoft functions from its "CRT" aka C-runtime library will link to ONE of the msvcrt.dll libraries. This is the Windows version of libc, and so you'd have to be doing something really specific to avoid using it at all.
ie malloc, strlen, etc are part of it, as well as CreateFileW and other things.
The variants of msvcrt include:
- Release dll
- Debug dll
- Release static lib
- Debug static lib
(static lib - your binary gets big as it sucks in the msvcrt lib into your exe, but now you can deploy the binary on older Windows computers without the users needing to also install the Microsoft MSVC2014 runtime libraries).
When linking an exe with multiple dlls or libraries, you have to be careful to only link to one of those variants. So a debug exe should link to dlls compiled and linked to the debug msvcrt.
MSVC's link.exe will often warn about linking multiple msvcrt variants, I forget if it ever stops the build. I think sometimes it does, which is where the real hair-pulling starts, because it can be hard to tell which library is linked to the 'other' msvcrt.
I think MSVC refuses to link if you have release and debug static msvcrt libs mixed in.
One reason you don't want to link to multiple msvcrts is because there is state contained within the msvcrt. I can't give a specific example off the top of my head, but I'll use the example of errno. Its a global variable set when things go wrong. EACH msvcrt.dll has a different errno.
So you can imagine there is all sorts of hidden global state in msvcrt - a recipe for chaos.
Another example of why to avoid mixing debug/release, and this goes for Linux too...
This is a C++ example, but may apply to C as well:
std::vector<> and other templates are very different when comparing debug and release builds, because of the debug features that NDEBUG cuts out.
It doesn't happen as much in the Linux world, but Microsoft added a lot of iterator validity checks into its STL library which changes the size of a vector<> struct. So you can imagine the chaos if you pass a malloc'd debug-vector to some code that expects a release-vector... many of the memory offsets would be off by X bytes.
So that is another reason why you don't want to mix debug and release binaries.
zlib is a simple library, and surely won't hit any of these problems (we hope), but I have had one experience where I needed to use the "static release msvcrt library", ie everything was compiled into one exe, no dlls. In that case, you really cannot mix msvcrt libraries anywhere at all.
CMakeLists.txt
Outdated
if(ENABLE_MANUAL OR ENABLE_DOCS OR BUILD_TESTING) | ||
# Required for building manual, docs, tests | ||
find_package(Perl REQUIRED) | ||
endif() |
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.
This is the perl related section already being discussed and work on in #1600. Please avoid that in your 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.
Ok
I assume I am supposed to adjust the pull like so... |
I personally think fixing up the commits and force-pushing them (in pull-requests) is still better so that we get to see the exact set of commits you want to merge. |
Thanks! |
FWIW pycurl windows build process goes through some effort to get libcurl and all of its dependencies linked to the same & correct msvcrt (which in turn is dictated by python). libcurl -> zlib: https://github.com/pycurl/pycurl/blob/7dee7311ecc4cc0ec42588e3d5c3ad9c97d1582d/winbuild/libcurl-fix-zlib-references.patch#L17 For example, even though pycurl links in libcurl, zlib, openssl etc. statically, all of them are built to reference msvcrt dll as that's what python uses. |
@p: I don't understand what your comment on this old closed PR means. Are you suggesting these patches should be merged? If so, could you please submit them as new PR(s) ? |
Just providing some background info in regards to msvcrt-related remarks by @paulharris above. No action is needed. As far as merging the patch(es) they would require a lot more investigation and pondering from my side at least to do so. |
I had to make these changes to allow cmake and MSVC to build libcurl the typical way I would expect - ie with postfixes.
And, I had to avoid finding the required Perl package, which seems to be only required for docs (and testing).