[Thrift] Make Thrift static again

[philjdf]

Master seems to be building Thrift as a shared library. Reviewing the history shows we've lost the flags which build Thrift statically.

[Rastaban]

Good catch, could you also bump the control version to 2019-05-07-3? This will help the vcpkg update command.

[philjdf]

@Rastaban done. Though maybe this patch still isn't right, shouldn't whether it's static or shared be driven by ${VCPKG_LIBRARY_LINKAGE}?

[GPSnoopy]

@philjdf I am myself confused by the vcpkg_check_linkage(ONLY_STATIC_LIBRARY) line in the same file.

[Rastaban]

If the library can be built as static or shared then yes, VCPKG_LIBRARY_LINKAGE should decide which way it it built. When I see vcpkg_check_linkage(ONLY_STATIC_LIBRARY) Then I assume it is only capable of being built as a static library. I'm not familiar with Thrift, but based on the existence of WITH_SHARED_LIB and WITH_STATIC_LIB it looks like it should support either configuration so I don't know why it was insisting on only static previously.

Go ahead and switch it to using VCPKG_LIBRARY_LINKAGE, if you run into issues with that then at least we can add a comment next to vcpkg_check_linkage(ONLY_STATIC_LIBRARY) explaining why for the next guys.

[dan-shaw]

WITH_SHARED_LIB and WITH_STATIC_LIB for Thrift seem to be deprecated, and Thrift supports using BUILD_SHARED_LIBS instead. It appears that thrift is built statically (unless I'm missing something) since setting vcpkg_check_linkage(ONLY_STATIC_LIBRARY) should turn off BUILD_SHARED_LIBS

[cbezault]

But it does appear that Thrift can be built both statically and dynamically?
Otherwise @dan-shaw, you're right vcpkg_check_linkage will override BUILD_SHARED_LIBS.

[cbezault]

I see thrift being built statically with mainline right now. If that's not true just comment below, otherwise I'm closing this PR.

[GPSnoopy]

@cbezault I've tried the following triplets:

• x64-windows -> static libraries (should be dynamic)
• x64-windows-static -> static libraries
• x64-linux -> dynamic libraries (should be static)

I'd say the PR with the modifications discussed above is needed.

[philjdf]

I've tried removing vcpkg_check_linkage(ONLY_STATIC_LIBRARY) and removing -DWITH_SHARED_LIB=OFF and -DWITH_STATIC_LIB=ON, because BUILD_SHARED_LIBS should be handled automatically. This then means a bit more care is required with the bin directory.

Not sure what's going on with file(REMOVE_RECURSE ${CURRENT_PACKAGES_DIR}/debug/include) so I left it.

Given one of the patches the Arrow port applies is to how it finds Thrift, to look in the tools dir for the compiler (rather than the bin dir as specified by the CMake target), is that something I can fix while I'm at it?

[philjdf]

These latest changes give:

• x64-windows -> shared
• x64-windows-static -> static
• x64-linux -> shared

So things are behaving on Windows but we're still getting an .so on Linux. I have verified that BUILD_SHARED_LIBS is OFF on Linux.

[dan-shaw]

Interesting, it appears that BUILD_SHARED_LIBS is force set to ON on Linux here: https://github.com/apache/thrift/blob/master/build/cmake/DefineOptions.cmake#L121
Also, the WITH_SHARED_LIB variable is set after that line, so it could explain why setting BUILD_SHARED_LIBS doesn't work but setting WITH_SHARED_LIB does.

[philjdf]

So if you're not Win32, it removes the option and hard-codes to a shared lib, otherwise if we're on Win32 we get the choice with default to static? It over-writes the correct value of BUILD_SHARED_LIBS for sure, and is therefore wrong?

In which case we need to set WITH_SHARED_LIB and WITH_STATIC_LIB to work around that? And should it be set to static or should it be driven by VCPKG_LIBRARY_LINKAGE?

When I said BUILD_SHARED_LIBS is OFF on Linux, I checked with a message() call at the start of the CMakeLists.txt, before it includes the DefineOptions file.

[dan-shaw]

It would appear to be that way. It seems the fix would be to:

1. Use WITH_SHARED_LIB and WITH_STATIC_LIB instead of BUILD_SHARED_LIBS. See https://github.com/microsoft/vcpkg/blob/master/docs/maintainers/maintainer-guide.md#choose-either-static-or-shared-binaries

2. And if our target platform is Windows VCPKG_TARGET_IS_WINDOWS, we should set vcpkg_check_linkage(ONLY_STATIC_LIBRARY) - this will set VCPKG_LIBRARY_LINKAGE and do some error checking.

[GPSnoopy]

2. And if our target platform is Windows VCPKG_TARGET_IS_WINDOWS, we should set vcpkg_check_linkage(ONLY_STATIC_LIBRARY) - this will set VCPKG_LIBRARY_LINKAGE and do some error checking.

From what @philjdf is saying, he got the Windows shared lib to build.

[cbezault]

It's true that it built, but there are a bunch of places that state that thrift should only be built statically on Windows. However, it's not exactly clear why to me.

[GPSnoopy]

@cbezault You're right. It seems that they are aiming to support Windows shared binaries for the 0.13 release:

• https://github.com/apache/thrift/blob/master/CHANGES.md
• THRIFT-4732: windows cmake refinements and add windows docker build support apache/thrift#1687
• https://issues.apache.org/jira/browse/THRIFT-4759

Given that the vcpkg port file picks up the master branch from a few months ago, the Windows Thrift DLL might still be missing some symbol or might now be working.

@philjdf As @Rastaban pointed, we should document this above the vcpkg_check_linkage(ONLY_STATIC_LIBRARY). But it sounds like this might become obsolete soon.

[philjdf]

Have made changes as discussed here. I've added a couple of comments, and kept the more careful clearing out of the compiler from the bin dir because that will be required when we can support building shared.

Can confirm:

• x64-windows -> static
• x64-windows-static -> static
• x64-linux -> static

[dan-shaw]

Thanks for the PR and comments! @philjdf @GPSnoopy