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.

Sign up for GitHub

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

[BUILD] Introduce CXX 20 CI pipeline for MSVC/Windows #2450

Merged
merged 13 commits into from
Dec 14, 2023
Merged

[BUILD] Introduce CXX 20 CI pipeline for MSVC/Windows #2450

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
e72d1cf
Introduce CXX 20 pipeline for MSVC/Windows
ThomsonTan Dec 13, 2023
ced4145
Update the job name
ThomsonTan Dec 13, 2023
54c61c6
Test if MSVC C++20 can catch the real issue
ThomsonTan Dec 13, 2023
1a3dd9c
Merge branch 'main' into msvc_cxx20
ThomsonTan Dec 13, 2023
f320936
Enable STL test
ThomsonTan Dec 14, 2023
cf2c445
Fix option for WITH_STL
ThomsonTan Dec 14, 2023
853090b
Set CXX standard to 20
ThomsonTan Dec 14, 2023
0a3662d
Revert "Test if MSVC C++20 can catch the real issue"
ThomsonTan Dec 14, 2023
8f3bd3e
Fix warning C4834
ThomsonTan Dec 14, 2023
7a6ead9
Fix VariantTest::Get for C++20 on MSVC
ThomsonTan Dec 14, 2023
3c76965
Update chagnelog
ThomsonTan Dec 14, 2023
5e7eee7
Merge branch 'main' into msvc_cxx20
lalitb Dec 14, 2023
d029d19
Fix variant test code for C++ 17
ThomsonTan Dec 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,22 @@ jobs:
- name: run tests
run: ./ci/do_ci.ps1 cmake.maintainer.test

cmake_msvc_maintainer_test_stl_cxx20:
name: CMake msvc (maintainer mode) with C++20
runs-on: windows-latest
steps:
- uses: actions/checkout@v4
with:
submodules: 'recursive'
- name: setup
run: |
./ci/setup_windows_cmake.ps1
./ci/setup_windows_ci_environment.ps1
- name: run tests
env:
CXX_STANDARD: '20'
run: ./ci/do_ci.ps1 cmake.maintainer.cxx20.stl.test

cmake_with_async_export_test:
name: CMake test (without otlp-exporter and with async export)
runs-on: ubuntu-latest
Expand Down
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ Increment the:
[#2435](https://github.com/open-telemetry/opentelemetry-cpp/pull/2435)
* [BUILD] Fix removing of NOMINMAX on Windows
[#2449](https://github.com/open-telemetry/opentelemetry-cpp/pull/2449)
* [BUILD] Introduce CXX 20 CI pipeline for MSVC/Windows
[#2450](https://github.com/open-telemetry/opentelemetry-cpp/pull/2450)

Important changes:

Expand Down
4 changes: 2 additions & 2 deletions api/test/nostd/string_view_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,8 @@ TEST(StringViewTest, SubstrPortion)
TEST(StringViewTest, SubstrOutOfRange)
{
string_view s = "abc123";
#if __EXCEPTIONS || ((defined(OPENTELEMETRY_STL_VERSION) && (OPENTELEMETRY_STL_VERSION >= 2020)))
EXPECT_THROW(s.substr(10), std::out_of_range);
#if __EXCEPTIONS || (defined(OPENTELEMETRY_STL_VERSION) && (OPENTELEMETRY_STL_VERSION >= 2020))
EXPECT_THROW((void)s.substr(10), std::out_of_range);
Copy link
Member

Choose a reason for hiding this comment

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

I suspect the existing code was incorrect, as std::string_view is introduced in C++17, not C++20.

Please investigate (local CXX 17 MSVC build) if the logic should be OPENTELEMETRY_STL_VERSION >= 2017 instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Verified that std::out_of_range exception was thrown out with MSVC/C++17. Thanks for the catch.

#else
EXPECT_DEATH({ s.substr(10); }, "");
#endif
Expand Down
2 changes: 1 addition & 1 deletion api/test/nostd/variant_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ TEST(VariantTest, Get)
EXPECT_EQ(nostd::get<int>(w), 12);
EXPECT_EQ(*nostd::get_if<int>(&v), 12);
EXPECT_EQ(nostd::get_if<float>(&v), nullptr);
#if __EXCEPTIONS
#if __EXCEPTIONS || (defined(OPENTELEMETRY_STL_VERSION) && (OPENTELEMETRY_STL_VERSION >= 2020))
Copy link
Member

Choose a reason for hiding this comment

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

std::variant is introduced in C++17, not C++20.

Please investigate (local CXX 17 MSVC build) if the logic should be OPENTELEMETRY_STL_VERSION >= 2017 instead.

Also, somehow the existing code passed in C++14, C++17, C++20 builds in CI with GCC.

Not sure which platform and/or which compiler do set __EXCEPTIONS, please clarify if you know the context.

Copy link
Member

Choose a reason for hiding this comment

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

Found out that __EXCEPTIONS is set by GCC and Clang, except when building with -fno-exceptions.

This is likely why a GCC build with exceptions passed this test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for looking into it. Will try MSVC C++ 17 build.

Copy link
Contributor Author

@ThomsonTan ThomsonTan Dec 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Verified that bad_variant_access exception was thrown out with MSVC/C++17. Thanks for the catch.

EXPECT_THROW(nostd::get<float>(w), nostd::bad_variant_access);
#else
EXPECT_DEATH({ nostd::get<float>(w); }, "");
Expand Down
24 changes: 24 additions & 0 deletions ci/do_ci.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,30 @@ switch ($action) {
exit $exit
}
}
"cmake.maintainer.cxx20.stl.test" {
cd "$BUILD_DIR"
cmake $SRC_DIR `
-DWITH_STL=CXX20 `
-DCMAKE_CXX_STANDARD=20 `
-DOTELCPP_MAINTAINER_MODE=ON `
-DWITH_NO_DEPRECATED_CODE=ON `
-DVCPKG_TARGET_TRIPLET=x64-windows `
"-DCMAKE_TOOLCHAIN_FILE=$VCPKG_DIR/scripts/buildsystems/vcpkg.cmake"
$exit = $LASTEXITCODE
if ($exit -ne 0) {
exit $exit
}
cmake --build . -j $nproc
$exit = $LASTEXITCODE
if ($exit -ne 0) {
exit $exit
}
ctest -C Debug
$exit = $LASTEXITCODE
if ($exit -ne 0) {
exit $exit
}
}
"cmake.with_async_export.test" {
cd "$BUILD_DIR"
cmake $SRC_DIR `
Expand Down