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

Unable to compile on windows Msys gcc 11.2.0/msvc 2022 pagination_range.h template overload #9111

Closed
EmotionalTristan opened this issue May 31, 2022 · 15 comments · Fixed by #9144
Assignees
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@EmotionalTristan
Copy link

When compiling on either on MSYS gcc 11.2 or with Msvc make run into over load template issue with the code in pagination_range.h - this is a bug with the msvc / gcc compiler being unable to resolve which template function to use for various classes.
The code was changed in version 1.21 and has not compiled since then.

typename StreamReader<T>::result_type GetNext() {
    if (current_ == page_.end()) {
      if (last_page_) return Status{};
      request_.set_page_token(std::move(token_));
      auto response = loader_(request_);
      if (!response.ok()) return std::move(response).status();
      token_ = ExtractPageToken(*response);
      if (token_.empty()) last_page_ = true;
      page_ = extractor_(*std::move(response));
      current_ = page_.begin();
      if (current_ == page_.end()) return Status{};
    }
    return std::move(*current_++);

template <typename U>
  static constexpr auto ExtractPageToken(U& u)
      -> decltype(std::move(*u.mutable_next_page_token())) {
    return std::move(*u.mutable_next_page_token());
  }
  template <typename U>
  static constexpr auto ExtractPageToken(U& u)
      -> decltype(std::move(u.next_page_token)) {
    return std::move(u.next_page_token);
  }

For current environment - used Msys Pacman to get Grpc / nlohmann/json and libcurl

Built crc32c
C:/msys64/mingw64/bin/cmake.exe -H. -Bbuild-output -G "MinGW Makefiles" -DCMAKE_MAKE_PROGRAM=C:/msys64/mingw64/bin/mingw32-make.exe -DCMAKE_C_COMPILER=C:/msys64/mingw64/bin/gcc.exe -DCMAKE_CXX_COMPILER=C:/msys64/mingw64/bin/g++.exe -DCMAKE_INSTALL_MESSAGE=NEVER -DCMAKE_BUILD_TYPE=Release -DBUILD_SHARED_LIBS=ON -DCMAKE_CXX_STANDARD=17 -DCRC32C_BUILD_TESTS=OFF -DCRC32C_BUILD_BENCHMARKS=OFF -DCRC32C_USE_GLOG=OFF

built google-cloud-cpp via
C:/msys64/mingw64/bin/cmake.exe -H. -Bbuild-output -G "MinGW Makefiles" -DCMAKE_MAKE_PROGRAM=C:/msys64/mingw64/bin/mingw32-make.exe -DCMAKE_C_COMPILER=C:/msys64/mingw64/bin/gcc.exe -DCMAKE_CXX_COMPILER=C:/msys64/mingw64/bin/g++.exe -DCMAKE_INSTALL_MESSAGE=NEVER -DCMAKE_BUILD_TYPE=Release -DBUILD_SHARED_LIBS=ON -DCMAKE_CXX_STANDARD=17 -DBUILD_TESTING=OFF -DGOOGLE_CLOUD_CPP_ENABLE_EXAMPLES=OFF

@coryan coryan added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p2 Moderately-important priority. Fix may not be included in next release. labels May 31, 2022
@coryan
Copy link
Contributor

coryan commented May 31, 2022

Thanks for the detailed bug report. I will try to repro this locally. When you say MSVC 2021, is that a typo and you meant MSVC 2022?

@EmotionalTristan
Copy link
Author

EmotionalTristan commented May 31, 2022 via email

@coryan coryan changed the title Unable to compile on windows Msys gcc 11.2.0/msvc 2021 pagination_range.h template overload Unable to compile on windows Msys gcc 11.2.0/msvc 2022 pagination_range.h template overload May 31, 2022
@coryan coryan self-assigned this May 31, 2022
@coryan
Copy link
Contributor

coryan commented Jun 1, 2022

These are mostly notes for myself:

  • Compiling with MSVC 2019 in C++17 mode worked (using vcpkg to install dependencies).
  • Currently I am testing with MSVC 2022 in C++17 mode (using vcpkg to install dependencies).
  • Cannot install gRPC and Abseil for msys2. One gets many errors like below. I will test with --overwrite next
mingw-w64-x86_64-abseil-cpp: /mingw64/lib/libabsl_random_internal_randen_slow.dll.a exists in filesystem (owned by mingw-w64-x86_64-grpc)
mingw-w64-x86_64-abseil-cpp: /mingw64/lib/libabsl_random_internal_seed_material.dll.a exists in filesystem (owned by mingw-w64-x86_64-grpc)
mingw-w64-x86_64-abseil-cpp: /mingw64/lib/libabsl_random_seed_gen_exception.dll.a exists in filesystem (owned by mingw-w64-x86_64-grpc)

@EmotionalTristan
Copy link
Author

EmotionalTristan commented Jun 1, 2022

A Note about Msys installing Grpc should install all its dependent libs - this should include absl.
If you remove absl and then install grpc it should have all the require libs in /mingw64/bin or mingw64/lib with the include in /mingw64/include.

To build using the msys Cmake package [mingw-w64-x86_64-cmake] - I had to ad the mingw64/bin to the PATH environment variable (would also have to add lib if building statically). (Also have to call cmake from and admin command line as msys terminal will set paths incorrectly)

I'll look into using the vcpkg I had built each MSVC package independently using Cmake so far which may be causing the issue.
Thanks for the quick response looking at this.

@coryan
Copy link
Contributor

coryan commented Jun 1, 2022

Just finished a successful build with MSVC 2022 and vcpkg.

@coryan
Copy link
Contributor

coryan commented Jun 1, 2022

I figured out why find_package(absl) failed: I was using the wrong shell. Using mingw64.exe seems to work better.

@coryan
Copy link
Contributor

coryan commented Jun 1, 2022

I got further along, but the builds failing with very strange errors (see below). I expect I somehow destroyed my dev environment for msys2. I am going to reinstall everything, and try to keep better notes this time.

cmake --build cmake-out/mingw
# Output:
# [2/1222] Building CXX object google/cloud/CMakeFiles/google_cloud_cpp_common.dir/internal/build_info.cc.obj
# FAILED: google/cloud/CMakeFiles/google_cloud_cpp_common.dir/internal/build_info.cc.obj
# /mingw64/bin/g++.exe -Dgoogle_cloud_cpp_common_EXPORTS -I/c/src/cpp-develop -isystem /c/src/cpp-develop/cmake-out/mingw -isystem /mingw64/include -O3 -DNDEBUG -fPIC -Wall -Wextra -Wconversion -Wno-sign-conversion -Werror -MD -MT google/cloud/CMakeFiles/google_cloud_cpp_common.dir/internal/build_info.cc.obj -MF google/cloud/CMakeFiles/google_cloud_cpp_common.dir/internal/build_info.cc.obj.d -o google/cloud/CMakeFiles/google_cloud_cpp_common.dir/internal/build_info.cc.obj -c /c/src/cpp-develop/cmake-out/mingw/google/cloud/internal/build_info.cc
# In file included from C:/msys64/mingw64/include/c++/12.1.0/ext/string_conversions.h:41,
#                  from C:/msys64/mingw64/include/c++/12.1.0/bits/basic_string.h:3960,
#                  from C:/msys64/mingw64/include/c++/12.1.0/string:53,
#                  from C:/src/cpp-develop/google/cloud/version.h:21,
#                  from C:/src/cpp-develop/google/cloud/internal/build_info.h:18,
#                  from C:/src/cpp-develop/cmake-out/mingw/google/cloud/internal/build_info.cc:15:
# C:/msys64/mingw64/include/c++/12.1.0/cstdlib:75:15: fatal error: stdlib.h: No such file or directory
#    75 | #include_next <stdlib.h>
#       |               ^~~~~~~~~~
# compilation terminated.

@coryan
Copy link
Contributor

coryan commented Jun 1, 2022

Okay, I removed msys2. These are the new notes:

Install msys2 from https://www.msys2.org/

https://github.com/msys2/msys2-installer/releases/download/2022-05-03/msys2-x86_64-20220503.exe

Running that program will download and install msys2, and start a msys2 shell.

Update the base packages

The instructions say this should be done twice. Run this command in the shell automatically launched by the install wizard. At the end it will ask for confirmation to close the shell (to upgrade it):

pacman -Syu

Open a new "msys2" shell to run the command again (sigh):

pacman -Syu

Install development tools

Launch a "MSYS MinGW 64-bit" shell. Run commands to update / install development tools:

pacman -S --needed --noconfirm base-devel mingw-w64-x86_64-toolchain mingw-w64-x86_64-cmake mingw-w64-x86_64-ninja

Install the dependencies from binary packages

pacman -S --needed --noconfirm mingw-w64-x86_64-protobuf mingw-w64-x86_64-grpc mingw-w64-x86_64-curl mingw-w64-x86_64-gtest mingw-w64-x86_64-benchmark mingw-w64-x86_64-nlohmann-json

@coryan
Copy link
Contributor

coryan commented Jun 1, 2022

Compile additional dependencies from source

mkdir -p /var/tmp/build/crc32c && cd /var/tmp/build/crc32c
 curl -sSL https://github.com/google/crc32c/archive/1.1.2.tar.gz | \
    tar -xzf - --strip-components=1
cmake -S . -B build-output -G Ninja -DCMAKE_C_COMPILER=gcc -DCMAKE_CXX_COMPILER=g++ -DCMAKE_BUILD_TYPE=Release -DBUILD_SHARED_LIBS=ON -DCMAKE_CXX_STANDARD=17 -DCRC32C_BUILD_TESTS=OFF -DCRC32C_BUILD_BENCHMARKS=OFF -DCRC32C_USE_GLOG=OFF
cmake --build build-output
cmake --install build-output --prefix /usr/local

@coryan
Copy link
Contributor

coryan commented Jun 1, 2022

Compile google-cloud-cpp

I had to set -DCMAKE_CXX_STANDARD=11 because at this moment gRPC and Abseil are compiled with C++11. There is a pending PR to fix that problem (see msys2/MINGW-packages#11758)

cd /s/src/cpp-develop # Or wherever your code is
cmake -S . -B cmake-out/mingw64 -G Ninja -DCMAKE_PREFIX_PATH=/usr/local/lib -DCMAKE_C_COMPILER=gcc -DCMAKE_CXX_COMPILER=g++ -DCMAKE_BUILD_TYPE=Release -DBUILD_SHARED_LIBS=ON -DCMAKE_CXX_STANDARD=11 -DBUILD_TESTING=OFF -DGOOGLE_CLOUD_CPP_ENABLE_EXAMPLES=OFF
cmake --build cmake-out/minggw64

There are some problems, I will be sending PRs to fix them:

  • _dupenv_s() in google/cloud/internal/getenv.cc
  • google/cloud/storage/internal/policy_document_request.cc is using features deprecated with C++17, needs a -D_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING

@coryan
Copy link
Contributor

coryan commented Jun 2, 2022

And finally I was able to repro:

[932/1222] Building CXX object google/cloud/bigtable/CMakeFiles/google_cloud_cpp_bigtable.dir/admin/internal/bigtable_instance_admin_connection_impl.cc.obj
FAILED: google/cloud/bigtable/CMakeFiles/google_cloud_cpp_bigtable.dir/admin/internal/bigtable_instance_admin_connection_impl.cc.obj
C:\msys64\mingw64\bin\g++.exe -D_WIN32_WINNT=0x600 -Dgoogle_cloud_cpp_bigtable_EXPORTS -IC:/src/cpp-develop -isystem C:/src/cpp-develop/cmake-out/mingw64 -isystem C:/src/cpp-develop/cmake-out/mingw64/external/googleapis -O3 -DNDEBUG -Wall -Wextra -Wconversion -Wno-sign-conversion -Werror -std=gnu++11 -MD -MT google/cloud/bigtable/CMakeFiles/google_cloud_cpp_bigtable.dir/admin/internal/bigtable_instance_admin_connection_impl.cc.obj -MF google\cloud\bigtable\CMakeFiles\google_cloud_cpp_bigtable.dir\admin\internal\bigtable_instance_admin_connection_impl.cc.obj.d -o google/cloud/bigtable/CMakeFiles/google_cloud_cpp_bigtable.dir/admin/internal/bigtable_instance_admin_connection_impl.cc.obj -c C:/src/cpp-develop/google/cloud/bigtable/admin/internal/bigtable_instance_admin_connection_impl.cc
In file included from C:/src/cpp-develop/google/cloud/bigtable/admin/internal/bigtable_instance_admin_connection_impl.cc:25:
C:/src/cpp-develop/google/cloud/internal/pagination_range.h: In instantiation of 'typename google::cloud::v1_41_0::internal::StreamReader<T>::result_type google::cloud::v1_41_0::internal::PagedStreamReader<T, Request, Response>::GetNext() [with T = google::bigtable::admin::v2::AppProfile; Request = google::bigtable::admin::v2::ListAppProfilesRequest; Response = google::bigtable::admin::v2::ListAppProfilesResponse; typename google::cloud::v1_41_0::internal::StreamReader<T>::result_type = absl::lts_20211102::variant<google::cloud::v1_41_0::Status, google::bigtable::admin::v2::AppProfile>; google::cloud::v1_41_0::internal::StreamReader<T> = std::function<absl::lts_20211102::variant<google::cloud::v1_41_0::Status, google::bigtable::admin::v2::AppProfile>()>]':
C:/src/cpp-develop/google/cloud/internal/pagination_range.h:174:51:   required from 'Range google::cloud::v1_41_0::internal::MakePaginationRange(Request, Loader, Extractor) [with Range = google::cloud::v1_41_0::StreamRange<google::bigtable::admin::v2::AppProfile>; Request = google::bigtable::admin::v2::ListAppProfilesRequest; Loader = google::cloud::bigtable_admin_internal::v1_41_0::BigtableInstanceAdminConnectionImpl::ListAppProfiles(google::bigtable::admin::v2::ListAppProfilesRequest)::<lambda(const google::bigtable::admin::v2::ListAppProfilesRequest&)>; Extractor = google::cloud::bigtable_admin_internal::v1_41_0::BigtableInstanceAdminConnectionImpl::ListAppProfiles(google::bigtable::admin::v2::ListAppProfilesRequest)::<lambda(google::bigtable::admin::v2::ListAppProfilesResponse)>]'
C:/src/cpp-develop/google/cloud/bigtable/admin/internal/bigtable_instance_admin_connection_impl.cc:321:60:   required from here
C:/src/cpp-develop/google/cloud/internal/pagination_range.h:100:32: error: call of overloaded 'ExtractPageToken(google::bigtable::admin::v2::ListAppProfilesResponse&)' is ambiguous
  100 |       token_ = ExtractPageToken(*response);
      |                ~~~~~~~~~~~~~~~~^~~~~~~~~~~
C:/src/cpp-develop/google/cloud/internal/pagination_range.h:118:25: note: candidate: 'static constexpr decltype (std::move((* u.mutable_next_page_token()))) google::cloud::v1_41_0::internal::PagedStreamReader<T, Request, Response>::ExtractPageToken(U&) [with U = google::bigtable::admin::v2::ListAppProfilesResponse; T = google::bigtable::admin::v2::AppProfile; Request = google::bigtable::admin::v2::ListAppProfilesRequest; Response = google::bigtable::admin::v2::ListAppProfilesResponse; decltype (std::move((* u.mutable_next_page_token()))) = std::__cxx11::basic_string<char>&&]'
  118 |   static constexpr auto ExtractPageToken(U& u)
      |                         ^~~~~~~~~~~~~~~~
C:/src/cpp-develop/google/cloud/internal/pagination_range.h:123:25: note: candidate: 'static constexpr decltype (std::move(u.next_page_token)) google::cloud::v1_41_0::internal::PagedStreamReader<T, Request, Response>::ExtractPageToken(U&) [with U = google::bigtable::admin::v2::ListAppProfilesResponse; T = google::bigtable::admin::v2::AppProfile; Request = google::bigtable::admin::v2::ListAppProfilesRequest; Response = google::bigtable::admin::v2::ListAppProfilesResponse; decltype (std::move(u.next_page_token)) = const std::__cxx11::basic_string<char>& (google::bigtable::admin::v2::ListAppProfilesResponse::*&&)() const]'
  123 |   static constexpr auto ExtractPageToken(U& u)
      |                         ^~~~~~~~~~~~~~~~

@coryan
Copy link
Contributor

coryan commented Jun 2, 2022

To compile most libraries the protobuf headers need a patch:

diff --git a/port_def.inc b/port_def.inc
index 7bfcfa9..3605ec1 100644
--- a/port_def.inc
+++ b/port_def.inc
@@ -433,7 +433,7 @@

 // Windows declares several inconvenient macro names.  We #undef them and then
 // restore them in port_undef.inc.
-#ifdef _MSC_VER
+#ifdef _WIN32
 #pragma push_macro("CREATE_NEW")
 #undef CREATE_NEW
 #pragma push_macro("DOUBLE_CLICK")
@@ -476,7 +476,7 @@
 #undef STRICT
 #pragma push_macro("timezone")
 #undef timezone
-#endif  // _MSC_VER
+#endif  // _WIN32

 #if defined(__clang__) || defined(__GNUC__) || defined(_MSC_VER)
 // Don't let Objective-C Macros interfere with proto identifiers with the same
diff --git a/port_undef.inc b/port_undef.inc
index 8591762..2504020 100644
--- a/port_undef.inc
+++ b/port_undef.inc
@@ -84,7 +84,7 @@
 #endif

 // Restore macro that may have been #undef'd in port_def.inc.
-#ifdef _MSC_VER
+#ifdef _WIN32
 #pragma pop_macro("CREATE_NEW")
 #pragma pop_macro("DOUBLE_CLICK")
 #pragma pop_macro("ERROR")

@coryan
Copy link
Contributor

coryan commented Jun 2, 2022

FWIW, the code compiles fine under GCC 11 and GCC 12 with godbolt: https://godbolt.org/z/3MaT7GoaK I suspect this is a problem specific to MinGW/MSYS2, thought I cannot imagine how that would be the case (a weird macro getting in the way? I would expect different errors. Some patches in the platform? I looked and nothing obvious).

In any case, I have a workaround, I will be sending a PR later today. It is a bit ugly, but not unbearable.

@coryan
Copy link
Contributor

coryan commented Jun 6, 2022

FWIW, I merged a fix to the "ambiguous" call problem. I also sent a fix for the protobuf _WIN32 vs. _MSC_VER confusion, that will take a few weeks to percolate to a Protobuf release.

Please keep in mind that this platform (Msys2) is not something we currently test on. We will be happy to fix bugs if you take the time to report them, and gladly accept fixes too. If you think we should include this in the test matrix, it would help us to have some evidence that this is an important market.

@EmotionalTristan
Copy link
Author

Cheers ill try downloading the new branch and try it on the test system.
Unfortunately where a small media processing company, while we need this to work I can't imagine were on scale large enough to add an additional test platform. That said I am happy to post any bugs I find on here in the same level of detail.
Thanks for fixing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants