nanomsg / nng

nanomsg-next-generation -- light-weight brokerless messaging
https://nng.nanomsg.org
MIT License
3.86k stars 493 forks source link

MinGW build problems #1878

Open gbburkhardt opened 1 month ago

gbburkhardt commented 1 month ago

I ran into 3 problems building on MinGW:

1) The 'mbed' libraries require libbcrypt and libws2_32 to link, at least when a static version of libmbedtls, et al, is used. Without those two libraries, the 'cmake' build of the makefiles fails when trying to determine the version of libmbedtls. My fix was

diff --git a/src/supplemental/tls/mbedtls/CMakeLists.txt b/src/supplemental/tls/mbedtls/CMakeLists.txt
index 27cf8927..016a0550 100644
--- a/src/supplemental/tls/mbedtls/CMakeLists.txt
+++ b/src/supplemental/tls/mbedtls/CMakeLists.txt
@@ -37,6 +37,6 @@ if (NNG_TLS_ENGINE STREQUAL "mbed")
         else()
             find_package(MbedTLS REQUIRED)
         endif()
-        nng_link_libraries_public(MbedTLS::mbedtls MbedTLS::mbedcrypto MbedTLS::mbedx509)
+        nng_link_libraries_public(MbedTLS::mbedtls MbedTLS::mbedcrypto MbedTLS::mbedx509 bcrypt ws2_32)
     endif()
 endif()

2) Compile error for nng/src/platform/windows/win_tcpconn.c

nng/src/platform/windows/win_tcpconn.c:263:36: error: passing argument 1 of 'CancelIoEx' makes pointer from integer without a cast [-Wint-conversion]
  263 |                         CancelIoEx(s, &c->send_io.olpd);
      |                                    ^
      |                                    |
      |                                    SOCKET {aka long long unsigned int}
In file included from C:/msys64-2024/ucrt64/include/winbase.h:21,
                 from C:/msys64-2024/ucrt64/include/windows.h:70,
                 from C:/Users/glenn/msys2/nng/src/platform/windows/win_impl.h:19,
                 from C:/Users/glenn/msys2/nng/src/core/platform.h:576,
                 from C:/Users/glenn/msys2/nng/src/core/nng_impl.h:27,
                 from C:/Users/glenn/msys2/nng/src/platform/windows/win_tcpconn.c:12:
C:/msys64-2024/ucrt64/include/ioapiset.h:27:48: note: expected 'HANDLE' {aka 'void *'} but argument is of type 'SOCKET' {aka 'long long unsigned int'}
   27 |   WINBASEAPI WINBOOL WINAPI CancelIoEx (HANDLE hFile, LPOVERLAPPED lpOverlapped);
      |                                         ~~~~~~~^~~~~
C:/Users/glenn/msys2/nng/src/platform/windows/win_tcpconn.c:264:36: error: passing argument 1 of 'CancelIoEx' makes pointer from integer without a cast [-Wint-conversion]
  264 |                         CancelIoEx(s, &c->recv_io.olpd);
      |                                    ^
      |                                    |
      |                                    SOCKET {aka long long unsigned int}
C:/msys64-2024/ucrt64/include/ioapiset.h:27:48: note: expected 'HANDLE' {aka 'void *'} but argument is of type 'SOCKET' {aka 'long long unsigned int'}
   27 |   WINBASEAPI WINBOOL WINAPI CancelIoEx (HANDLE hFile, LPOVERLAPPED lpOverlapped);
      |                                         ~~~~~~~^~~~~

I'm not convinced that the CancelIoEx() function will work with a SOCKET argument. I don't see any mention of it in the MSDN doc for that function. There is a note on StackOverflow indicating that it works if WSA_FLAG_OVERLAPPED is used to create the socket.

My fix was simply to typecast to SOCKET

index 0e74ccfd..2532e431 100644
--- a/src/platform/windows/win_tcpconn.c
+++ b/src/platform/windows/win_tcpconn.c
@@ -260,8 +260,8 @@ tcp_close(void *arg)
                c->s      = INVALID_SOCKET;

                if (s != INVALID_SOCKET) {
-                       CancelIoEx(s, &c->send_io.olpd);
-                       CancelIoEx(s, &c->recv_io.olpd);
+                        CancelIoEx((HANDLE)s, &c->send_io.olpd);
+                        CancelIoEx((HANDLE)s, &c->recv_io.olpd);
                        shutdown(s, SD_BOTH);
                        closesocket(s);
                }

3) For the final build, libbcrypt and libws2_32 are needed.

diff --git a/src/supplemental/tls/mbedtls/CMakeLists.txt b/src/supplemental/tls/mbedtls/CMakeLists.txt
index 27cf8927..016a0550 100644
--- a/src/supplemental/tls/mbedtls/CMakeLists.txt
+++ b/src/supplemental/tls/mbedtls/CMakeLists.txt
@@ -37,6 +37,6 @@ if (NNG_TLS_ENGINE STREQUAL "mbed")
         else()
             find_package(MbedTLS REQUIRED)
         endif()
-        nng_link_libraries_public(MbedTLS::mbedtls MbedTLS::mbedcrypto MbedTLS::mbedx509)
+        nng_link_libraries_public(MbedTLS::mbedtls MbedTLS::mbedcrypto MbedTLS::mbedx509 bcrypt ws2_32)
     endif()
 endif()

Environment Details nng version: GitHub repo as of 3 Oct 2024 MSYS_NT-10.0-22631 3.4.10.x86_64 2023-12-22 10:06 UTC x86_64 Msys gcc.exe (Rev1, Built by MSYS2 project) 14.2.0 cmake version 3.28.1 mbedtls: GitHub repo as of 3 Oct 2024

Additional context build command: cmake -G Ninja -DNNG_ENABLE_TLS=ON -DNNG_TLS_ENGINE=mbed ..

I'm not sure without a lot more study of 'nng' of what the correct fixes would be. But I have a build now. And all the tests pass.

gdamore commented 1 month ago

The mbedTLS link problems are because those libraries are not expressing the need to include the relevant libraries. I don't think it's right for me to add those here for NNG. Rather they should be fixed in Mbed TLS itself.

Which version of Mbed TLS are you using?

Note also that using MinGW has always had a point of friction, because that toolchain frequently lags the official Microsoft APIs. Our efforts to support that are limited to "best effort only".

gbburkhardt commented 1 month ago

As I noted in the "Environment Details" section, I cloned Mbed TLS from its GitHub repo, and it was current as of 3 Oct. It doesn't look to me as though this is a problem with the MinGW environment:

C:/msys64-2024/ucrt64/bin/../lib/gcc/x86_64-w64-mingw32/14.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: C:/msys64-2024/usr/local/lib/libmbedcrypto.a(entropy_poll.o):entropy_poll.c:(.text+0x43): undefined reference to `BCryptGenRandom'
C:/msys64-2024/ucrt64/bin/../lib/gcc/x86_64-w64-mingw32/14.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: C:/msys64-2024/usr/local/lib/libmbedx509.a(x509_crt.o):x509_crt.c:(.text+0xd38): undefined reference to `__imp_inet_pton'
C:/msys64-2024/ucrt64/bin/../lib/gcc/x86_64-w64-mingw32/14.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: C:/msys64-2024/usr/local/lib/libmbedx509.a(x509_crt.o):x509_crt.c:(.text+0xd57): undefined reference to `__imp_inet_pton'
collect2.exe: error: ld returned 1 exit status

But I did determine that these linking problems don't occur if a shared version of the Mbed TLS library is built. The default build instructions for Mbed TLS build only the static library. Using:

make SHARED=1 WINDOWS_BUILD=1

for the Mbed TLS library, and then

cmake -G Ninja -DBUILD_SHARED_LIBS=ON -DNNG_ENABLE_TLS=ON -DNNG_TLS_ENGINE=mbed -DCMAKE_INSTALL_PREFIX=/usr/local -DMBEDTLS_ROOT=~/mbedtls/library ..

works fine. I think it's worth a note that a shared version of the Mbed TLS library should be built. That's probably true for Linux, as well.

There's still the problem listed initially as item 2 - it's not documented that CancelIoEx works with a socket pointer for a HANDLE, AFAIK.

gdamore commented 1 month ago

As far as CancelIoEx, it works. And if it doesn't we have nothing else we can do. There isn't any other API available to us.

gdamore commented 1 month ago

WRT to the link libraries -- the Mbed package is meant to provide a definition of the targets that should include a list of any of its dependencies. That list can be different for static vs dynamic builds.

The decision as to static vs dynamic for these libraries is entirely up to the library consumer. There are valid arguments for static, and there are valid arguments for dynamic.

DoumanAsh commented 1 month ago

Just a note I was building 1.9.0 with msvc and it doesn't seem to be compiling even on native compiler?

nng\src\platform\windows\win_tcpconn.c(264,15): error: incompatible integer to pointer conversion passing 'SOCKET' (aka 'unsigned long long') to parameter of type 'HANDLE' (aka 'void *') [-Wint-conversion]
    264 |                         CancelIoEx(s, &c->recv_io.olpd);

Shouldn't there be explicit cast?

gdamore commented 1 month ago

Yeah that seems like a mistake.

DoumanAsh commented 1 month ago

I saw someone already cherry picked changes and merged it into stable branch so it is all good Not sure about other MinGW issues as I do not use it 😄