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

MinGW build problems #1878

Open
gbburkhardt opened this issue Oct 3, 2024 · 7 comments
Open

MinGW build problems #1878

gbburkhardt opened this issue Oct 3, 2024 · 7 comments

Comments

@gbburkhardt
Copy link

gbburkhardt commented Oct 3, 2024

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()
  1. 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);
                }
  1. 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
Copy link
Contributor

gdamore commented Oct 6, 2024

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
Copy link
Author

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
Copy link
Contributor

gdamore commented Oct 6, 2024

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
Copy link
Contributor

gdamore commented Oct 6, 2024

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

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
Copy link
Contributor

gdamore commented Oct 12, 2024

Yeah that seems like a mistake.

@DoumanAsh
Copy link

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 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants