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

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

Merged
merged 3 commits into from
Oct 16, 2023
Merged

Conversation

zm1060
Copy link
Contributor

@zm1060 zm1060 commented Oct 16, 2023

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

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

zm1060 commented Oct 16, 2023

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);

Copy link
Member

@dmah42 dmah42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@LebedevRI
Copy link
Collaborator

LebedevRI commented Oct 16, 2023

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

zm1060 commented Oct 16, 2023

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

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 LebedevRI changed the title Fix error: 'WC_ERR_INVALID_CHARS' was not declared in this scope Fix building on MinGW: default WINVER is too old Oct 16, 2023
@LebedevRI LebedevRI merged commit ea3c3f9 into google:main Oct 16, 2023
53 of 60 checks passed
@LebedevRI
Copy link
Collaborator

@zm1060 thank you!

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

Successfully merging this pull request may close these issues.

3 participants