Skip to content

Backport GCC tests+fixes #12743

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

Merged
merged 1 commit into from
May 9, 2023
Merged

Backport GCC tests+fixes #12743

merged 1 commit into from
May 9, 2023

Conversation

mkruskal-google
Copy link
Member

This also adds tests for C++17 and C++20 to prevent further regressions.

PiperOrigin-RevId: 530693318

This also adds tests for C++17 and C++20 to prevent further regressions.

PiperOrigin-RevId: 530693318
@mkruskal-google mkruskal-google added the back-port Cherrypick PRs to release branches label May 9, 2023
@mkruskal-google mkruskal-google requested a review from sbenzaquen May 9, 2023 20:54
@mkruskal-google mkruskal-google requested review from a team as code owners May 9, 2023 20:54
@mkruskal-google mkruskal-google requested review from perezd and deannagarcia and removed request for a team May 9, 2023 20:54
@mkruskal-google mkruskal-google merged commit 0ce78c6 into 23.x May 9, 2023
@mkruskal-google mkruskal-google deleted the gcc-23 branch May 9, 2023 23:22
@h-vetinari
Copy link
Contributor

Hey there. I backported this patch at the suggestion of @coryan in conda-forge/libprotobuf-feedstock#156, but things still failed with GCC 12.2 and C++17. Background is we need to compile everything with C++17 due to the ABI-impact of the standard version on abseil; For 4.22.x, this worked fine.

compilation error log
[...]
[342/391] Building CXX object CMakeFiles/tests.dir/src/google/protobuf/unittest.pb.cc.o
FAILED: CMakeFiles/tests.dir/src/google/protobuf/unittest.pb.cc.o 
$BUILD_PREFIX/bin/x86_64-conda-linux-gnu-c++ -DGOOGLE_PROTOBUF_CMAKE_BUILD -DGTEST_LINKED_AS_SHARED_LIBRARY=1 -DHAVE_ZLIB -DPROTOBUF_USE_DLLS -I$SRC_DIR/build-shared -I$SRC_DIR/src -I$SRC_DIR/third_party/utf8_range -fvisibility-inlines-hidden -fmessage-length=0 -march=nocona -mtune=haswell -ftree-vectorize -fPIC -fstack-protector-strong -fno-plt  -ffunction-sections -pipe -isystem $PREFIX/include -fdebug-prefix-map=$SRC_DIR=/usr/local/src/conda/libprotobuf-4.23.0 -fdebug-prefix-map=$PREFIX=/usr/local/src/conda-prefix -DNDEBUG -O3 -DNDEBUG -std=gnu++17 -fPIE -DGOOGLE_PROTOBUF_TEST_PLUGIN_PATH=\"$SRC_DIR/build-shared/test_plugin\" -MD -MT CMakeFiles/tests.dir/src/google/protobuf/unittest.pb.cc.o -MF CMakeFiles/tests.dir/src/google/protobuf/unittest.pb.cc.o.d -o CMakeFiles/tests.dir/src/google/protobuf/unittest.pb.cc.o -c $SRC_DIR/src/google/protobuf/unittest.pb.cc
$SRC_DIR/src/google/protobuf/unittest.pb.cc: In constructor 'constexpr protobuf_unittest::TestCord::TestCord(google::protobuf::internal::ConstantInitialized)':
$SRC_DIR/src/google/protobuf/unittest.pb.cc:3084:73: error: temporary of non-literal type 'absl::lts_20230125::Cord' in a constant expression
 3084 |       TestCord::Impl_::_default_optional_bytes_cord_default_func_{})}} {}
      |                                                                         ^
In file included from $SRC_DIR/src/google/protobuf/io/coded_stream.h:134,
                 from $SRC_DIR/src/google/protobuf/unittest.pb.h:24,
                 from $SRC_DIR/src/google/protobuf/unittest.pb.cc:4:
$PREFIX/include/absl/strings/cord.h:150:7: note: 'absl::lts_20230125::Cord' is not literal because:
  150 | class Cord {
      |       ^~~~
$PREFIX/include/absl/strings/cord.h:150:7: note:   'absl::lts_20230125::Cord' has a non-trivial destructor
Build invocation, roughly
cmake -G "Ninja" \
    -DCMAKE_BUILD_TYPE=Release \
    -DCMAKE_CXX_STANDARD=17 \
    -Dprotobuf_ABSL_PROVIDER="package" \
    -Dprotobuf_BUILD_SHARED_LIBS=ON \
    -Dprotobuf_JSONCPP_PROVIDER="package" \
    -Dprotobuf_USE_EXTERNAL_GTEST=ON \
    -Dprotobuf_WITH_ZLIB=ON \
    ..

I note that GCC 12.2 is just at the boundary of when PROTOBUF_GNUC_MIN(12, 2) kicks in, so I tried the following patch on top of this one:

@@ -683,7 +683,7 @@ static_assert(PROTOBUF_CPLUSPLUS_MIN(201402L), "Protobuf only supports C++14 and
 // GCC doesn't support constinit aggregate initialization of absl::Cord.
 # if PROTOBUF_GNUC_MIN(12, 2)
 #  define PROTOBUF_CONSTINIT
-#  define PROTOBUF_CONSTEXPR constexpr
+#  define PROTOBUF_CONSTEXPR
 # endif
 #else
 # if defined(__cpp_constinit) && !defined(__CYGWIN__)

And then the build succeeds (and ctest --progress --output-on-failure passes).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
back-port Cherrypick PRs to release branches
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants