Patches for MinGW-w64 #2553

ed-alertedh · 2019-10-07T00:50:26Z

This set of changes allows me to build librdkafka with a MinGW-w64 cross-compiler and the existing CMake configuration. The main changes were:

Check for __MINGW32__ (MinGW-w64 defines this for both 32-bit and 64-bit targets) in addition to _MSC_VER
Use __thread rather than __declspec(thread) for MinGW.
Change the case of a few headers so that it works on a Linux host with a case-sensitive filesystem.
Define WINVER and _WIN32_WINNT to match the Windows 8.1 target set in the .vcxproj files

All the test cases that are enabled for Windows passed when I ran it. I had some trouble testing that the Visual Studio build still works because the OpenSSL lib you are linking with has moved to VS 2017 in the latest builds and they do not retain old builds for security reasons. It still compiles in VS 2015 but the link fails due to mismatched runtime libs.

If you are interested in merging this I'm very willing to make any necessary revisions. E.g. you might prefer to create a new define rather than repeating defined(_MSC_VER) || defined(__MINGW32__) everywhere. I also have a docker container with the cross-compiler toolchain I can contribute to help you build the tests.

edit:

Defined UNICODE to eliminate bug with mismatched char* and wchar_t*
Added a couple of pointer casts to silence GCC warnings about LONG* vs int32_t* (they are the same size under MinGW-w64, as they are with MSVC)

edenhill · 2019-10-07T06:57:15Z

Thanks for your contribution!

I think it does make sense to create a "WITH_WIN32" define to replace the MSC_VER or MINGW checks.

ed-alertedh · 2019-10-08T01:53:25Z

Not at all, thanks for providing such a useful library!

I've just refactored to use WITH_WIN32 instead (there are a couple of special cases where it still checks for MSVC vs MinGW). Having taken a look at your current CI setup I think it might be easiest to take advantage of the "Visual Studio 2013" Appveyor image you are using already. It has CMake and various MinGW-w64 versions installed already so it should be possible to build with CMake and run tests in there. I'm happy to take a crack at setting that up if you'd like.

neptoess · 2020-01-06T10:49:07Z

@ed-alertedh,
Does using _WIN32 instead of the the custom WITH_WIN32 work for this?

The only change I’m concerned with is using __thread instead of __declspec(thread). Both of these are compiler-specific, so it may make sense to explicitly check MSVC or MinGW in this case (maybe fallback to _Thread_local if both MSVC and MinGW checks fail? Assumes whatever compiler this is supports C11 though). Thoughts?

ed-alertedh · 2020-01-06T23:16:36Z

Hmm, it's been a while since I looked at this. @neptoess as far as I can tell, _WIN32 is compiler-specific and is only defined by MSVC [1], hence why I was originally checking for _MSC_VER or __MINGW32__. It could be that your cgo build system is defining some extra symbols for compatibility.

I agree that this PR could still be improved. Last time I worked on this I opted to make the build system define WITH_WIN32 since there didn't seem to be a common header included by everything. But I hadn't really considered consumers of the public header, so really the solution is probably to include something like this everywhere:

#if defined(_MSC_VER) || defined(__MINGW32__)
#define WITH_WIN32
#endif

This is a little fragile though, because if you forgot to include it in one file it will build incorrectly. That was the other motivation for defining WITH_WIN32 in the build system.

With respect to __thread, vs __declspec(thread), I believe I already put compiler-specific checks in there out of necessity. Not being super familiar with C11 I didn't know _Thread_local was a fallback option, I guess it can't hurt to add that.

[1] http://mingw.5.n7.nabble.com/difference-between-WIN32-and-WIN32-macro-tp10055p10057.html
"As far as I know, WIN32/_WIN32 are defines that are builtins of the
Microsoft compiler."

neptoess · 2020-01-07T01:18:19Z

@ed-alertedh,
If I run use the MinGW environment and run gcc -E -dM test.c I see _WIN32. Can you try this with the Linux mingw gcc? I agree that we don’t want to rely on the build system defining macros.

Regarding __thread vs __declspec, I think you got that covered pretty well. No need to bring _Thread_local into it.

I think we’re fairly safe either way we handle MSVC / coexistence (as long as we don’t rely on the build system) since:

The Windows binaries for rdkafka are compiled with MSVC, and both options leave that build working
Regardless of how we handle MinGW, I highly doubt a 3rd compiler targeting Windows will enter the mix.

I’ll close my PR since this one getting merged will solve the same problem.

ed-alertedh · 2020-01-07T01:40:28Z

@neptoess my apologies, you're right!

# /usr/local/bin/x86_64-w64-mingw32-gcc  -E -dM test.c
[...]
#define _WIN32 1
[...]
#define _WIN64 1
[...]

That makes things much cleaner, good find. This is still something we're planning to use whether it gets merged or not, so I'll aim to merge the latest changes from master and refactor to use _WIN32 some time in the next week or so.

edenhill

Great stuff! Just needs some minor modifications.

It would be really helpful if you could add a job to the travis matrix to build on mingw.

src/tinycthread_extra.h

examples/rdkafka_complex_consumer_example.cpp

src/rdatomic.h

ed-alertedh · 2020-02-06T07:07:40Z

I've made a start at building it in Travis using CMake and a cross-compiler. Would you prefer it to use mklove instead? The CMake settings I have configured in .travis.yml worked locally in an Ubuntu Trusty VM, but I ran out of time today to get it building in Travis.

The older version of MinGW-w64 included with Trusty seems to be missing ctime_s and ctime_r so I have put in an alternate implementation for now - not sure if a non-english locale might break it with weekday and month names that aren't three chars long. In my codebase I am using a more recent version of MinGW-w64 and I did not seem to have issues using ctime_s.

edenhill · 2020-02-06T07:38:21Z

I recommend you use CMake for mingw build, suspecting that mklove will require quite a bit of modifications to fully support mingw.

It would be good if the mingw build was using a (standard) docker image, have a look at packaging/tools/build-...sh for various platforms.

ed-alertedh · 2020-02-07T05:29:38Z

I spent some time today working on the CI build for this with mixed results. The history for this branch is getting pretty long so at a later point I can rebase if you like.

I found that the Trusty MinGW package is a little old and WSAPoll isn't properly defined in the headers. I had no problems building locally in the ubuntu:xenial docker container but for some reason the Travis xenial environment still didn't work for me and neither did bionic - unsure if there is some weird caching going on or what.

For now I have added a packaging script to build inside a container which seems to be working. The bincrafters/docker-mingw-gcc7 image doesn't seem to be maintained though so the perhaps it will be better to just use a debian/ubuntu image and install the mingw packages in the packaging script.

ed-alertedh · 2020-02-11T05:26:03Z

OK, I have builds working now in:

Travis bionic env
ubuntu:bionic docker container as another packaging script that uploads artifacts
Travis windows env (using MSYS2 to provide cmake+make+mingw-w64)

I also have the tests running and passing in the Travis windows env (same set you are running for the VS build in appveyor).

Do you think you would distribute binaries for this configuration? I'm happy to keep building from source but if so, I'm wondering if it would be better to upload artifacts for the windows build since that is now the one that is tested. At the very least I think it would be nice to leave one linux cross-compile in there to check that it still builds in that configuration (main difference being the case-sensitive filesystem).

neptoess · 2020-04-10T16:00:58Z

Anything I can do to help here? I see some of the tests failed in travis, but I'm not sure if that's the only thing preventing this from being merged.

EDIT: I was just looking through the confluent-kafka-go repo, and realized that, if this gets merged, we can build a librdkafka_windows.a and greatly simplify using confluent-kafka-go on Windows (I know it's not officially supported, but you can do this today by jumping through a few hoops to get MinGW gcc to link to the MSVC-built DLL)

edenhill · 2020-04-14T07:28:36Z

Sorry for dropping the ball on this one, let's schedule it for the upcoming v1.5.0 release.

One thing that needs to be addressed is SSL support, a prebuilt librdkafka without SSL is not that useful for most people in this age.
Rumour has it that the Shining light OpenSSL builds that we're using in MSVC should work with MinGW as well, so might want to give that a stab?

I'd also like to see the build commands in .travis.yml be turned into cmd/bat files in packaging/mingw.. to keep .travis.yml cleaner and compartmentalize the mingw build logic to its own directory.

edenhill · 2020-04-14T07:28:49Z

Also make sure to rebase this on latest master.

ed-alertedh · 2020-04-21T00:57:08Z

@edenhill OK, sounds good. I'll do my best to prioritise this as you're clearly very busy! To give me a rough idea, what kind of target date are we talking here?

MinGW can't link with MSVC .lib static libs, but I could try dynamically linking with the DLLs in the Shining Light distribution. If the goal is to use the same OpenSSL library between the MSVC and MinGW librdkafka builds though, there will be a little extra work required to upgrade your version of Visual Studio: "September 13, 2018 - Visual Studio 2017 is being used for builds meaning new runtime requirements". I can't download the old versions of the libs living on your MSVC build instance - the Shining Light page has an amusing security-oriented explanation why.

neptoess · 2020-04-21T01:23:47Z

MSYS2 distributes a MinGW version of openssl via pacman (https://packages.msys2.org/package/mingw-w64-x86_64-openssl), and it's up to date. It does include some DLLs, but there are no dependencies on the Visual C++ redistributables, so I strongly suspect they're using MinGW to build them.

ed-alertedh · 2020-04-21T01:59:55Z

@neptoess I thought maybe the Shining Light build integrated with the Windows certificate store but it doesn't seem like it, so I think using the MSYS2 package is probably an easier option to set up and keep up-to-date. Only disadvantage I can see is differing versions/builds of OpenSSL might add more maintenance burden, but realistically it seems like majority of users will keep using the MSVC build.

edit: Just saw your comment about this potentially allowing for easier golang support on Windows. I guess in that case this build configuration could end up with a decent user base!

edenhill · 2020-04-21T06:32:09Z

Using the msys2 openssl package sounds like a good idea 👍

neptoess · 2020-04-21T11:44:36Z

.travis.yml

@@ -62,7 +62,7 @@ matrix:
   env:
    - SKIP_MAKE=y
   before_script:
-    - $mingw64 cmake -DCMAKE_MAKE_PROGRAM=mingw32-make -G "MSYS Makefiles" -DCMAKE_TOOLCHAIN_FILE=./packaging/mingw-w64/mingw_toolchain.cmake \
+    - $mingw64 cmake -DCMAKE_MAKE_PROGRAM=mingw32-make -G "MSYS Makefiles" -DCMAKE_TOOLCHAIN_FILE=./packaging/mingw-w64/msys2_mingw_toolchain.cmake \


Suggested change

- $mingw64 cmake -DCMAKE_MAKE_PROGRAM=mingw32-make -G "MSYS Makefiles" -DCMAKE_TOOLCHAIN_FILE=./packaging/mingw-w64/msys2_mingw_toolchain.cmake \

- $mingw64 cmake -DCMAKE_MAKE_PROGRAM=mingw32-make -G "MinGW Makefiles" -DCMAKE_TOOLCHAIN_FILE=./packaging/mingw-w64/msys2_mingw_toolchain.cmake \

https://cmake.org/cmake/help/latest/generator/MinGW%20Makefiles.html
Pretty sure we should be using MinGW Makefiles, not MSYS Makefiles. MSYS is a POSIX emulation environment and creates a dependency on the MSYS DLL, where MinGW builds native Windows applications.

EDIT: Let me know if you want help with the specifics of this switch, since I see you're also using the MSYS2 shell and exporting MAKE.

From memory I did initially try to generate MinGW Makefiles and could not get them to work under the MSYS2 shell. I now see they are intended to run under cmd.exe. There would probably be some work required to change approach and set up the path correctly for cmd.exe.

The cmake toolchain file I defined sets MAKE, CC, etc. so that it cross-compiles for MinGW. I will verify today that I haven't accidentally built against the MSYS runtime instead, but the fact that the unit tests run with only /mingw64/bin/{libwinpthread-1.dll,libgcc_s_seh-1.dll,libstdc++-6.dll} copied across suggests that it's working (unless MSYS has put its libraries in the system PATH in this build env).

So I guess in summary, I think this works already but if there's a much simpler way to do it then I'm open to it.

I forget exactly what happened when I switched it to "MSYS Makefiles" - for a while there I was changing all sorts of things and suspect a stale cache on Travis was giving me the same results every time.

"MinGW Makefiles" appears to work so long as the build runs in the MinGW64 shell rather than the MSYS2 shell, meaning the toolchain file is now only needed to cross-compile from linux.

neptoess · 2020-04-22T11:50:23Z

packaging/mingw-w64/configure-build-msys2-mingw.sh

+      -DRDKAFKA_BUILD_STATIC:BOOL=OFF \
+      -DCMAKE_WINDOWS_EXPORT_ALL_SYMBOLS:BOOL=TRUE . 1>&2
+$mingw64 mingw32-make 1>&2
+cp /mingw64/bin/{libwinpthread-1.dll,libgcc_s_seh-1.dll,libstdc++-6.dll,libssl-1*.dll,libcrypto-1*.dll} src/librdkafka.dll src-cpp/librdkafka++.dll ./tests


If I'm thinking of this correctly, I don't think any of these DLLs should need copied out of mingw64/bin. mingw64/openssl, for example, allows both static and dynamic linking, and these are the only DLLs that get distributed with librdkafka on Windows (ignore the msvc*.dll files, those are only necessary when building with Visual Studio)

edenhill

Looking good, left a couple of comments.

.travis.yml

examples/rdkafka_complex_consumer_example.cpp

edenhill · 2020-04-23T06:00:40Z

packaging/mingw-w64/configure-build-linux-mingw.sh

+      -DWITH_LIBDL:BOOL=OFF \
+      -DWITH_PLUGINS:BOOL=OFF \
+      -DWITH_SASL:BOOL=OFF \
+      -DWITH_SSL:BOOL=OFF \


As started previously, this build without SSL (and zlib) will not be widely usable.

Maybe I should just get rid of the cross-compiled build? It was mainly intended to check if the code still builds with a Linux host (as I'm currently doing on my project) but if official MinGW builds start becoming available then I don't actually need that any more.

packaging/mingw-w64/configure-build-linux-mingw.sh

packaging/mingw-w64/mingw_toolchain.cmake

packaging/mingw-w64/travis-before-install.sh

edenhill · 2020-04-23T06:07:38Z

packaging/mingw-w64/travis-before-install.sh

+    windows)
+        [[ ! -f C:/tools/msys64/msys2_shell.cmd ]] && rm -rf C:/tools/msys64
+        choco uninstall -y mingw
+        choco upgrade --no-progress -y msys2


can choco be used to install libssl, zlib, etc?

Currently it's all being installed with pacman in MSYS2. When I tested locally, CMake found my Shining Light OpenSSL installation and was able to dynamically link to it (looked like it understands import libs). Unsure if it works as seamlessly for zlib, etc but it is probably an option.

An advantage to installing the libs via MSYS2 is that you could statically link the libs if you wanted (assuming that the choco packages only provide DLLs and maybe a static lib for MSVC). That might matter a little more for golang support.

I agree with Ed that it's smarter to install the MinGW packages via the MSYS2 package manager. It's almost guaranteed that we'll have a dependency on one or more Microsoft Visual C++ Redistributables if we install via choco, and CMake will have to hunt down all the headers and .lib files, where they all go to a well known spot if we use the MSYS2 package manager.

It looks like the MSYS2 MinGW packages support both static and dynamic linking, so, as Ed said, that makes the future brighter for confluent-kafka-go.

Lastly, zlib looks pretty seamless as well, https://packages.msys2.org/package/mingw-w64-x86_64-zlib provides all necessary headers, libz.a, libz.dll.a, and zlib1.dll

edenhill · 2020-04-23T06:10:25Z

src/tinycthread_extra.h

-#define rwlock_wrlock_d(rwl)   do { if (1) printf("Thr %i: at %i:   WRLOCK %p   %s (%i, %i)\n", GetCurrentThreadId(), __LINE__, rwl, __FUNCTION__, (rwl)->rcnt, (rwl)->wcnt); assert((rwl)->rcnt >= 0 && (rwl)->wcnt >= 0); AcquireSRWLockExclusive(&(rwl)->lock); InterlockedIncrement(&(rwl)->wcnt); } while (0)
-#define rwlock_rdunlock_d(rwl) do { if (1) printf("Thr %i: at %i: RDUNLOCK %p   %s (%i, %i)\n", GetCurrentThreadId(), __LINE__, rwl, __FUNCTION__, (rwl)->rcnt, (rwl)->wcnt); assert((rwl)->rcnt > 0 && (rwl)->wcnt >= 0); ReleaseSRWLockShared(&(rwl)->lock); InterlockedDecrement(&(rwl)->rcnt); } while (0)
-#define rwlock_wrunlock_d(rwl) do { if (1) printf("Thr %i: at %i: RWUNLOCK %p   %s (%i, %i)\n", GetCurrentThreadId(), __LINE__, rwl, __FUNCTION__, (rwl)->rcnt, (rwl)->wcnt); assert((rwl)->rcnt >= 0 && (rwl)->wcnt > 0); ReleaseSRWLockExclusive(&(rwl)->lock); InterlockedDecrement(&(rwl)->wcnt); } while (0)
+#define rwlock_rdlock(rwl)   do { if (0) printf("Thr %i: at %i:   RDLOCK %p   %s (%i, %i)\n", GetCurrentThreadId(), __LINE__, rwl, __FUNCTION__, (rwl)->rcnt, (rwl)->wcnt); assert((rwl)->rcnt >= 0 && (rwl)->wcnt >= 0); AcquireSRWLockShared(&(rwl)->lock); InterlockedIncrement((LONG *)&(rwl)->rcnt); } while (0)


Instead of casting here, in rwlock_t struct add this:

#if _WIN32 LONG rcnt; LONG wcnt; #else int rcnt; int wcnt; #endif

Looks like it's already guarded by _TTHREAD_WIN32_ so probably safe to just change it to LONG? (it's declared as LONG here https://docs.microsoft.com/en-us/windows/win32/api/winnt/nf-winnt-interlockedincrement)

ed-alertedh · 2020-04-23T08:40:52Z

OK, hopefully it's close now. I haven't forgotten about rebasing but have been holding off on that because I tend to generate a lot of small commits!

Probably the major questions left are whether to get rid of the Linux cross-compile (which was mainly intended as a "smoke test") and whether to continue installing libs with MSYS2/pacman or consider switching to choco.

neptoess · 2020-04-23T12:49:27Z

Probably the major questions left are whether to get rid of the Linux cross-compile (which was mainly intended as a "smoke test")

Since the artifacts coming out of the build are the same as the Windows MinGW build, and I think MSYS2's package manager makes wrangling dependencies more straightforward than cross-compiling everything from a Linux host, I don't see a downside to removing the Linux cross-compile build.

and whether to continue installing libs with MSYS2/pacman or consider switching to choco.

choco is mostly focused on automating application installs. NuGet is the system most often used for development package management, but, either way, neither is really focused on functionality / compatibility with the MinGW tooling. The MinGW packages from MSYS2 are actually intended to be used by the MinGW tooling, and are tested against it.
tl;dr I see no upside to using choco instead of pacman

…WIN32 instead of _MSC_VER in header guards. Various other small patches included to account for compiler differences.

ed-alertedh · 2020-04-24T04:09:58Z

I've rebased/squashed onto latest master. The linux cross-compile is gone. I tend to agree that MSYS2 is a better way to manage the library dependencies than choco in this case. Let me know if you want anything else revised.

edenhill

Looks good, just some minor comments left

edenhill · 2020-04-27T11:43:01Z

.travis.yml

+   env:
+    - SKIP_MAKE=y
+   before_install:
+    - source ./packaging/mingw-w64/travis-before-install.sh 1>&2


Why is the output redirect necessary? Travis logs both stdout and stderr.

Probably from when I was testing in docker, will get rid of it!

edenhill · 2020-04-27T11:45:31Z

examples/rdkafka_complex_consumer_example.cpp

@@ -81,7 +81,7 @@ static void print_time () {
        struct timeval tv;
        char buf[64];
        gettimeofday(&tv, NULL);
-        strftime(buf, sizeof(buf) - 1, "%Y-%m-%d %H:%M:%S", localtime(&tv.tv_sec));
+        strftime(buf, sizeof(buf) - 1, "%Y-%m-%d %H:%M:%S", localtime((time_t *)&tv.tv_sec));


Not sure this cast is safe.
When (on what platform) is tv_sec not a time_t?

Looks like winsock2 pulls in this definition: https://github.com/mirror/mingw-w64/blob/master/mingw-w64-headers/crt/_timeval.h#L12

https://developercommunity.visualstudio.com/content/problem/291705/move-timeval-struct-out-of-winsock2h.html

I will see if there's any way to avoid including winsock2 in this example. The danger would be if sizeof(time_t) > sizeof(long).

I opted to just replace the use of ATL with a WinAPI call here so that MinGW can share the code with MSVC. It wasn't apparent to me how I could stop winsock2.h getting included since it's needed for sockets.

edenhill · 2020-04-27T11:46:35Z

packaging/mingw-w64/configure-build-msys2-mingw.sh

+$mingw64 mingw32-make install
+
+export PATH="$PWD/dest/bin:/mingw64/bin/:${PATH}"
+./tests/test-runner.exe -l -Q -p1


test-runner needs to be run from the test/ directory as the current directory.

edenhill · 2020-04-27T11:51:47Z

src/rdkafka_int.h

-#else
+#endif
+
+#ifdef _MSC_VER


I'm guessing this is intentional because mingw provides mode_t and msvc does not?

edenhill · 2020-04-27T11:52:19Z

src/rdkafka_offset.c

+#include <shlwapi.h>
+#endif
+
+#ifdef _MSC_VER


Not sure we really need this typedef here since it is already in rdkafka_int.h

edenhill · 2020-04-27T11:54:50Z

tests/test.c

@@ -5516,7 +5516,9 @@ void test_fail0 (const char *file, int line, const char *function,
        char timestr[32];
        time_t tnow = time(NULL);

-#ifdef _MSC_VER
+#ifdef __MINGW32__
+        strftime(timestr, sizeof(timestr), "%a %b %d %H:%M:%S %Y\n", localtime(&tnow));


nit: no need for the newline (but it is ok), the removal below is optional

OK. Side note: IIRC the strftime was only necessary on older versions of MinGW. They implemented ctime_s correctly at some point. I will leave it there for wider compatibility.

…om sys/time.h instead of winsock2.h for mingw

…t available in MinGW)

ed-alertedh · 2020-04-28T05:47:57Z

@edenhill OK, I think I've addressed all your comments. I'm struggling to explain the error in this build. It think it might somehow be due to a bad/stale cache because the error relates to a codepath that shouldn't be active with _WIN32 defined. It builds fine for me locally in MSYS2 shell on Windows.

Unsure if I needed to do more when I added Windows to the matrix to ensure caching works correctly? I'm not super familiar with Travis CI.

edenhill · 2020-04-28T06:58:12Z

Do you mean this error?

C:\Users\travis\build\edenhill\librdkafka\tests\test.c:3903:11: warning: implicit declaration of function 'WIFSIGNALED' [-Wimplicit-function-declaration]

Don't see why that ifdef-guard doesnt work

edenhill · 2020-04-28T06:58:44Z

.travis.yml

@@ -1,7 +1,7 @@
 language: c
 dist: trusty
 sudo: false
-cache: ccache
+# cache: ccache


Make sure to back this out when you're done testing.

edenhill

Can you educate me on when and how the MinGW builds are useful, compared to the msvc nuget package? I'm not questioning the need, just want to understand since my knowledge of Windows runtimes are so-and-so.

edenhill · 2020-04-28T07:01:37Z

packaging/mingw-w64/configure-build-msys2-mingw.sh

+$mingw64 mingw32-make
+$mingw64 mingw32-make install
+
+export PATH="$PWD/dest/bin:/mingw64/bin/:${PATH}"


out of curiosity, why is this PATH update needed for the test-runner?

$PWD/dest/bin is for librdkafka.dll and librdkafka++.dll. /mingw64/bin/ is for the MinGW runtime outside of the mingw64 shell. (on the other test runs you're setting LD_LIBRARY_PATH for this stuff)

Ah, ok, so PATH is used for dynamic library lookups on mingw (or is it windows in general)? Didnt know that.

Correct, Windows in general uses PATH to find dlls. Unlike *nix it also always searches the current working directory.

ed-alertedh · 2020-04-28T07:42:25Z

In my use-case, I wanted to keep all my CI running in Linux on our existing CircleCI plan so I prefer to cross-compile my application with MinGW. So first and foremost, this PR makes the rdkafka.h header work on MinGW. This should allow dynamic linking with your current MSVC dll builds.

But it wasn't a lot more effort to make the whole thing compile on MinGW, so I thought it was worth it. Being able to statically link librdkafka is convenient for me and I can't do that with the MSVC build.

It sounded like all of this would also enable confluent-kafka-go to build on windows without patches, either linked dynamically against your MSVC builds or linked statically (which is standard for golang programs).

edit: I have to finish up for the day but it looks like I accidentally cancelled my test build with cache disabled -_-

edenhill · 2020-04-28T08:35:06Z

Thank you for the explanation.

Some follow up questions:

Can mingw dynamic libraries be linked by msvc?
Can mingw static libraries be linked by msvc/vs?
Can mingw dynamic libraries depend on msvc DLLs?

neptoess · 2020-04-28T13:24:28Z

@edenhill

All answers assume pure C code. C++ compiler ABIs are a mess. clang has this problem solved (they implemented the ability to output in the same ABI as Visual C++), but gcc does not.

Can mingw dynamic libraries be linked by msvc?

Yes. Even if the import libraries generated by MinGW couldn't be linked by Visual Studio, Visual Studio will generate import libraries based on a DLLs exports if you try to link to the .dll file directly.

Can mingw static libraries be linked by msvc/vs?

Yes, in practice. However, for this question and the previous one, we should probably consider something. Currently, there appears to be link compatibility between objects built with MinGW and those built with MSVC, and this has been the case for well over a decade as far as I know. That said, I can find no evidence that link compatibility with MSVC is a project goal of MinGW, So we're in the clear now, but it's probably best that we don't knowingly do anything in the code that breaks the ability to build with Visual Studio.

Can mingw dynamic libraries depend on msvc DLLs?

Yes, but things can get weird here once you try to distribute those DLLs. It's fairly common knowledge that software built with MSVC will rely on its runtime, so it's on the user to distribute that runtime. I noticed librdkafka.redist actually ships the runtime DLLs directly in the NuGet package. I'm not positive whether this complies with the runtime's license, but the vast majority of Windows software ships with the version of Microsoft's Visual C++ redistributable installer matching the compiler (https://support.microsoft.com/en-us/help/2977003/the-latest-supported-visual-c-downloads).
The same restrictions will apply to your MinGW dynamic library if it depends on libraries built with MSVC (newer than v6.0). Everything will build fine, but silently fail at runtime on the client machine if you forget to distribute the Visual C++ runtime.

neptoess · 2020-04-28T13:39:32Z

@edenhill

Can you educate me on when and how the MinGW builds are useful, compared to the msvc nuget package? I'm not questioning the need, just want to understand since my knowledge of Windows runtimes are so-and-so.

The big benefit for me is, same as @ed-alertedh , being able to use rdkafka.h with MinGW, which allows me to use confluent-kafka-go on Windows (cgo doesn't support Microsoft's compilers). However, benefits for the overall project might be:

No need to maintain a separate Visual Studio build for the C library (which right now is stuck on VS2013 and can't use mainline OpenSSL I believe).
Opens up the possibility of static linking on Windows (which has huge potential for streamlining confluent-kafka-go on Windows)
Lets us take advantage of the MSYS2 distribution's packages for pulling in dependencies like OpenSSL
librdkafka.redist can stop shipping libzstd.dll and zlib.dll
Allows the MSYS2 community to create a librdkafka package, so users can develop Windows programs that use Kafka without Visual Studio.

There would be more, but the C++ part will have to be built by the same compiler the end user plans on using, so we'll have to keep Visual Studio around for that. However, if the only thing we need Visual Studio to build is the C++ code, we could consider removing C++ support from librdkafka.redist, and migrating it to vcpkg, which would be much easier to maintain as new Visual Studio versions are released.

ed-alertedh · 2020-04-29T05:20:43Z

I don't think I have much to add to @neptoess 's answers to the questions. I'll admit I wasn't aware that MinGW happens to be link-compatible with MSVC for static C libs, but I agree that it seems safer not to mix compilers when linking statically. I'm not sure if I would recommend switching your main Windows build over to MinGW given you still need VS to build the C++ lib (as pointed out, MinGW and MSVC are not ABI compatible for C++).

I've fixed that build error now. I was "chasing ghosts" trying to turn caching on and off. The actual fix was to patch another instance of _MSC_VER that was recently introduced on master (I forgot that the PR builds run after merging with master).

edenhill · 2020-04-29T06:04:04Z

Thank you both for the answers, getting a static win32 lib into confluent-kafka-go would be huge.

edenhill

LGTM! Is this ready to be merged?

ed-alertedh · 2020-04-29T06:22:20Z

Yep, ready to merge!

edenhill · 2020-04-29T06:25:14Z

Superb, thank you!

ed-alertedh · 2020-04-29T06:31:16Z

Likewise, thanks! Also, thanks for your reviews and your input @neptoess

edenhill · 2020-05-19T10:44:40Z

We're seeing recurring mingw installation failures on travis:
https://travis-ci.org/github/edenhill/librdkafka/jobs/688737080

@ed-alertedh Can you take a look?

neptoess · 2020-05-19T13:19:42Z

@edenhill / @ed-alertedh ,
Looks like we're not the only ones running into this https://chocolatey.org/packages/msys2#discussion
I'm thinking we can get away with adding pacman -Sy --noconfirm pacman before the choco upgrade in travis-before-install.sh, i.e. change

        choco uninstall -y mingw
        choco upgrade --no-progress -y msys2
        export msys2='cmd //C RefreshEnv.cmd '
        export msys2+='& set MSYS=winsymlinks:nativestrict '
        export msys2+='& C:\\tools\\msys64\\msys2_shell.cmd -defterm -no-start'
        export mingw64="$msys2 -mingw64 -full-path -here -c "\"\$@"\" --"
        export msys2+=" -msys2 -c "\"\$@"\" --"
        $msys2 pacman --sync --noconfirm --needed mingw-w64-x86_64-toolchain mingw-w64-x86_64-cmake mingw-w64-x86_64-openssl mingw-w64-x86_64-cyrus-sasl

to

        choco uninstall -y mingw
        export msys2='cmd //C RefreshEnv.cmd '
        export msys2+='& set MSYS=winsymlinks:nativestrict '
        export msys2+='& C:\\tools\\msys64\\msys2_shell.cmd -defterm -no-start'
        export mingw64="$msys2 -mingw64 -full-path -here -c "\"\$@"\" --"
        export msys2+=" -msys2 -c "\"\$@"\" --"
        $msys2 pacman -Sy --noconfirm pacman
        choco upgrade --no-progress -y msys2
        $msys2 pacman --sync --noconfirm --needed mingw-w64-x86_64-toolchain mingw-w64-x86_64-cmake mingw-w64-x86_64-openssl mingw-w64-x86_64-cyrus-sasl

edenhill · 2020-05-19T13:25:21Z

Sounds good, wanna PR?

neptoess · 2020-05-19T13:38:18Z

#2892

ed-alertedh · 2020-05-20T00:16:35Z

Looks like you resolved it while I was asleep, happy to help in future if needed though!

neptoess mentioned this pull request Jan 6, 2020

Use _WIN32 preprocessor macro instead of _MSC_VER #2670

Closed

edenhill suggested changes Feb 5, 2020

View reviewed changes

src/tinycthread_extra.h Outdated Show resolved Hide resolved

examples/rdkafka_complex_consumer_example.cpp Outdated Show resolved Hide resolved

src/rdatomic.h Outdated Show resolved Hide resolved

src/rdatomic.h Outdated Show resolved Hide resolved

neptoess reviewed Apr 21, 2020

View reviewed changes

neptoess reviewed Apr 22, 2020

View reviewed changes

edenhill suggested changes Apr 23, 2020

View reviewed changes

Add build configuration for MinGW-w64. Primarily just needed to use _…

81b37a0

…WIN32 instead of _MSC_VER in header guards. Various other small patches included to account for compiler differences.

ed-alertedh force-pushed the master branch from cf0d242 to 81b37a0 Compare April 24, 2020 04:03

edenhill suggested changes Apr 27, 2020

View reviewed changes

edbordin added 2 commits April 28, 2020 12:21

address review comments - try to use declaration of struct timeval fr…

044f033

…om sys/time.h instead of winsock2.h for mingw

try alternative win32 datetime format method avoiding ATL (which isn'…

e192c56

…t available in MinGW)

fix bug introduced to example when dividing milliseconds by 1000

e3a988d

try disable cache

77e0cb9

edenhill reviewed Apr 28, 2020

View reviewed changes

edbordin added 2 commits April 28, 2020 17:44

really disable the cache this time

f13da6a

put it back how it was before I forget

c0a6651

edbordin added 5 commits April 29, 2020 10:04

try again with no cache

6dcbe87

try touching test.c to invalidate cache if there is one

7bdea4d

Merge remote-tracking branch 'upstream/master'

b2fdcb7

update patch to work with latest master

c2c742b

revert whitespace

e2567e2

edenhill approved these changes Apr 29, 2020

View reviewed changes

edenhill merged commit 5f1fc42 into confluentinc:master Apr 29, 2020

edenhill mentioned this pull request May 4, 2020

How Can I use librdkafka in windows with Qt and mingw ? #2817

Closed

7 tasks

	- $mingw64 cmake -DCMAKE_MAKE_PROGRAM=mingw32-make -G "MSYS Makefiles" -DCMAKE_TOOLCHAIN_FILE=./packaging/mingw-w64/msys2_mingw_toolchain.cmake \
	- $mingw64 cmake -DCMAKE_MAKE_PROGRAM=mingw32-make -G "MinGW Makefiles" -DCMAKE_TOOLCHAIN_FILE=./packaging/mingw-w64/msys2_mingw_toolchain.cmake \

Patches for MinGW-w64 #2553

Patches for MinGW-w64 #2553

Conversation

ed-alertedh commented Oct 7, 2019 • edited Loading

edenhill commented Oct 7, 2019

ed-alertedh commented Oct 8, 2019

neptoess commented Jan 6, 2020

ed-alertedh commented Jan 6, 2020

neptoess commented Jan 7, 2020

ed-alertedh commented Jan 7, 2020

edenhill left a comment

Choose a reason for hiding this comment

ed-alertedh commented Feb 6, 2020

edenhill commented Feb 6, 2020

ed-alertedh commented Feb 7, 2020

ed-alertedh commented Feb 11, 2020

neptoess commented Apr 10, 2020 • edited Loading

edenhill commented Apr 14, 2020

edenhill commented Apr 14, 2020

ed-alertedh commented Apr 21, 2020

neptoess commented Apr 21, 2020

ed-alertedh commented Apr 21, 2020 • edited Loading

edenhill commented Apr 21, 2020

neptoess Apr 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neptoess Apr 22, 2020 • edited Loading

Choose a reason for hiding this comment

edenhill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neptoess Apr 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ed-alertedh Apr 23, 2020 • edited Loading

Choose a reason for hiding this comment

ed-alertedh commented Apr 23, 2020

neptoess commented Apr 23, 2020

ed-alertedh commented Apr 24, 2020

edenhill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ed-alertedh commented Apr 28, 2020

edenhill commented Apr 28, 2020

Choose a reason for hiding this comment

edenhill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ed-alertedh commented Apr 28, 2020 • edited Loading

edenhill commented Apr 28, 2020

neptoess commented Apr 28, 2020 • edited Loading

neptoess commented Apr 28, 2020 • edited Loading

ed-alertedh commented Apr 29, 2020 • edited Loading

edenhill commented Apr 29, 2020

edenhill left a comment

Choose a reason for hiding this comment

ed-alertedh commented Apr 29, 2020

edenhill commented Apr 29, 2020

ed-alertedh commented Apr 29, 2020

edenhill commented May 19, 2020

neptoess commented May 19, 2020

edenhill commented May 19, 2020

neptoess commented May 19, 2020

ed-alertedh commented Oct 7, 2019 •

edited

Loading

neptoess commented Apr 10, 2020 •

edited

Loading

ed-alertedh commented Apr 21, 2020 •

edited

Loading

neptoess Apr 21, 2020 •

edited

Loading

neptoess Apr 22, 2020 •

edited

Loading

neptoess Apr 23, 2020 •

edited

Loading

ed-alertedh Apr 23, 2020 •

edited

Loading

ed-alertedh commented Apr 28, 2020 •

edited

Loading

neptoess commented Apr 28, 2020 •

edited

Loading

neptoess commented Apr 28, 2020 •

edited

Loading

ed-alertedh commented Apr 29, 2020 •

edited

Loading