Better handling of OPENTELEMETRY_STL_VERSION.

[bcsgh]

Fixes #2470

Changes

• Add an #error when OPENTELEMETRY_STL_VERSION is unsupported by the actually C++ version.

NOTE: this doesn't work because compilers do strange things with __cplusplus

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: bcsgh (7baf5f8, 3cc5c1b, 77b6912, b543295, daca53b, 697794d, d486cd7, 163b341)
• ✅ login: ThomsonTan / name: Tom Tan (69154a1)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (497eaf4) 87.12% compared to head (163b341) 87.12%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2490   +/-   ##
=======================================
  Coverage   87.12%   87.12%           
=======================================
  Files         200      200           
  Lines        6109     6109           
=======================================
  Hits         5322     5322           
  Misses        787      787           

[bcsgh]

Does anyone know how to get the text of the CLA I'm being asked to agree to before I start the process of agreeing to it?

I.e. I'm looking for a public link to that document that can be viewed by anyone on the internet without any authentication or sign in of any kind.

[lalitb]

Does anyone know how to get the text of the CLA I'm being asked to agree to before I start the process of agreeing to it?

https://github.com/cncf/cla

[bcsgh]

https://github.com/cncf/cla

The PDF's there refer to v1 where as the link here refers to v2. Is there only one version of the CLA but two version of EasyCLA?

(Really, EasyCLA should show the full text of the agreement before it even asks you to log in. The fact it doesn't seem to actively want the world to know what the agreement is kinds seems suggestive, and not in a good way.)

[lalitb]

Sorry, ignore the link I shared. But I believe there should be a pdf link to download the agreement text before you sign, while selecting the option for individual or corporate contributor as mentioned here - https://docs.linuxfoundation.org/lfx/easycla/v2-current/contributors/individual-contributor.

[bcsgh]

Sorry, ignore the link I shared. But I believe there should be a pdf link to download the agreement text before you sign, while selecting the option for individual or corporate contributor as mentioned here - https://docs.linuxfoundation.org/lfx/easycla/v2-current/contributors/individual-contributor.

Step 4 there is basically "create an account" on system I haven't yet decided I'm willing to have any connection to. They don't even let me file a ticket about that without creating the account. :-(

That said, the v1 CLA seems palatable enough (heck, it doesn't even seem to want a licence for interplanetary space!) that I'm willing to take the risk.

[ThomsonTan]

Suggested change

	# error "OPENTELEMETRY_STL_VERSION set to version newer than compilation version."
	# error "OPENTELEMETRY_STL_VERSION is set to a version newer than the compiler version."

[bcsgh]

"the compiler version" is ambiguous. Is it saying the version of GCC/Clang it self? The version of C++ supported by the compiler? The version currently being used by the compiler? I'm not sure that "compilation version" is any more accurate, but it at least doesn't invite that first incorrect interpretation. Any suggestions on how to avoid that?

I'll go with whatever you want, but if we can't come up with something clear, then I'd rather something that makes people think a bit before they make a bad assumption.

[owent]

We can use _MSVC_LANG to check C++ version without /Zc:__cplusplus using MSVC.

[bcsgh]

Both done (used slightly different verbiage and I'm still depending on the MSVC CI to test things for me).

[ThomsonTan]

For the issue of setting OPENTELEMETRY_STL_VERSION newer than the supported compiler version, would the compilation just fail?

[bcsgh]

For the issue of setting OPENTELEMETRY_STL_VERSION newer than the supported compiler version, would the compilation just fail?

Likely? But not necessarily. It's possible you could avoid using any of the headers that check for something between the two. But that might actually be worse because including another header somewhere down the line might surface a preexisting issue and you won't even have the hint that it's related to something you just changed.

Regardless, the resulting error message is inscrutable and doesn't tell you anything about what the actual issue is or how to fix to.

[bcsgh]

On further inspection, things on MSVC are more complicated:

• https://learn.microsoft.com/en-us/cpp/build/reference/zc-cplusplus
• bcsgh@b543295

Note, I don't have a MSVC system to even try to test that on so my only option is blindly firing it at the CI.

that said, I'm not totally sure this warrens the added complexity given that __cplusplus is already used in a few places where that issue isn't (correctly) addressed:

• https://github.com/open-telemetry/opentelemetry-cpp/blob/main/exporters/ostream/include/opentelemetry/exporters/ostream/common_utils.h
• https://github.com/open-telemetry/opentelemetry-cpp/blob/main/api/include/opentelemetry/nostd/internal/absl/base/config.h
• https://github.com/open-telemetry/opentelemetry-cpp/blob/main/api/include/opentelemetry/nostd/internal/absl/base/policy_checks.h (Notes the issue but ignores it.)
• https://github.com/open-telemetry/opentelemetry-cpp/blob/main/api/include/opentelemetry/nostd/internal/absl/meta/type_traits.h

[lalitb]

yeah, the MSVC compiler historically doesn't update the _cplusplus macros. Probably existing usage in code fails to catch this error.

[bcsgh]

Oh fun! From the CMake C++20 test(GCC) CI test:

...
/home/runner/work/opentelemetry-cpp/opentelemetry-cpp/api/include/opentelemetry/version.h:13:6: error: #error "OPENTELEMETRY_STL_VERSION is set to a version newer than the curent C++ version."
...
/home/runner/work/opentelemetry-cpp/opentelemetry-cpp/api/include/opentelemetry/version.h:12:115: note: #pragma message: 2023 vs. 201709L
...

The test claims to be testing C++20, the test is asking for c++23 and it's actually using C++17.

Anyone want to take a guess as to what the correct fix is?

---

Things also fail under CMake C++20 test(Clang with libc++) and the common factor between those two CI runs seems to be they both trigger via run: ./ci/do_ci.sh cmake.c++20.test. And that seems to end up here where nothing seems to be done to actauly set the C++ version to 2020

There is also cmake.c++20.stl.test that seems to not actualy mention a C++ version?

[lalitb]

Anyone want to take a guess as to what the correct fix is?

Probably the runtime environment is not correct for this test - the default gcc compiler in ubuntu-20.04 won't be supporting C++20. We should be using ubuntu-latest. Though need to see from where C++23 is coming :)

[ThomsonTan]

Is the checking here similar to CMAKE_CXX_STANDARD_REQUIRED? Then probably we can add below policy to the project (also need a cmake version check because it is added to cmake 3.8).

cmake_policy(SET CMP0067 NEW) 

[bcsgh]

cmake_policy(SET CMP0067 NEW) 

I don't know near enough about CMake to figure out how to do that ??

[bcsgh]

Though need to see from where C++23 is coming :)

• ci/do_ci.sh#cmake.c++20.stl.test sets WITH_STL=ON
• api/CMakeLists.txt equates that to WITH_STL=CXX23

I'm thinking that the C++2020 test should explicitly set C++2020.

And it also looks like WITH_STL=ON is supposed to be doing what WITH_STL=BEST would do, but just assumes the compiler's default C++ version.

[marcalff]

And it also looks like WITH_STL=ON is supposed to be doing what WITH_STL=BEST would do, but just assumes the compiler's default C++ version.

Good point, we can probably alter the meaning of "ON" instead of adding yet another choice.

[ThomsonTan]

I don't know near enough about CMake to figure out how to do that ??

cmake_policy(SET CMP0067 NEW) 

I don't know near enough about CMake to figure out how to do that ??

Never mind, the CMake policy is meant to check CXX_STANDARD in CMake, probably it cannot be used to check OPENTELEMETRY_STL_VERSION here.

[bcsgh]

It seems that __cplusplus has some odd values:

• GCC 9.4.0 with C++20 reports as 201709L
• GCC 11.4.0 with C++23 reports as 202100L
• Clang 14.0.0 with C++23 reports as 202101L

Well, all of those are greater than the official values for the prior standard versions, so we have that goin' for us, which is nice.

While pondering this dilemma, this is what happens without the version guard: #2503

[ThomsonTan]

Should this be closed and superseded by #2503 @bcsgh

[bcsgh]

@ Maybe?

Should this be closed and superseded by #2503 @bcsgh

Maybe? This is doing more than that, and that "more" is things that I think should be done at some point, but I'm not sure how to do those things and don't want to block the other bits on that unknown.

I'm inclined say the other PR should be merged and this one updated to just be what's left over. Even if this PR gets closed at that point, it would leave this in a state that is good documentation on what's going on and why.

[bcsgh]

@ThomsonTan bumped back to a draft for now.

[marcalff]

Now that most of the original code from this PR is merged to main, the last remaining part is:

error "OPENTELEMETRY_STL_VERSION is set to a version newer than the curent C++ version."

For example, when:

• OPENTELEMETRY_STL_VERSION is set to 2017
• The C++ compiler is building with std::c++14

this PR wants to raise an error and prevent the build.

Such a combination can be surprising, but I am not aware of any fundamental reason, such as a limitation in opentelemetry-cpp, to justify failing the build in this case.

If a user ask for a very recent STL, uses a very old C++ standard, and some class from the STL requires support from the compiler (new syntax, C++ intrinsic properties), the build will fail anyway, so this extra check does not provide any value or safety.

If a user asks for an STL just slightly more recent than the C++ standard used at compile time, for example to use a basic class like std::span or std::string_view, I don't think opentelemetry-cpp should fail the build, just because the combination "looks strange".

People may have very strange requirements or environments, including when cross compiling, including when rebuilding the STL from source, and opentelemetry-cpp should not get in the way here, if this really is what people want.

Unless we know of an actual bug that this extra check will prevent, I think this extra check is not justified, and should not be merged to main.

Keeping the PR open a few days so this can be discussed if necessary.

[bcsgh]

At this point I'm mostly indifferent as what I got in solves the problem for my case. That said, a few observations:

• The define is insufficient to provide ABI comparability. IIRC basically all modern C++ std lib's use inline namespaces so builds will only be binary compatible between builds if both the OPENTELEMETRY_STL_VERSION and the actual library version match. On top of that, allowing them to not match expands the support matrix.
• C++11 and later have a standard mechanism for detecting if a given feature is present. In light of that, it should be possible to make the existing define only flag if the std-lib is to be used at all and chose the rest based on what's actually present. Or it could be deprecated in favor of a differently named define that does that. In theory that could also expand the test matrix with different availability of features that were nominally added at the same time, but that has a 1:1 correspondence to compiler and library versions so I suspect in practice it wound't actually expand what should be QA'ed.
• Barring a legitimate, supported reason for an end user wanting a mismatch, the ability for someone to get this wrong seems like a foot-gun. Having two different things that need to be updated in lockstep, by people who are unlikely to be experts, and with no mechanism to detect errors; that seems like an invitation to human error.

Either way, given that __cplusplus has unexpected values in some cases, I don't think this implementation is viable, even though I think the general concept is still worth considering.

[marcalff]

@bcsgh

I agree with you that this whole area needs further analysis, especially with the overlapping concern with inline namespaces that could be used in the stl. Things in this area are never simple and clear cut.

For this very PR in any case, it can not be merged as is, due to the additional complexity caused by some compilers, which don't provide __cplusplus values reflecting the actual C++ standard used. This causes some CI breaks currently, and investing more time to try to fix it is probably not worth the effort.

This PR only contains a remaining build check, all the important bits have been merged with #2503 already (and thanks again for that).

Closing.