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

Better handling of OPENTELEMETRY_STL_VERSION. #2490

Closed
wants to merge 9 commits into from
Closed

Better handling of OPENTELEMETRY_STL_VERSION. #2490

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7baf5f8
Sketch a way to set OPENTELEMETRY_STL_VERSION under bazel.
bcsgh Jan 16, 2024
3cc5c1b
Add some '#if defined' to generate a better error if OPENTELEMETRY_ST…
bcsgh Jan 17, 2024
77b6912
Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…
bcsgh Jan 17, 2024
b543295
Special case MSVC (because it prefers breaking correct future usage o…
bcsgh Jan 18, 2024
daca53b
Special case MSVC in version guard (and update message).
bcsgh Jan 18, 2024
69154a1
Merge branch 'main' into bcsgh/with_cxx_stdlib
ThomsonTan Jan 18, 2024
697794d
Make formatting happy.
bcsgh Jan 18, 2024
d486cd7
Make formatting (even more) happy.
bcsgh Jan 18, 2024
163b341
Merge branch 'master' into HEAD
bcsgh Jan 31, 2024
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
30 changes: 29 additions & 1 deletion api/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# Copyright The OpenTelemetry Authors
# SPDX-License-Identifier: Apache-2.0

load("@bazel_skylib//rules:common_settings.bzl", "bool_flag")
load("@bazel_skylib//rules:common_settings.bzl", "bool_flag", "string_flag")

package(default_visibility = ["//visibility:public"])

Expand All @@ -10,12 +10,35 @@ bool_flag(
build_setting_default = False,
)

CPP_STDLIBS = [
"none",
"best",
"2014",
"2017",
"2020",
"2023",
]

string_flag(
name = "with_cxx_stdlib",
values = CPP_STDLIBS,
build_setting_default = "best",
)

cc_library(
name = "api",
hdrs = glob(["include/**/*.h"]),
defines = select({
":with_external_abseil": ["HAVE_ABSEIL"],
"//conditions:default": [],
}) + select({
":set_cxx_stdlib_none": [],
":set_cxx_stdlib_best": ["OPENTELEMETRY_STL_VERSION=(__cplusplus/100)"],
lalitb marked this conversation as resolved.
Show resolved Hide resolved
":set_cxx_stdlib_2014": ["OPENTELEMETRY_STL_VERSION=2014"],
":set_cxx_stdlib_2017": ["OPENTELEMETRY_STL_VERSION=2017"],
":set_cxx_stdlib_2020": ["OPENTELEMETRY_STL_VERSION=2020"],
":set_cxx_stdlib_2023": ["OPENTELEMETRY_STL_VERSION=2023"],
"//conditions:default": [],
}),
strip_include_prefix = "include",
tags = ["api"],
Expand All @@ -33,3 +56,8 @@ config_setting(
name = "with_external_abseil",
flag_values = {":with_abseil": "true"},
)

[config_setting(
name = "set_cxx_stdlib_%s" % v,
flag_values = {":with_cxx_stdlib": v},
) for v in CPP_STDLIBS]
7 changes: 7 additions & 0 deletions api/include/opentelemetry/version.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,13 @@
#include "opentelemetry/common/macros.h"
#include "opentelemetry/detail/preprocessor.h"

#if defined(OPENTELEMETRY_STL_VERSION)
# if OPENTELEMETRY_STL_VERSION > (__cplusplus/100)
# pragma message OPENTELEMETRY_STRINGIFY(OPENTELEMETRY_STL_VERSION) " vs. " OPENTELEMETRY_STRINGIFY(__cplusplus)
# error "OPENTELEMETRY_STL_VERSION set to version newer than compilation version."
Copy link
Contributor

Choose a reason for hiding this comment

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

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."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

# endif
#endif

#ifndef OPENTELEMETRY_ABI_VERSION_NO
# define OPENTELEMETRY_ABI_VERSION_NO 1
#endif
Expand Down
Loading