Skip to content
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

Closed
wants to merge 2 commits into from
Closed

Conversation

paulharris
Copy link

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).

@mention-bot
Copy link

@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.

@paulharris
Copy link
Author

Note that postfixes on the binaries are required if you use MSVC to "install" it to a folder.
I typically ask cmake to install to the build directory,
eg c:\dev\build_curl\install
And both debug+release libs get installed there for my other projects to find and use.

The headers are all identical, but the binaries differ (of course).
So the suffixes/postfixes (ie libcurl-d.dll) allow the binaries to be installed to the same lib or bin folder.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 74.396% when pulling 793b28c on paulharris:cmake-msvc-fixes into 2d86e8d on curl:master.

@bagder bagder added the cmake label Jul 6, 2017
@bagder
Copy link
Member

bagder commented Jul 6, 2017

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()

Copy link
Member

@bagder bagder Jul 6, 2017

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?

Copy link
Author

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"
Copy link
Member

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.

Copy link
Author

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()
Copy link
Member

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

@paulharris
Copy link
Author

I assume I am supposed to adjust the pull like so...
I manually reverted those perl/docs changes and pushed the commit.
Alternatively, I could have redone the initial commit and force-pushed it, but that's not what the github gods intended, right?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.004%) to 74.472% when pulling 4a3821f on paulharris:cmake-msvc-fixes into 2d86e8d on curl:master.

@bagder
Copy link
Member

bagder commented Jul 7, 2017

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.

@bagder bagder closed this in 45f3994 Jul 7, 2017
@bagder
Copy link
Member

bagder commented Jul 7, 2017

Thanks!

@p
Copy link
Contributor

p commented Dec 9, 2017

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

openssl: https://github.com/pycurl/pycurl/blob/7dee7311ecc4cc0ec42588e3d5c3ad9c97d1582d/winbuild/openssl-fix-crt-1.1.0.patch

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.

@bagder
Copy link
Member

bagder commented Dec 9, 2017

@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) ?

@p
Copy link
Contributor

p commented Dec 10, 2017

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.

@lock lock bot locked as resolved and limited conversation to collaborators May 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

None yet

5 participants