google / benchmark

A microbenchmark support library
Apache License 2.0
8.6k stars 1.57k forks source link

Fix building on MinGW: default `WINVER` is too old #1681

Closed zm1060 closed 8 months ago

zm1060 commented 8 months ago

When building leveldb, add_test(NAME "leveldb_tests" COMMAND "leveldb_tests").

FAILED: third_party/benchmark/src/CMakeFiles/benchmark.dir/sysinfo.cc.obj 
D:\MingW\bin\g++.exe -DBENCHMARK_STATIC_DEFINE -DHAVE_STD_REGEX -DHAVE_STEADY_CLOCK -DUNICODE -D_UNICODE -IF:/Contribute/leveldb/cmake-build-debug/include -IF:/Contribute/leveldb/. -IF:/Contribute/leveldb/third_party/benchmark/include -IF:/Contribute/leveldb/third_party/benchmark/src -fno-exceptions -fno-rtti  -Wall  -Wextra  -Wshadow  -Wfloat-equal  -Werror  -Wsuggest-override  -pedantic  -pedantic-errors  -fstrict-aliasing  -Wno-deprecated-declarations  -Wno-deprecated  -fno-exceptions  -Wstrict-aliasing -g -std=c++11 -fvisibility=hidden -fno-keep-inline-dllexport -fdiagnostics-color=always -MD -MT third_party/benchmark/src/CMakeFiles/benchmark.dir/sysinfo.cc.obj -MF third_party\benchmark\src\CMakeFiles\benchmark.dir\sysinfo.cc.obj.d -o third_party/benchmark/src/CMakeFiles/benchmark.dir/sysinfo.cc.obj -c F:/Contribute/leveldb/third_party/benchmark/src/sysinfo.cc
F:/Contribute/leveldb/third_party/benchmark/src/sysinfo.cc: In function 'std::__cxx11::string benchmark::{anonymous}::GetSystemName()':
F:/Contribute/leveldb/third_party/benchmark/src/sysinfo.cc:432:42: error: 'WC_ERR_INVALID_CHARS' was not declared in this scope
   int len = WideCharToMultiByte(CP_UTF8, WC_ERR_INVALID_CHARS, hostname,
                                          ^~~~~~~~~~~~~~~~~~~~
F:/Contribute/leveldb/third_party/benchmark/src/sysinfo.cc:432:42: note: suggested alternative: 'MB_ERR_INVALID_CHARS'
   int len = WideCharToMultiByte(CP_UTF8, WC_ERR_INVALID_CHARS, hostname,
                                          ^~~~~~~~~~~~~~~~~~~~
                                          MB_ERR_INVALID_CHARS
LebedevRI commented 8 months ago

This is missing some words. Why WC_ERR_INVALID_CHARS is missing? https://learn.microsoft.com/en-us/windows/win32/api/stringapiset/nf-stringapiset-widechartomultibyte mentions it. Is there a missing include perhaps?

zm1060 commented 8 months ago

These are some lines of winnls.h

#define MB_PRECOMPOSED 0x00000001
#define MB_COMPOSITE 0x00000002
#define MB_USEGLYPHCHARS 0x00000004
#define MB_ERR_INVALID_CHARS 0x00000008
#define WC_DISCARDNS 0x00000010
#define WC_SEPCHARS 0x00000020
#define WC_DEFAULTCHAR 0x00000040
#if WINVER >= 0x0600
#define WC_ERR_INVALID_CHARS 0x00000080
#endif

The update code below will work correctly!

#ifndef WC_ERR_INVALID_CHARS
    #define WC_ERR_INVALID_CHARS 0x00000080
#endif
  // `WideCharToMultiByte` returns `0` when the conversion fails.
  int len = WideCharToMultiByte(CP_UTF8, WC_ERR_INVALID_CHARS, hostname,
                                DWCOUNT, NULL, 0, NULL, NULL);
  str.resize(len);
  WideCharToMultiByte(CP_UTF8, MB_ERR_INVALID_CHARS, hostname, DWCOUNT, &str[0],
                      str.size(), NULL, NULL);
LebedevRI commented 8 months ago

if WINVER >= 0x0600

So in other words benchmark currently can not be built on windows versions older than vista, On which version are you building it on? Is MingW just messing up?

https://en.wikipedia.org/wiki/Windows_Vista says:

Vista was succeeded by [Windows 7](https://en.wikipedia.org/wiki/Windows_7) (2009), which retained and refined many of the features that Vista introduced. Microsoft ended mainstream support for Vista on April 10, 2012, and extended support on April 11, 2017.

Do we want to support building on completely EOL system?

zm1060 commented 8 months ago

this is not an appropriate fix, it's a hack. we need to understand why the definition is not available given what we expect. is the windows version wrong? what platform is the error seen on? our windows CI bots don't have this issue so there's a difference somewhere.

if WINVER >= 0x0600

So in other words benchmark currently can not be built on windows versions older than vista, On which version are you building it on? Is MingW just messing up?

https://en.wikipedia.org/wiki/Windows_Vista says:

Vista was succeeded by [Windows 7](https://en.wikipedia.org/wiki/Windows_7) (2009), which retained and refined many of the features that Vista introduced. Microsoft ended mainstream support for Vista on April 10, 2012, and extended support on April 11, 2017.

Do we want to support building on completely EOL system?

I building it on Microsoft Windows 11 Home Edition 10.0.22621.
Check: Setting WINVER or _WIN32_WINNT The problem is that there is no code that defines WINVER or uses other methods instead. However, there are multiple ways we can set it up 1.Directly in Source Code 2.Project-wide Compiler Flags 3.CMakeLists.txt 4.Visual Studio Project Settings. The first three methods require modifying the corresponding files, and the fourth method should be introduced in README.md to help with building.

LebedevRI commented 8 months ago

Ok, so mingw defaults to some lower baseline winver. https://learn.microsoft.com/en-us/cpp/porting/modifying-winver-and-win32-winnt?view=msvc-170 makes it sound like we should just do: (please change the patch to the following)

 #ifdef BENCHMARK_OS_WINDOWS
+#if !defined(WINVER) || WINVER < 0x0600
+#undef WINVER
+#define WINVER 0x0600
+#endif // WINVER handling
 #include <shlwapi.h>
 #undef StrCat  // Don't let StrCat in string_util.h be renamed to lstrcatA
 #include <versionhelpers.h>
 #include <windows.h>
LebedevRI commented 8 months ago

@zm1060 thank you!