Allow building with MSVC, behind an opt-in flag - #2136
Conversation
__attribute__((format(printf,...))), format_arg and warn_unused_result are used directly across the tree, and inconsistently: i18n.h already guarded some of its declarations and not others. There is no MSVC spelling for any of them, so a compiler without them cannot compile these headers at all. Add core/compiler.h with CORE_FORMAT_PRINTF, CORE_FORMAT_ARG and CORE_WARN_UNUSED_RESULT, which expand to the attribute on GCC and Clang and to nothing elsewhere, and use them at the existing sites. No warning is lost on the compilers that had them.
MinGW supplies these transparently, so the tree calls them unguarded. MSVC supplies none of them: gettimeofday(), strcasecmp()/strncasecmp(), PATH_MAX, mode_t, and dirname() -- there is no <libgen.h> at all. Add core/os.h with the minimal shims (gettimeofday via GetSystemTimeAsFileTime, the string comparisons via _stricmp/_strnicmp, PATH_MAX as _MAX_PATH) and include it where those names are actually used. Two related fixes come along, because they are the same problem: <unistd.h> was included unconditionally by TcpSocket.cxx and vncviewer.cxx, and <libgen.h> by ServerDialog.cxx, none of which exist on MSVC; vncviewer.cxx also called _open_osfhandle() without <io.h>. And xdgdirs' own compatibility macro expanded mkdir(path, mode) to mkdir(path), a symbol MinGW has and MSVC does not -- it needs _mkdir from <direct.h>.
<sys/time.h> is not a Windows header. MinGW ships a shim, which is why these direct includes have never been a problem; without one, struct timeval comes from <winsock2.h> instead. Guard each include accordingly. FdInStream.cxx is the odd one out: its <sys/time.h> sat above the platform #ifdef rather than inside it, unlike its FdOutStream.cxx sibling, so the include is moved into the existing #else branch rather than wrapped.
usage() puts #ifndef WIN32 / #endif inside the argument list of the _() macro. GCC and Clang accept that as an extension, but the standard leaves a directive spanning a macro invocation undefined and MSVC refuses it. Write the two message variants out in full instead. DesktopWindow.cxx uses the alternative operator tokens (and, not). They are standard C++, but MSVC only recognises them via <ciso646>, where GCC and Clang know them in the lexer. Including it is a no-op on the compilers that do not need it.
core/i18n.h redirects the rest of the tree into this file's wrappers with "#define dgettext dgettext_rfb" and friends. dgettext_rfb() then tries to reach the real function underneath with its own #undef -- but #undef only removes the current definition, it cannot restore whatever the name meant before. With ENABLE_NLS on, that happens to work: libintl's dgettext is a genuine extern function, and #undef does not touch functions. With ENABLE_NLS off, gettext.h's fallback is itself a macro, so the #undef destroys it and dgettext_rfb() ends up calling an identifier that is not declared anywhere. Implement the disabled-NLS pass-through semantics directly rather than routing through a macro that is already gone. This is not compiler specific; it only shows up because almost nobody configures with NLS genuinely off.
Recent Windows SDKs only declare CLSID_VideoProcessorMFT in <mfidl.h> when building against WINVER < _WIN32_WINNT_WINTHRESHOLD; above that the declaration is gone, even though the GUID data still ships in wmcodecdspuuid.lib for compatibility. check_variable_exists() tests whether the symbol links, not whether the header declares it, so it reports the symbol found and suppresses this file's own fallback declaration -- leaving the identifier undeclared at compile time. The probe cannot answer the question being asked, so remove it and declare a distinctly named constant unconditionally. The rename is what makes that safe: an unconditional CLSID_VideoProcessorMFT of our own would collide wherever the SDK does still declare it.
add_subdirectory(win) and add_subdirectory(unix) run unconditionally, so there is no way to build only the viewer. That matters on Windows, where rfb_win32 has portability problems of its own that a viewer-only build has no reason to care about. Defaults to ON, alongside the existing BUILD_VIEWER and BUILD_JAVA, so no existing build changes.
With the portability work in the preceding commits in place, relax the
outright refusal to configure under MSVC into an opt-in,
-DALLOW_MSVC_EXPERIMENTAL=ON, and fix the last few things that are wrong
once cl.exe is actually allowed to run.
The GNU-only flag spellings (-Og, -std=gnu99/gnu++11, -Wall -Wextra
-Wformat=2 -Wvla and friends, _FORTIFY_SOURCE) were applied
unconditionally; cl.exe rejects them outright ("D8021: invalid numeric
argument '/Wextra'") or collides with its own defaults ("D8016: '/RTC1'
and '/Og' command-line options are incompatible"). They are now behind
if(NOT MSVC), which also covers the -Wno-error handed to the FLTK version
probe. Most of that hunk is the re-indent this forces; git diff -w shows
the real change is small.
The NOMINMAX definition named the rfb target roughly 340 lines before
add_subdirectory(common) creates it, so target_compile_definitions() there
could only ever have failed. It has never run, because the hard MSVC gate
above aborted first. Make it a global add_compile_definitions() instead.
WIN32_LEAN_AND_MEAN is new. MinGW's own <windows.h> defines it by default,
so on that toolchain nothing pulled in the legacy <winsock.h>; under MSVC
it does, and it then conflicts with <winsock2.h> in any translation unit
that sees both, regardless of include order.
This is deliberately last: every commit before it leaves MSVC refused, so
nothing bisects into a half-ported state, and none of them change what any
currently supported toolchain builds. It is also not a claim that Visual
Studio is supported -- it is the smallest change that lets someone try.
The MSVC opt-in only covered the client libraries and the viewer; turning BUILD_SERVER on still failed, so the option read as a workaround for an unported tree rather than a genuine choice. core/os.h gains ffs(), ssize_t and usleep(), joining the shims already there. mingw supplies all three and MSVC none of them. wm_hooks put its shared variables in a named section with a gcc attribute. MSVC has no per-variable equivalent, so the two groups are bracketed with data_seg pragmas instead. The linker directive is what makes the section genuinely shared -- without it the section still exists but each process gets its own copy, which would have been a silent behavioural difference rather than a build failure. VNCServerService.cxx wrote the calling convention before the pointer rather than on it: 'typedef void WINAPI (*SendSAS_proto)(BOOL)'. gcc accepts the transposed form; MSVC does not. vncconfig needs <commdlg.h> explicitly, because WIN32_LEAN_AND_MEAN stops <windows.h> pulling it in and mingw's headers had been supplying it transitively. SSecurityRSAAES.cxx uses inline variables, so MSVC needs /std:c++17. The GNU standard flags sit behind if(NOT MSVC), which left MSVC with no standard flag at all and its own older default. GestureHandler.cxx needs _USE_MATH_DEFINES for M_PI. Verified on Windows ARM64: a full build with BUILD_SERVER=ON produces winvnc4.exe, vncconfig.exe and wm_hooks.dll, and every unit test compiles.
979a332 to
47911ed
Compare
|
I'm afraid that I'm extremely sceptical about merging this. This is a very invasive change that affects a lot of files. For a compiler that is not used by any of the developers. You say that this is primarily for ARM. What is the current state of MinGW there? I saw that gcc got support a while ago. So I would hope that the latest versions would work just fine. |
os.h guarded its POSIX replacements on _WIN32, so mingw picked them up too and collided with the declarations it already provides: mode_t is unsigned short in pthread_compat.h, and strcasecmp, gettimeofday and dirname all exist there as well. That broke the existing mingw build at common/core/Logger_file.cxx. mingw does supply all of these for real, just from the POSIX headers rather than the ones a Windows source file would otherwise include, so include those on that side and keep the shims for MSVC only. Both halves are needed: dropping the shims without adding the includes leaves gettimeofday undeclared in common/core/Timer.cxx. PATH_MAX stays under _WIN32 as it is #ifndef-guarded and wanted by both toolchains.
abe34e2 to
7619144
Compare
|
Fair, and thanks for looking at it properly rather than just closing it. First, the CI failure on this PR is mine and it is a straightforward bug, not On mingw for ARM: you're right, and I should not overstate this. MSYS2's The main reason is a preference, not a necessity: I would rather build on That is not an argument that anything is broken today, and I recognise it is a If there is a version of this you would consider, I would guess it is a much |
2 participants
TigerVNC currently refuses to configure under MSVC with a hard
FATAL_ERROR. This series makes the tree actually compile withcl.exe,then relaxes that refusal into an opt-in —
-DALLOW_MSVC_EXPERIMENTAL=ON.Without the flag, nothing changes for any currently supported toolchain.
The motivation is Windows on ARM64, where MinGW is awkward to obtain and
MSVC is the native toolchain. Most of the work is not ARM-specific though —
it is the set of things MinGW provides transparently and MSVC does not.
What is in it
core/compiler.h— portable macros for the GCC-only function attributes(
format(printf),format_arg,warn_unused_result), which were useddirectly and inconsistently across the tree.
core/os.h— minimal shims for the POSIX interfaces MSVC lacks:gettimeofday,strcasecmp/strncasecmp,PATH_MAX,mode_t,dirname. There is no<libgen.h>on MSVC at all.struct timevaltaken from<winsock2.h>on Windows, since<sys/time.h>is not a Windows header and MinGW's shim is what has beenhiding that.
a macro argument list, and the alternative operator tokens without
<ciso646>.BUILD_SERVERoption, defaultingON, so a viewer-only build ispossible.
add_subdirectory(win)/(unix)currently run unconditionally.CLSID_VideoProcessorMFT: thecheck_variable_exists()probe testswhether the symbol links, not whether the header declares it, so it
reported success and suppressed the file's own fallback declaration.
if(NOT MSVC), and theNOMINMAXdefinition — which named therfbtarget ~340 lines before
add_subdirectory(common)creates it, so itcould only ever have failed silently.
One of these is not MSVC-specific
dgettext_rfb()and friends try to reach the real function with#undef,but
#undefcannot restore what a name meant before. WithENABLE_NLSonthat happens to work, because libintl's
dgettextis a real function. WithNLS off,
gettext.h's fallback is itself a macro, so the#undefdestroysit and the wrapper calls an identifier that is not declared anywhere. It
only shows up because almost nobody configures with NLS genuinely off.
Happy to split that one out if you would rather take it on its own.
Ordering
The opt-in is deliberately last. Every commit before it leaves MSVC
refused, so a bisect cannot land in a half-ported state, and none of them
changes what any currently supported toolchain builds.
The Windows server builds too
BUILD_SERVER=ONunder MSVC needed six more things, all small and all inthe last commit:
ffs/ssize_t/usleepjoining thecore/os.hshims;wm_hooks' shared data section expressed withdata_segpragmas plus thelinker directive that actually makes it shared; a
typedefinVNCServerService.cxxthat wrote the calling convention before the pointerrather than on it;
<commdlg.h>forvncconfig, whichWIN32_LEAN_AND_MEANstops<windows.h>supplying;/std:c++17, sincethe GNU standard flags sit behind
if(NOT MSVC)and MSVC was left with itsown older default; and
_USE_MATH_DEFINESforM_PI.The
wm_hooksone is the only behavioural change worth flagging: withoutthe linker directive the section still exists but every process gets its
own copy, so it would have been a silent difference rather than a build
failure.
Testing
Built on Windows ARM64 with Visual Studio 17 2022 against vcpkg
(
arm64-windows). A full build withBUILD_SERVER=ONcompletes andproduces
winvnc4.exe,vncconfig.exeandwm_hooks.dll; the viewerbuilds against FLTK 1.3.11; and the unit tests compile, with the seven
toolkit-independent ones (
configargs,conv,convertlf,hostport,parameters,pixelformat,unicode) run and passing.I have not built this on Linux, macOS or MinGW — the changes are
guarded so those paths should be unaffected, but that is reasoning, not a
test, and CI will say more than I can.
This is not a claim that Visual Studio is supported. It is the smallest
change that lets someone try.