Fix issues #740 and #741

[owent]

Fixes #740 #741

Changes

• Move HAVE_CPP_STDLIB and HAVE_GSL into INTERFACE_COMPILE_DEFINITIONS of opentelemetry_api.
• Replace protobuf_FOUND with Protobuf_FOUND.

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

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ OWenT (c0e2def)

[codecov]

Codecov Report

Merging #742 (8ef25ca) into main (414a731) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #742   +/-   ##
=======================================
  Coverage   94.75%   94.75%           
=======================================
  Files         217      217           
  Lines        9919     9919           
=======================================
  Hits         9399     9399           
  Misses        520      520           

[reyang]

LGTM.

[lalitb]

@owent - can you rebase this so that we can merge it

[owent]

@owent - can you rebase this so that we can merge it

Done.

[lalitb]

@owent - Can you please rebased once again. There was one unresolved comment so couldn't merge last time. Looks good to merge now.

[halayli]

I am a bit confused by this change. What does STL has to do with GSL? What if someone wants STL but doesn't want GSL?

I feel this trickled from msvc environment. How about having this behavior for windows only at least?

[owent]

I am a bit confused by this change. What does STL has to do with GSL? What if someone wants STL but doesn't want GSL?

I feel this trickled from msvc environment. How about having this behavior for windows only at least?

I'm not familiar with GSL. It seems to import some experimental libraries of STL, and alias to the implement if STL when STL support them. And it support all of MSVC, GCC and Clang, not only for MSVC.

[halayli]

I feel this needs to be a flag. automagically importing a third party library when specifying WITH_STL will surprise developers.

[lalitb]

I feel this needs to be a flag. automagically importing a third party library when specifying WITH_STL will surprise developers.

We only use gsl::span from GSL. But I agree with you. If the WITH_STL option is specified, it should be using std::span if available (for C++20), and fall back to nostd::span instead of gsl::span. Use gsl::span only if developers specifies WITH_GSL option.

Will wait for @maxgolov to reply, probably gsl::span is a more complete implementation than nostd::span and so this logic.

[owent]

I feel this needs to be a flag. automagically importing a third party library when specifying WITH_STL will surprise developers.

Agree too.

[maxgolov]

@halayli @owent @lalitb

std::span is only available since C++20.

Will wait for @maxgolov to reply, probably gsl::span is a more complete implementation than nostd::span and so this logic.

That's true indeed. The only official 1-1 conforming implementation of span for C++14 and above is gsl::span. For example, even Abseil C++ library does not claim to provide 1-1 conformance. GSL span is also described in Cpp Core Guidelines. We agreed to follow these guidelines at the beginning of the project.

I think we should try to use that one for DLL flavor on Windows, as on Windows we support Visual Studio 2015 Update 2 and above in C++14 mode. And with that one should be able to compile gsl::span. That'd be relevant only in v1.1 when we get to publish a DLL.

The option (including GSL span) is described in greater level of detail here in this document in our repo here:
https://github.com/open-telemetry/opentelemetry-cpp/blob/main/docs/building-with-stdlib.md
See Build and Test Considerations section, that elaborates on logical reasoning for the gsl::span.

I have already submitted a change that automagically falls-back to nostd::span in case if GSL span is not available in the build environment, as part of this PR #771 . However, GSL is friendly-licensed and is readily available as a submodule here. GSL span is cross-platform. I'd rather use that one as default span implementation, since it's certainly more trusted than whatever alternate "our own nostd::span" we can come up with. I'd rather outsource the implementation to well-tested and trusted library that is recommended by Cpp Core Guidelines by-default.