[CI] Upgrade GoogleTest version from 1.12.1 to 1.13.0 #2114

cngzhnp · 2023-04-21T22:19:28Z

Upgraded GoogleTest to 1.13.0 + C++14

Changes

Revised the versions used for GoogleTest:

By default, upgraded to 1.13.0, which requires C++14
Still using 1.12.1 (unchanged) for CI builds that requires C++11
Still using 1.10.0 (unchanged) for CI builds using GCC 4.8

For significant contributions please make sure you have completed the following items:

CHANGELOG.md updated for non-trivial changes
Unit tests have been added
Changes in public API reviewed

codecov · 2023-04-23T09:58:11Z

Codecov Report

Merging #2114 (e0ed1eb) into main (d99cbab) will decrease coverage by 0.02%.
The diff coverage is n/a.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2114      +/-   ##
==========================================
- Coverage   87.31%   87.29%   -0.02%     
==========================================
  Files         169      169              
  Lines        4901     4900       -1     
==========================================
- Hits         4279     4277       -2     
- Misses        622      623       +1

see 2 files with indirect coverage changes

lalitb · 2023-04-24T03:48:42Z

ci/setup_cmake.sh

 if [ "x$GOOGLETEST_VERSION" = "x" ]; then
-  export GOOGLETEST_VERSION=1.12.1
+  export GOOGLETEST_VERSION=1.10.0


Why is default version changed to 1.10.0, can it remain as 1.12.1 ?

If GoogleTest version is not set on CI Pipeline, it was set as 1.10.0 by default. It could not be 1.12.1 version because GCC 4.8 oldish environment to support C++14.

lalitb · 2023-04-24T03:49:52Z

Can you update the description for the changes. As the latest version doesn't support C++11, how are we handling now CI tests for C++11 ?

cngzhnp · 2023-04-24T09:50:47Z

Can you update the description for the changes. As the latest version doesn't support C++11, how are we handling now CI tests for C++11 ?

Could you please check the description and verify that?

lalitb · 2023-05-03T16:29:09Z

CMakeLists.txt

@@ -535,6 +535,7 @@ list(APPEND CMAKE_PREFIX_PATH "${CMAKE_BINARY_DIR}")

 include(CTest)
 if(BUILD_TESTING)
+  set(CMAKE_CXX_STANDARD 20)


@cngzhnp - I think this will break the builds for users who are building library (along with tests) using gcc4.8 (or any C++11 only compiler) ? Do you think we should revisit upgrade to googletest once we drop C++11 only compiler support in #1830.

Yes, you are right. However, it is like CI/CD pipeline design problem for opentelemetry-cpp. There are a lot of scripts which setup different versions of third party libraries or submodules etc. All of them are not in one page.

From my perspective, if we still want to support GCC 4.8 version with C++11 standard, we need a better CMake/CI-CD/Script design for this. Otherwise, like me people might only focus on passing the CI tests but not the real usecases.

This sets C++20 for all builds using tests, which breaks gcc4.8.

See related comment, to set C++14 is relevant ci/do_ci.sh targets instead.

.github/workflows/ci.yml

marcalff

@cngzhnp

Thanks for the PR, looking into this.

In the mean time, please merge the main branch into this PR, to resolve the conflicts.

CHANGELOG.md

cngzhnp · 2023-07-04T15:09:52Z

@marcalff Could you please verify the changes after your comments? Also, could you please take a look that why CodeQL build is failed, I am not sure that fail is directly related to my changes?

marcalff · 2023-07-10T10:01:52Z

@cngzhnp Thanks for the cleanup.

Remaining points below.

CodeQL

The CodeQL / CodeQL-Build build fails on:

  [  0%] Building CXX object api/test/core/CMakeFiles/version_test.dir/version_test.cc.o
  cd /home/runner/work/opentelemetry-cpp/opentelemetry-cpp/_lgtm_build_dir/api/test/core && /usr/bin/c++  -DENABLE_TEST -I/home/runner/work/opentelemetry-cpp/opentelemetry-cpp/api/include  -fpermissive -O3 -DNDEBUG   -Wno-error=deprecated-declarations -std=gnu++11 -o CMakeFiles/version_test.dir/version_test.cc.o -c /home/runner/work/opentelemetry-cpp/opentelemetry-cpp/api/test/core/version_test.cc
  In file included from /usr/include/gtest/gtest-message.h:57,
                   from /usr/include/gtest/gtest-assertion-result.h:46,
                   from /usr/include/gtest/gtest.h:64,
                   from /home/runner/work/opentelemetry-cpp/opentelemetry-cpp/api/test/core/version_test.cc:6:
  /usr/include/gtest/internal/gtest-port.h:270:2: error: #error C++ versions less than C++14 are not supported.
    270 | #error C++ versions less than C++14 are not supported.
        |  ^~~~~

To fix this and keep the build in C++11, add:

GOOGLETEST_VERSION: 1.12.1

in file codeql-analysis.yml

setup-cmake.sh

Per previous comments, this code:

# This variable not set on CI pipeline for only legacy environment(GCC 4.8).
# With 1.13.0 version, C++14 must be set which does not supported by legacy environment anymore.
# Also with this version, release version path needs to be adapted.
if [ -z "${GOOGLETEST_VERSION}" ]; then
  # By default, GoogleTest version set the following version.
  export GOOGLETEST_VERSION=1.13.0
  GOOGLETEST_VERSION_PATH="v${GOOGLETEST_VERSION}"
  GOOGLETEST_FOLDER_PATH="googletest-${GOOGLETEST_VERSION}"
else
  GOOGLETEST_VERSION_PATH="release-${GOOGLETEST_VERSION}"
  GOOGLETEST_FOLDER_PATH="googletest-release-${GOOGLETEST_VERSION}"

fi

should decouple setting the default release with the release format.

Something like:

if [ -z "${GOOGLETEST_VERSION}" ]; then
  # Version by default. Requires C++14
  export GOOGLETEST_VERSION=1.13.0
fi

followed by something like:

if [  << GOOGLETEST_VERSION greater or equal to 1.13.0 >> ]; then
  GOOGLETEST_VERSION_PATH="v${GOOGLETEST_VERSION}"
  GOOGLETEST_FOLDER_PATH="googletest-${GOOGLETEST_VERSION}"
else
  GOOGLETEST_VERSION_PATH="release-${GOOGLETEST_VERSION}"
  GOOGLETEST_FOLDER_PATH="googletest-release-${GOOGLETEST_VERSION}"
fi

Expectation is that all the following use case works

unset GOOGLETEST_VERSION
setup_cmake

installs 1.13.0

GOOGLETEST_VERSION=1.13.0
setup_cmake

installs 1.13.0

GOOGLETEST_VERSION=1.12.1
setup_cmake

installs 1.12.1

GOOGLETEST_VERSION=1.10.0
setup_cmake

installs 1.10.0

cngzhnp · 2023-07-10T12:02:44Z

@marcalff Could you please check & verify what I changed so for? Now, I fixed the CodeQL and split up GoogleTest version and its path.

marcalff · 2023-07-10T13:08:36Z

@marcalff Could you please check & verify what I changed so for? Now, I fixed the CodeQL and split up GoogleTest version and its path.

Thanks.

How about this instead:

OLD_VERSION_REGEXP="^1\.([0-9]|10|11|12)(\..*)?$"

if [[ ${GOOGLETEST_VERSION} =~ ${OLD_VERSION_REGEXP} ]]; then
  # Old package name
else
  # New (starting with 1.13.0) package name
fi

marcalff · 2023-07-10T15:38:36Z

Somehow, adding quotes broke the regexp test.

Tested locally, this works:

if [[ ${GOOGLETEST_VERSION} =~ ${OLD_VERSION_REGEXP} ]]; then

Tested locally, this fails:

if [[ "${GOOGLETEST_VERSION}" =~ "${OLD_VERSION_REGEXP}" ]]; then

breaking the build for 1.10.0 (Gcc 4.8) and 1.12.1 (C++11 builds)

marcalff

The code LGTM.

Please revise:

the PR description
code comments

for cleanup.

@owent

This change may cause merge collisions with your work, when setting CMAKE_CXX_STANDARD. I assume these will be trivial to fix, so not an issue.

@lalitb @esigo @owent

Please take a second look.

owent

LGTM and thanks.

owent · 2023-07-11T13:58:42Z

The code LGTM.

Please revise:

the PR description

code comments

for cleanup.

@owent

This change may cause merge collisions with your work, when setting CMAKE_CXX_STANDARD. I assume these will be trivial to fix, so not an issue.

@lalitb @esigo @owent

Please take a second look.

It's fine by me , we also use gtest 13 and a high CMAKE_CXX_STANDARD in some of our products.While for the users still use gcc 4.8 and C++11, them can still use gtest 1.10.0 and it works well now.

marcalff · 2023-07-11T15:09:52Z

~~Set merge temporarily on hold, to avoid conflicts with the release of opentelemetry-cpp 1.10.0.~~

CMakeLists.txt

ci/do_ci.sh

lalitb

LGTM. Thanks for the upgrade.

cngzhnp force-pushed the feature/update_googletest_version branch from 6bfa052 to e61cde0 Compare April 23, 2023 09:45

cngzhnp force-pushed the feature/update_googletest_version branch 14 times, most recently from ea8ec98 to 4d0420e Compare April 23, 2023 14:12

cngzhnp marked this pull request as ready for review April 23, 2023 14:53

cngzhnp requested a review from a team April 23, 2023 14:53

cngzhnp force-pushed the feature/update_googletest_version branch 2 times, most recently from c2a1ebf to 4e17174 Compare April 23, 2023 15:09

lalitb reviewed Apr 24, 2023

View reviewed changes

lalitb reviewed May 3, 2023

View reviewed changes

ThomsonTan reviewed May 8, 2023

View reviewed changes

.github/workflows/ci.yml Show resolved Hide resolved

marcalff reviewed May 22, 2023

View reviewed changes

marcalff added this to the Migrate to C++14 milestone May 23, 2023

marcalff changed the title ~~Upgrade GoogleTest version from 1.12.1 to 1.13.0~~ [CI] Upgrade GoogleTest version from 1.12.1 to 1.13.0 May 26, 2023

cngzhnp force-pushed the feature/update_googletest_version branch from 4e17174 to ec849a1 Compare May 26, 2023 12:20

marcalff reviewed May 26, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

cngzhnp added a commit to cngzhnp/opentelemetry-cpp that referenced this pull request Jul 4, 2023

Fix scripts after review comments open-telemetry#2114

262aacf

cngzhnp force-pushed the feature/update_googletest_version branch from 17f4fd6 to 262aacf Compare July 4, 2023 09:53

cngzhnp added a commit to cngzhnp/opentelemetry-cpp that referenced this pull request Jul 4, 2023

Fix scripts after review comments open-telemetry#2114

fcecc4c

cngzhnp force-pushed the feature/update_googletest_version branch from 262aacf to fcecc4c Compare July 4, 2023 14:05

cngzhnp added a commit to cngzhnp/opentelemetry-cpp that referenced this pull request Jul 4, 2023

Fix scripts after review comments open-telemetry#2114

8900a2f

cngzhnp force-pushed the feature/update_googletest_version branch from fcecc4c to 8900a2f Compare July 4, 2023 14:34

marcalff and others added 2 commits July 10, 2023 11:24

[DEPRECATION] Drop C++11 support (open-telemetry#2146)

e3aa0a5

Fix scripts after review comments open-telemetry#2114

cc68517

cngzhnp force-pushed the feature/update_googletest_version branch from 8900a2f to cc68517 Compare July 10, 2023 09:27

Split up GoogleTest version and deploy path

dab4111

Use regex instead of version compare for GoogleTest

83ee1cf

Remove double quotes for different GoogleTest version comparison

a3fa3c9

marcalff approved these changes Jul 10, 2023

View reviewed changes

owent approved these changes Jul 11, 2023

View reviewed changes

marcalff added the pr:do-not-merge This PR is not ready to be merged. label Jul 11, 2023

ThomsonTan reviewed Jul 11, 2023

View reviewed changes

CMakeLists.txt Show resolved Hide resolved

marcalff removed the pr:do-not-merge This PR is not ready to be merged. label Jul 11, 2023

lalitb reviewed Jul 12, 2023

View reviewed changes

ci/do_ci.sh Show resolved Hide resolved

lalitb approved these changes Jul 12, 2023

View reviewed changes

Merge branch 'main' into feature/update_googletest_version

e0ed1eb

ThomsonTan added the ok-to-merge The PR is ok to merge (has two approves or raised by a maintainer/approver and has one approve) label Jul 12, 2023

marcalff merged commit b3d547c into open-telemetry:main Jul 12, 2023

chenrui333 mentioned this pull request Aug 22, 2023

opentelemetry-cpp 1.11.0 Homebrew/homebrew-core#140216

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CI] Upgrade GoogleTest version from 1.12.1 to 1.13.0 #2114

[CI] Upgrade GoogleTest version from 1.12.1 to 1.13.0 #2114

cngzhnp commented Apr 21, 2023 •

edited by marcalff

Loading

codecov bot commented Apr 23, 2023 •

edited

Loading

lalitb Apr 24, 2023

cngzhnp Apr 24, 2023

lalitb commented Apr 24, 2023

cngzhnp commented Apr 24, 2023

lalitb May 3, 2023 •

edited

Loading

cngzhnp May 4, 2023

marcalff May 26, 2023

marcalff left a comment

cngzhnp commented Jul 4, 2023

marcalff commented Jul 10, 2023

cngzhnp commented Jul 10, 2023

marcalff commented Jul 10, 2023 •

edited

Loading

marcalff commented Jul 10, 2023 •

edited

Loading

marcalff left a comment

owent left a comment

owent commented Jul 11, 2023

marcalff commented Jul 11, 2023 •

edited

Loading

lalitb left a comment •

edited

Loading

[CI] Upgrade GoogleTest version from 1.12.1 to 1.13.0 #2114

[CI] Upgrade GoogleTest version from 1.12.1 to 1.13.0 #2114

Conversation

cngzhnp commented Apr 21, 2023 • edited by marcalff Loading

Changes

codecov bot commented Apr 23, 2023 • edited Loading

Codecov Report

lalitb Apr 24, 2023

Choose a reason for hiding this comment

cngzhnp Apr 24, 2023

Choose a reason for hiding this comment

lalitb commented Apr 24, 2023

cngzhnp commented Apr 24, 2023

lalitb May 3, 2023 • edited Loading

Choose a reason for hiding this comment

cngzhnp May 4, 2023

Choose a reason for hiding this comment

marcalff May 26, 2023

Choose a reason for hiding this comment

marcalff left a comment

Choose a reason for hiding this comment

cngzhnp commented Jul 4, 2023

marcalff commented Jul 10, 2023

CodeQL

setup-cmake.sh

cngzhnp commented Jul 10, 2023

marcalff commented Jul 10, 2023 • edited Loading

marcalff commented Jul 10, 2023 • edited Loading

marcalff left a comment

Choose a reason for hiding this comment

owent left a comment

Choose a reason for hiding this comment

owent commented Jul 11, 2023

marcalff commented Jul 11, 2023 • edited Loading

lalitb left a comment • edited Loading

Choose a reason for hiding this comment

cngzhnp commented Apr 21, 2023 •

edited by marcalff

Loading

codecov bot commented Apr 23, 2023 •

edited

Loading

lalitb May 3, 2023 •

edited

Loading

marcalff commented Jul 10, 2023 •

edited

Loading

marcalff commented Jul 10, 2023 •

edited

Loading

marcalff commented Jul 11, 2023 •

edited

Loading

lalitb left a comment •

edited

Loading