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

vcpkg.targets should not automatically attempt to link in every single installed library #306

Closed
Orvid opened this Issue Nov 19, 2016 · 4 comments

Comments

Projects
None yet
2 participants
@Orvid
Copy link

Orvid commented Nov 19, 2016

Currently the vcpkg targets file adds *.lib as extra dependencies to the link step, what's more, it adds them before anything that it's possible for the user to specify. This means that the user has absolutely no control what-so-ever over what libraries are searched for symbols. This becomes a significant problem when multiple libraries provide the same symbols. In the case that bit me today, gmock_main.lib includes the symbols for gmock.lib in addition to a main function that correctly starts up gflags and runs the available tests. gmock.lib itself further includes every symbol from gtest.lib in addition to the actual mocking library. This means that, regardless of what the user wanted, if they reference a symbol in gtest, they will always link against gmock.lib, because it comes first in the link order. This isn't a big deal for gtest symbols, but when a program is expecting to link against gmock_main.lib, the only function that gets resolved to there is main, every other function gets resolved to gmock.lib, with the net result that no tests are found by the testing framework and everything is very sad :(

I just spent the last 2 hours cleaning, building, and rebuilding my CMake configuration, target configs and every last one of my tests because of this :(

Adding the vcpkg directories as search directories is fine; adding every library as an input is not.

@ras0219-msft ras0219-msft self-assigned this Nov 21, 2016

@Orvid Orvid referenced this issue Nov 25, 2016

Closed

Add zstd port #324

ras0219-msft added a commit that referenced this issue Nov 26, 2016

[vcpkg-integrate] Provide an opt-out for autolinking. Properly suppre…
…ss integration via MSBuild when using CMake.

Add lib\ and lib\manual-link\ to additional library paths.

[gtest] Because the four libraries provided by gtest supply redundant symbols and define main, opt them all out.

Addresses #306.
@ras0219-msft

This comment has been minimized.

Copy link
Contributor

ras0219-msft commented Nov 26, 2016

First, thank you for bringing up the issue with gtest/gmock; I'm sincerely sorry that it cause you so much pain :(. I believe I've fixed the issue on the fix-auto-link branch and it passes my local tests, but I'd like you to try it out first to ensure your projects work exactly as you'd like as well.

MSBuild

What we are trying to implement using *.lib is providing the equivalent of #pragma comment(lib, X) for every library while using MSBuild (scroll down for the situation with CMake). This is a popular technique for libraries which hold Windows as a primary platform (boost, for example), but many cross-platform libraries aren't very interested in implementing and maintaining MSVC specific directives.

In the case of per-project package managers, this is an essential feature (NuGet, conan, hunter, etc). In the general case of system package managers, this is completely impossible because decisions are (rarely, but often enough) made during the link process like the issue you hit above: Do I implement main or does gmock implement main for me? Do I use version X or version Y? In the case of Vcpkg, due to the specific constraints we have placed on the system, the vast majority of libraries (99%+) will not have decisions to be made at link time. All decisions about the binary (version/compiler/flags/etc) are made via the portfile.cmake and the triplet file. We know the exact .lib that matches the #include <openssl/openssl.h> you included and there is no issue with mismatch. The linker will automatically discard any .libs that you don't need, because you aren't importing any of their symbols.

However, there are some pitfalls just like the one you hit above! For some reason, it seems gmock and gtest didn't factor their symbols into uniquely-owning DLLs and instead gtest is a complete subset of gmock. If they were different packages, this would clearly be a violation of the good library citizen contract: don't re-export someone else's symbols! However, since they're both in the same source repository, and the library is quite popular, and the official guidance is to replace your gtest.lib link with gmock.lib when using gmock, this requires an exception. We fully anticipate there to be few, but more of these cases in the future.

To address this issue, I have added a subdirectory of lib\ called manual-link which will always be opted-out of any autolinking we perform. This will allow libraries which step beyond the bounds of a "normal" library (such as defining _main), to always opt-out of automatic linking. This subdirectory will be available on your link path (along with the normal lib) so that you can simply list gmock_main.lib in your AdditionalDependencies when using MSBuild or use it in a target_link_libraries() directive in CMake.

Additionally, you're totally right that some developers will want to use the link line as a "final defense" against pulling in additional dependencies. To this end, I've also added an MSBuild property called VcpkgAutoLink which when set to false will completely disable the automatic linking for your projects (you will still get the include and lib paths).

CMake

Above, I mentioned MSBuild; what about CMake? CMake is designed from the ground up to allow your project to be as platform agnostic as possible, so this functionality should be entirely opt-in for CMake projects, if available, to mimic other platforms as closely as possible. We previously had a bug which would enable the autolinking for MSBuild projects generated via our CMake toolchain -- I have also fixed that bug in 32157f8 so if you're currently using CMake for your projects that should fix the issue for you.

Finally, thanks for also bringing up the issue that we're adding the include and lib paths ahead of the user-specified paths! I believe I've fixed that issue in the above commit as well.

@Orvid

This comment has been minimized.

Copy link
Author

Orvid commented Nov 29, 2016

It's taken me a bit to get to testing this, but I'll hopefully be able to later tonight or tomorrow.

@Orvid

This comment has been minimized.

Copy link
Author

Orvid commented Nov 29, 2016

Well, I got a chance to test it sooner than I thought I would. Everything builds (and works) as I would expect, although I end up with this as the "Additional Library Directories" for the Visual Studio projects generated by CMake:

C:/Projects/vcpkg/scripts/buildsystems/../../installed/x64-windows/debug/lib/manual-link
C:/Projects/vcpkg/scripts/buildsystems/../../installed/x64-windows/debug/lib/manual-link/$(Configuration)
C:/Projects/vcpkg/scripts/buildsystems/../../installed/x64-windows/lib/manual-link
C:/Projects/vcpkg/scripts/buildsystems/../../installed/x64-windows/lib/manual-link/$(Configuration)

Although it's mostly the ones with $(Configuration) that I question, I wonder, is it really necessary to add the lib paths to the VS projects themselves if they've already been added when CMake ran and did it's configuration? CMake should have already resolved the absolute path to the libraries, and that's what should be getting linked in, so the individual projects shouldn't actually need the library directory set.

@ras0219-msft

This comment has been minimized.

Copy link
Contributor

ras0219-msft commented Nov 30, 2016

I think this is CMake's doing -- we're not automatically adding those as far as I'm aware. I just appended them to CMAKE_LIBRARY_PATH. It shouldn't hurt anything, since we shouldn't have any subdirectories there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.