Repository navigation
Added Mingw support - #1652
Added Mingw support#1652TracerDS wants to merge 5 commits into
Conversation
✅ Deploy Preview for dpp-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
🟡 Changes recommended
Five unresolved findings remain, including two critical issues affecting Windows/MSYS2 builds.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds MinGW/MSYS2 support, including UCRT64 and CLANG64, through centralized platform detection, Windows/networking updates, CMake integration, and documentation.
Changes:
- Adds platform and compiler detection macros.
- Updates socket, SSL, compatibility, and CMake handling.
- Documents MSYS2 build configurations.
File summaries
| File | Changes | Final review note |
|---|---|---|
src/unittest/test.h |
Uses centralized platform/compiler checks. | Critical (3 votes): DPP_USE_MSVC and DPP_USE_WINDOWS are tested before DPP headers are included, causing Windows unit tests to select libdpp.so. |
src/sockettest/socket.cpp |
Adjusts Windows socket setup. | No final comment. |
src/dpp/voice/enabled/enabled.h |
Updates Windows detection. | No final comment. |
src/dpp/utility.cpp |
Updates Windows C runtime handling. | No final comment. |
src/dpp/sslconnection.cpp |
Updates platform-specific socket logic. | No final comment. |
src/dpp/ssl_context.cpp |
Updates Windows certificate handling. | No final comment. |
src/dpp/socketengine.cpp |
Updates Winsock cleanup detection. | No final comment. |
src/dpp/socket.cpp |
Removes a redundant template instantiation. | No final comment. |
src/dpp/queues.cpp |
Updates Windows I/O includes. | No final comment. |
src/dpp/httpsclient.cpp |
Uses the invalid-socket sentinel. | No final comment. |
src/dpp/discordevents.cpp |
Updates time conversion detection. | No final comment. |
src/dpp/dave/persisted_key_pair_generic.cpp |
Updates platform-specific file handling. | Critical (3 votes): DPP_USE_WINDOWS is evaluated before DPP headers, causing Windows and MSYS2 builds to include unistd.h instead of the required Windows declarations. |
src/dpp/cluster.cpp |
Updates Windows configuration validation. | No final comment. |
library/CMakeLists.txt |
Adds MinGW detection and dependency handling. | Moderate (3 votes): The new DPP_MINGW fallback is not used consistently with existing MINGW checks, potentially selecting MSVC libraries and omitting MinGW libraries. |
include/dpp/win32_safe_warnings.h |
Limits warning pragmas to MSVC. | No final comment. |
include/dpp/system.h |
Adds platform/compiler detection. | Moderate (3 votes): Apple toolchains match the generic Unix branch first, so DPP_USE_MACOS is not defined and macOS is reported as Unix. |
include/dpp/sysdep.h |
Uses centralized platform guards. | No final comment. |
include/dpp/sslconnection.h |
Uses a platform-neutral error sentinel. | No final comment. |
include/dpp/socket.h |
Makes socket types platform-aware. | No final comment. |
include/dpp/socket_listener.h |
Fixes invalid socket checking. | No final comment. |
include/dpp/nlohmann/json.hpp |
Adds platform/compiler metadata. | Moderate (2 votes): DPP macros may be undefined when the public JSON header is included first, causing json::meta() to report unknown platform and compiler values. |
include/dpp/message.h |
Removes enum export decoration. | No final comment. |
include/dpp/export.h |
Integrates system detection. | No final comment. |
include/dpp/dns.h |
Updates socket includes. | No final comment. |
include/dpp/discordvoiceclient.h |
Updates Windows defaults. | No final comment. |
include/dpp/compat.h |
Adds MinGW-compatible Winsock handling. | No final comment. |
docpages/building/windows.md |
Documents MSYS2 builds. | No final comment. |
Review details
- Files reviewed: 27/27 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
this pr is way too big. it touches so much stuff. how can we know it doesn't break something, it's diving in changing functionality outside the build system, renaming variables, moving includes. is it tested? does it have working ci? secondly, who is going to support it after it's merged? I for one don't really like mingw and it is another target to support, test in ci, run unit tests on. thirdly who added all these ai reviews, copilot is NOT an authorised reviewer of dpp! |
braindigitalis
left a comment
There was a problem hiding this comment.
this pr is far too big in its current form and changes too many things outside the build system.
we already support proper native windows builds so mingw as a crutch is not needed.
|
|
||
| template bool raii_socket::set_option<int>(int, int, int); | ||
| template bool raii_socket::set_option<bool>(int, int, bool); | ||
| template bool raii_socket::set_option<socklen_t>(int, int, socklen_t); |
There was a problem hiding this comment.
It causes issues as socklen_t = int on mingw toolchain.
So it causes redefinition
template bool raii_socket::set_option<int>(int, int, int);
template bool raii_socket::set_option<bool>(int, int, bool);
template bool raii_socket::set_option<int>(int, int, int);There was a problem hiding this comment.
So you need to remove the int specialization then, not the socklen_t specialization.
There was a problem hiding this comment.
sure, either one will work.
The codebase uses explicit int template, e.g.
include/dpp/socket_listener.h
fd.set_option<int>(SOL_SOCKET, SO_REUSEADDR, 1);extern template DPP_EXPORT bool raii_socket::set_option<int>(int level, int name, int value);
It doesnt change any funcitonality, only CMakeLists is modified and made definitions rely on compiler macros. Basically the same thing as it was before but with more detections.
Its working fine.
You dont run unit tests to check compilers. You write unit tests to check the code. It will naturally be supported as no changes touched the actual codebase.
Its been added by default. Your repo, your settings 🙄
MSVC only is not a good support choice. Clang on windows works dubiously if at all. |
- Removed changes in the thirdparty lib - Removed useless change in `current_date_time` - Improved Mingw detection in `library/CMakeLists` - Fixed possible macro issues
|
@braindigitalis See discord for discussion on Copilot, seems they've broken it again, not the fault of this review (from what I understand). |
What are you talking about? We literally test clang-cl as part of our CI/CD pipeline. |
Mishura4
left a comment
There was a problem hiding this comment.
We will not officially support MinGW. None of the maintainers use it as there is almost no reason to with MSVC and clang-cl. We would have to be careful that none of our changes break it, and that's not something we can or are willing to do.
With that said I don't mind a PR that fixes it for now, but then we cannot have a tutorial that explains how to build it in MinGW, as we cannot officially support it. If it breaks in the future, then it breaks, and we can't care.
As stated above there is nothing to actually support. As long as the include checks compare the system macros ( |
|
You have been given reasons as to why we will not officially support MinGW, and you have chosen to ignore them. You have been given a proposition for a compromise, and you have chosen to ignore it as well. Well, good luck. |
??? What are you taking about? I've given reasons to support it and the only valid reason you said you won't is because current collaborators sit on msvc only. It is not my role to start supporting it or not, there is no compromise as I'm not the one that's making the decision. |
In what way is MSVC/clang-cl not a good compiler for windows? It's compliant to the WinAPI ABI, and has no issues with optimisations, linking, or building in general I've personally found MinGW to hold extensive compatibility issues with Windows especially around networking since the Windows TCP stack is different to Unix/Linux. MinGW tends to also get flagged as suspicious/PUP because they don't follow the standard PE architecture without having to add rs and rec files to the project |
mingw is nowadays fully compatible with windows. It also has no issues with optimisations, linking or building in general. In fact, mingw is a better candidate for building stuff because it is basically a posix shell on windows so backporting anything non-windows to windows is trivial. It also compiles a fully native windows PE exec. So if they get flagged, we got one reason out of the picture. Resource files are also part of the windows resource mechanism, not a required thing to be conformant with PE execs. MSVC is notoriously known for not following the standard, it requires you to have the exact same toolchain to work, you cant crosscompile. The networking stack isnt what you say it is really. Windows exposes a bsd api for sockets (and thus, networking) to be more or less compatible with earlier iterations of berkeley's sockets (the api all networking on linux relies on). It has its own winapi for networking but the general backbone was still dependent on the old sockets behaviour. Sure, the differences in how each system does networking is different. But its trivial to separate these issues and fix them especially with the knowledge and libraries we have in 2026. Don't take it as an attack for MSVC and saying how MSVC is a terrible compiler and to never ever ever use it. |
[ Please outline your change here. Be sure to check the checkboxes below. ]
The current version of DPP does not support mingw environments (ucrt64, clang64).
This PR adds this support.
Code change checklist