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

[ci] upgrade to GoogleTest v1.14.0 (fixes #5976) #5981

Merged
merged 14 commits into from
Dec 28, 2023
Merged

Conversation

jameslamb
Copy link
Collaborator

@jameslamb jameslamb commented Jul 14, 2023

Fixes #5976

Updates to the latest version of GoogleTest, v1.14.0 (https://github.com/google/googletest/releases), to get recent miscellaneous bugfixes and improvements and to keep up with versions that LightGBM contributors and uses are more likely to have.

Other changes:

  • removes C++ standard from flags in CMakeLists.txt, in favor of setting CMake variable CMAKE_CXX_STANDARD
  • sets CMAKE_CXX_STANDARD_REQUIRED to ON, to ensure a loud error is raised if using a compiler that doesn't support the requested standard (instead of silently falling back to an earlier version, which could lead to cryptic errors or undefined behavior)
  • uses C++14 standard when running the C++ tests with GoogleTest

Why only go to C++14?

C++14 is the minimum version supported by newer versions of GoogleTest.

I tried to go all the way to C++20 in this PR, but encountered some compilation errors like the following:

* LightGBM/include/LightGBM/utils/common.h:1203:35:   in 'constexpr' expansion of 'fmt::v8::basic_format_string<char, const unsigned int&>(format)'
* LightGBM/include/LightGBM/utils/common.h:1203:35: error: 'format' is not a constant expression

Which might be related to fmtlib/fmt#2775 or fmtlib/fmt#2898.

I'm proposing separating the work of adding C++20 out to #6033.

Adding C++14 specifically also improves the project's CI coverage of C++ standards:

  • C++11 = most tests (Python-package, CMake-based R-package, SWIG, CUDA): C++11
  • C++14 = the cpp tests
  • C++17 = CRAN-style R-package tests

References

The changes in CMakeLists.txt are supported by the following:

How I tested this

git clean -d -f -X
rm -rf ./build
mkdir build
cd ./build
cmake -DBUILD_CPP_TEST=ON -DUSE_OPENMP=OFF ..

VERBOSE=1 \
make -j2 testlightgbm

../testlightgbm

Observed that the expected --std flags were passed to the compiler (e.g.

@jameslamb
Copy link
Collaborator Author

Ok good! Just updating the version without changing the C++ standard, I see exactly the same errors in CI as reported in #5976.

/__w/1/s/build/_deps/googletest-src/googletest/include/gtest/internal/gtest-port.h:270:2: error: C++ versions less than C++14 are not supported.
#error C++ versions less than C++14 are not supported.

https://dev.azure.com/lightgbm-ci/lightgbm-ci/_build/results?buildId=14929&view=logs&j=c1aa5445-36f0-58f5-2c69-68691d629457&t=c07c8f5b-ba24-5225-6ddc-80219dbcdc35

@jameslamb jameslamb changed the title WIP: [ci] upgrade to GoogleTest v1.13.0 WIP: [ci] upgrade to GoogleTest v1.13.0 (fixes #5976) Jul 15, 2023
This was referenced Aug 11, 2023
@jameslamb jameslamb changed the title WIP: [ci] upgrade to GoogleTest v1.13.0 (fixes #5976) WIP: [ci] upgrade to GoogleTest v1.14.0 (fixes #5976) Nov 15, 2023
@jameslamb jameslamb changed the title WIP: [ci] upgrade to GoogleTest v1.14.0 (fixes #5976) [ci] upgrade to GoogleTest v1.14.0 (fixes #5976) Nov 15, 2023
@jameslamb jameslamb marked this pull request as ready for review November 15, 2023 06:00
@jameslamb jameslamb requested a review from borchero as a code owner December 28, 2023 03:17
@jameslamb
Copy link
Collaborator Author

@guolinke @shiyu1994 could I please get a review on this one?

@jameslamb
Copy link
Collaborator Author

thank you!

@jameslamb jameslamb merged commit b74528f into master Dec 28, 2023
41 checks passed
@jameslamb jameslamb deleted the googletest-version branch December 28, 2023 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[c++] googletest tests fail on newer versions of googletest which dropped C++11 support
2 participants