Support mingw-w64 (from msys2) on Windows. #1570

sighingnow · 2021-01-23T17:55:20Z

Supporting for MinGW on Windows is a long-standling feature request for the cpprestsdk project. This PR makes the codebase compilable with mingw-w64 from msys2, include main library, test cases and examples.

At the same time, this PR has minimized the changeset for the convenience of code review.

Resolves #202, #1361 and #1541.

Signed-off-by: Tao He <[email protected]>

sighingnow · 2021-01-23T17:56:45Z

/cc @barcharcraz.

barcharcraz

I took a quick look at this, and I'm a bit concerned about how many new preprocessor branches are added. I'm also concerned that you are conditionally avoiding calling functions that should be available on mingw-w64, as they are in the UCRT and the legacy CRT.

barcharcraz · 2021-01-28T23:19:15Z

Release/CMakeLists.txt

+  # on mingw there are tons of warnings.
+  set(WERROR OFF CACHE BOOL "Treat Warnings as Errors.")


let's keep WERROR on, and either address the warnings or tell people to turn it off.

Addressing the warnings requires a lot changes in the codebase. Could I print a CMake warning if users are working with MinGW and WERROR is ON?

yeah a warning is fine, I just don't like setting werror based on platform

barcharcraz · 2021-01-28T23:21:11Z

Release/CMakeLists.txt

+SET(DEFAULT_BUILD_TYPE "Release")
+if(NOT CMAKE_BUILD_TYPE AND NOT CMAKE_CONFIGURATION_TYPES)
+  message(STATUS "Setting build type to '${DEFAULT_BUILD_TYPE}' as none was specified.")
+  set(CMAKE_BUILD_TYPE "${DEFAULT_BUILD_TYPE}" CACHE STRING "Choose the type of build." FORCE)
+  set_property(CACHE CMAKE_BUILD_TYPE PROPERTY STRINGS "Debug" "Release" "MinSizeRel" "RelWithDebInfo")
+endif()


don't change the build type like this, just print a warning during configuration if the build isn't likely to work.

barcharcraz · 2021-01-28T23:21:40Z

Release/CMakeLists.txt

+#
+# see also: https://stackoverflow.com/questions/14125007/gcc-string-table-overflow-error-during-compilation
+if(MINGW AND CMAKE_BUILD_TYPE STREQUAL "Debug")
+  message(FATAL_ERROR "mingw-w64 cannot build the debug version of this library")


this is a forward compat issue, just warn.

Will fixes that by using a warning.

barcharcraz · 2021-01-28T23:23:47Z

Release/include/cpprest/details/basic_types.h

-#ifdef _WIN32
+#if defined(_WIN32)


driveby change

barcharcraz · 2021-01-28T23:25:24Z

Release/include/cpprest/details/cpprest_compat.h

@@ -69,7 +69,7 @@
 #endif // __clang__
 #endif // _WIN32

-#ifdef _NO_ASYNCRTIMP
+#if defined(_NO_ASYNCRTIMP) || defined(__MINGW32__)


just force _NO_ASYNCRTIMP on mingw, don't special case the feature here.

oh, actually this is a visibility macro, so I'm not sure this is even right, even on mingw if you're building a shared library I think you need __declspec(dllexport) (attribute((visibility)) may work as well)

If I remember it correctly mingw-w64 (gcc) exports all symbol by default, just like what happens to the shared library on Linux.

But I would restore the dllexport behaviour on mingw-w64 by good.

barcharcraz · 2021-01-28T23:41:29Z

Release/src/json/json_parsing.cpp

+#if defined(__MINGW32__)
+    // link error on mingw-w64: undefined reference to `__imp__wcstod_l'
+    return wcstod(str, nullptr);


_wcstod_l should be available in mingw

barcharcraz · 2021-01-28T23:43:51Z

Release/src/pch/stdafx.h

-#else // LINUX or APPLE
+#endif // _WIN32
+
+#if !defined(_WIN32) || defined(__MINGW32__) // LINUX, APPLE or MinGW


should probably either be based on GCC or on detecting libstdc++

barcharcraz · 2021-01-28T23:45:03Z

Release/src/streams/fileio_posix.cpp

+bool _open_fsb_str(_filestream_callback* callback, const wchar_t* filename, std::ios_base::openmode mode, int prot) {
+    return _open_fsb_str(callback, utility::conversions::to_utf8string(filename).c_str(), mode, prot);
+}


I don't understand why these wide functions are required for mingw but somehow not for msvc with /UNICODE

barcharcraz · 2021-01-28T23:55:02Z

Release/tests/common/TestRunner/test_runner.cpp

+#if !defined(__MINGW32__)
    _CrtSetReportMode(_CRT_ASSERT, _CRTDBG_MODE_FILE | _CRTDBG_MODE_WNDW | _CRTDBG_MODE_DEBUG);
    _CrtSetReportFile(_CRT_ASSERT, _CRTDBG_FILE_STDERR);
    _CrtSetReportMode(_CRT_ERROR, _CRTDBG_MODE_FILE | _CRTDBG_MODE_WNDW | _CRTDBG_MODE_DEBUG);
    _CrtSetReportFile(_CRT_ERROR, _CRTDBG_FILE_STDERR);
    _CrtSetReportMode(_CRT_WARN, _CRTDBG_MODE_FILE | _CRTDBG_MODE_DEBUG);
    _CrtSetReportFile(_CRT_WARN, _CRTDBG_FILE_STDERR);
+#endif // !defined(__MINGW32__)


I think we can just do _MSC_VER here, or perhaps _UCRT

Thanks, will fix.

barcharcraz · 2021-01-28T23:59:57Z

Release/src/json/json_serialization.cpp

-
+#if defined(__MINGW32__)
+        const auto numChars = strnlen(tempBuffer, tempSize);
+#else
        const auto numChars = strnlen_s(tempBuffer, tempSize);


strnlen_s should be available in mingw (but not msys)

I will double check that, but I remember here is an compile error.

sighingnow · 2021-01-31T05:37:09Z

I took a quick look at this, and I'm a bit concerned about how many new preprocessor branches are added. I'm also concerned that you are conditionally avoiding calling functions that should be available on mingw-w64, as they are in the UCRT and the legacy CRT.

@barcharcraz Thanks for reviewing this PR! I will revise that as soon as my Windows laptop back.

ysangkok · 2021-04-29T20:54:54Z

Got this error during "make -j":

[ 98%] Linking CXX shared library ../../../../Binaries/libhttplistener_test.dll
C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/10.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: CMakeFiles/httplistener_test.dir/objects.a(building_response_tests.cpp.obj):building_response_tests.cpp:(.rdata$.refptr._ZN3web4http12status_codes2OKE[.refptr._ZN3web4http12status_codes2OKE]+0x0):
undefined reference to `web::http::status_codes::OK'

Lord-Kamina · 2022-08-30T14:01:31Z

I guess this PR is dead?

sighingnow · 2022-09-01T01:50:19Z

I guess this PR is dead?

Not dead yet. Will address the review comments in this Sep.

sighingnow · 2023-02-19T10:02:50Z

Closing. Will reopen when I come back to this project.

Support mingw-w64 (from msys2) on Windows.

0967e8e

Signed-off-by: Tao He <[email protected]>

sighingnow mentioned this pull request Jan 23, 2021

How to build cpprestsdk with MinGW on windows? #1541

Open

barcharcraz suggested changes Jan 29, 2021

View reviewed changes

sighingnow closed this Feb 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support mingw-w64 (from msys2) on Windows. #1570

Support mingw-w64 (from msys2) on Windows. #1570

sighingnow commented Jan 23, 2021 •

edited

Loading

sighingnow commented Jan 23, 2021

barcharcraz left a comment

barcharcraz Jan 28, 2021

sighingnow Jan 31, 2021

barcharcraz Feb 1, 2021

barcharcraz Jan 28, 2021

barcharcraz Jan 28, 2021

sighingnow Jan 31, 2021

barcharcraz Jan 28, 2021

barcharcraz Jan 28, 2021

barcharcraz Jan 28, 2021

sighingnow Jan 31, 2021

barcharcraz Jan 28, 2021

barcharcraz Jan 28, 2021

barcharcraz Jan 28, 2021

barcharcraz Jan 28, 2021

sighingnow Jan 31, 2021

barcharcraz Jan 28, 2021

sighingnow Jan 31, 2021

sighingnow commented Jan 31, 2021

ysangkok commented Apr 29, 2021 •

edited

Loading

Lord-Kamina commented Aug 30, 2022

sighingnow commented Sep 1, 2022

sighingnow commented Feb 19, 2023

		# on mingw there are tons of warnings.
		set(WERROR OFF CACHE BOOL "Treat Warnings as Errors.")

		#ifdef _WIN32
		#if defined(_WIN32)

Support mingw-w64 (from msys2) on Windows. #1570

Support mingw-w64 (from msys2) on Windows. #1570

Conversation

sighingnow commented Jan 23, 2021 • edited Loading

sighingnow commented Jan 23, 2021

barcharcraz 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

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

sighingnow commented Jan 31, 2021

ysangkok commented Apr 29, 2021 • edited Loading

Lord-Kamina commented Aug 30, 2022

sighingnow commented Sep 1, 2022

sighingnow commented Feb 19, 2023

sighingnow commented Jan 23, 2021 •

edited

Loading

ysangkok commented Apr 29, 2021 •

edited

Loading