[CI] Upgrade to abseil 20240116.1 (CMake only)

[marcalff]

Upgrade to abseil 20240116.1

Changes

• Upgrade to abseil 20240116.1, but only for the CMake build.
• The bazel build is unchanged by this PR.

See related issue #2619.

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

[lalitb]

Bazel build is failing with:

external/com_google_absl/absl/base/policy_checks.h:79:2: error: #error "C++ versions less than C++14 are not supported."

[marcalff]

Bazel build is failing with:

external/com_google_absl/absl/base/policy_checks.h:79:2: error: #error "C++ versions less than C++14 are not supported."

Yes, saw this.

Bazel is supposed to use --cxxopt=-std=c++14, I don't understand why it still fails.

[keith]

I suggest #2600 to fix that C++14 issue

[keith]

I assume the core issue is that https://github.com/open-telemetry/opentelemetry-cpp/pull/2600/files#diff-46043b297959e098526162a2aa686e9f27bf41881f5d0ed6cdab21e4cb2acd13L73 this line only has --cxxopt and not --host_cxxopt

[keith]

with my change + this change I do see what I think are real compiler errors tho (on macOS), but we'll see what you see on CI:

bazel-out/darwin_arm64-fastbuild/bin/api/_virtual_includes/api/opentelemetry/nostd/./internal/absl/types/../utility/utility.h:344:33: error: reference to 'decay_t' is ambiguous
          std::tuple_size<absl::decay_t<Tuple>>::value>{});
                                ^
external/com_google_absl/absl/meta/type_traits.h:304:1: note: candidate found by name lookup is 'absl::lts_20240116::decay_t'
using decay_t = typename std::decay<T>::type;
^
bazel-out/darwin_arm64-fastbuild/bin/api/_virtual_includes/api/opentelemetry/nostd/./internal/absl/types/../utility/../base/internal/../../meta/type_traits.h:627:1: note: candidate found by name lookup is 'absl::otel_v1::decay_t'
using decay_t = typename std::decay<T>::type;
^
fatal error: too many errors emitted, stopping now [-ferror-limit=]
20 errors generated.

[marcalff]

Thanks @keith for the help.

Any insight on what is going on here is most welcome, personally I can not make any sense of the error reported during the build.

[keith]

I noticed that as well. I discovered that passing --@//api:with_abseil=true seems to make the build pass. So it must have something to do with the various HAVE_ABSEIL conditionals in

opentelemetry-cpp/api/include/opentelemetry/nostd/variant.h

Line 17 in 3f8d954

# ifndef HAVE_ABSEIL

being off?

[keith]

I think that must be because this target:

opentelemetry-cpp/exporters/otlp/BUILD

Line 139 in 3f8d954

"@com_google_absl//absl/strings",

always includes absl? so if these aliases

opentelemetry-cpp/api/include/opentelemetry/nostd/internal/absl/types/variant.h

Lines 52 to 57 in 3f8d954

	namespace absl {
	OTABSL_NAMESPACE_BEGIN
	using std::bad_variant_access;
	using std::get;
	using std::get_if;
	using std::holds_alternative;

are loaded in that dependency tree, there will be conflicts? It looks to me like abseil also unconditionally comes through the grpc dependency, so even if that direct dep is removed the same error occurs.

That doesn't explain why the update triggered it. Best I can assume is something in the dep tree changed to include the conflicting symbols? Doesn't explain why cmake doesn't have the same issue (maybe that difference isn't surprising)

[keith]

Might be a crazy suggestion but would removing that absl condition be an option? to always enable it, since it's always coming from grpc anyways

[marcalff]

Interesting.

opentelemetry-cpp in general may or may not use abseil (HAVE_ABSEIL), both builds are supported.

Now, the OTLP exporter does have a hard dependency on protobuf, which now requires to use abseil.

So, when building with the OTLP exporter, the build should use abseil.

There is also the issue, undecided at this point, if the opentelemetry-cpp build should use a OPENTELEMETRY_HAVE_ABSEIL to avoid name collision with HAVE_ABSEIL that can be set elsewhere (like by protobuf).

[marcalff]

CMake contains the following logic:

if(WITH_OTLP_GRPC OR WITH_OTLP_HTTP)
  find_package(Protobuf)
  if(Protobuf_VERSION AND Protobuf_VERSION VERSION_GREATER_EQUAL "3.22.0")
    if(NOT WITH_ABSEIL)
      message(
        FATAL_ERROR
          "Protobuf 3.22 or upper require abseil-cpp(Recommended version: 20230125 or upper)"
      )
    endif()
  endif()

Not sure what needs to be done in bazel for the same, so that the otlp exporter is always build with abseil.

[keith]

I think in bazel that condition doesn't currently make sense to exist since grpc / protobuf are never optional. which makes it sound to me like absl should just be non-optional in bazel too if so?

[keith]

I don't really suggest this option, but here's something that does work:

diff --git a/api/BUILD b/api/BUILD
index 0ae0061f..d196c992 100644
--- a/api/BUILD
+++ b/api/BUILD
@@ -25,34 +25,46 @@ string_flag(
     values = CPP_STDLIBS,
 )
 
+_DEFINES_SELECT = select({
+    ":set_cxx_stdlib_none": [],
+    ### automatic selection
+    ":set_cxx_stdlib_best": ["OPENTELEMETRY_STL_VERSION=(__cplusplus/100)"],
+    # See https://learn.microsoft.com/en-us/cpp/build/reference/zc-cplusplus
+    ":set_cxx_stdlib_best_and_msvc": ["OPENTELEMETRY_STL_VERSION=(_MSVC_LANG/100)"],
+    ### manual selection
+    ":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": [],
+})
+
 cc_library(
-    name = "api",
+    name = "api_with_abseil",
     hdrs = glob(["include/**/*.h"]),
-    defines = select({
-        ":with_external_abseil": ["OPENTELEMETRY_HAVE_ABSEIL"],
-        "//conditions:default": [],
-    }) + select({
-        ":set_cxx_stdlib_none": [],
-        ### automatic selection
-        ":set_cxx_stdlib_best": ["OPENTELEMETRY_STL_VERSION=(__cplusplus/100)"],
-        # See https://learn.microsoft.com/en-us/cpp/build/reference/zc-cplusplus
-        ":set_cxx_stdlib_best_and_msvc": ["OPENTELEMETRY_STL_VERSION=(_MSVC_LANG/100)"],
-        ### manual selection
-        ":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": [],
-    }),
+    defines = ["OPENTELEMETRY_HAVE_ABSEIL"] + _DEFINES_SELECT,
     strip_include_prefix = "include",
     tags = ["api"],
-    deps = select({
-        ":with_external_abseil": [
-            "@com_google_absl//absl/base",
-            "@com_google_absl//absl/strings",
-            "@com_google_absl//absl/types:variant",
-        ],
-        "//conditions:default": [],
+    deps = [
+        "@com_google_absl//absl/base",
+        "@com_google_absl//absl/strings",
+        "@com_google_absl//absl/types:variant",
+    ],
+)
+
+cc_library(
+    name = "api_without_abseil",
+    hdrs = glob(["include/**/*.h"]),
+    defines = _DEFINES_SELECT,
+    strip_include_prefix = "include",
+    tags = ["api"],
+)
+
+alias(
+    name = "api",
+    actual = select({
+        ":with_external_abseil": ":api_with_abseil",
+        "//conditions:default": ":api_without_abseil",
     }),
 )
 
diff --git a/examples/grpc/BUILD b/examples/grpc/BUILD
index b1d04e9f..e45c5458 100644
--- a/examples/grpc/BUILD
+++ b/examples/grpc/BUILD
@@ -43,7 +43,7 @@ cc_binary(
     deps = [
         "messages_cc_grpc",
         ":tracer_common",
-        "//api",
+        "//api:api_with_abseil",
         "//sdk/src/trace",
         "@com_github_grpc_grpc//:grpc++",
     ],
@@ -59,7 +59,7 @@ cc_binary(
     deps = [
         "messages_cc_grpc",
         ":tracer_common",
-        "//api",
+        "//api:api_with_abseil",
         "//sdk/src/trace",
         "@com_github_grpc_grpc//:grpc++",
     ],
diff --git a/exporters/otlp/BUILD b/exporters/otlp/BUILD
index a9f1c887..b45e9d73 100644
--- a/exporters/otlp/BUILD
+++ b/exporters/otlp/BUILD
@@ -1,10 +1,10 @@
 # Copyright The OpenTelemetry Authors
 # SPDX-License-Identifier: Apache-2.0
 
-package(default_visibility = ["//visibility:public"])
-
 load("//bazel:otel_cc_benchmark.bzl", "otel_cc_benchmark")
 
+package(default_visibility = ["//visibility:public"])
+
 cc_library(
     name = "otlp_recordable",
     srcs = [
@@ -58,6 +58,7 @@ cc_library(
         "otlp_grpc",
     ],
     deps = [
+        "//api:api_with_abseil",
         "//ext:headers",
         "//sdk/src/common:global_log_handler",
         "@com_github_grpc_grpc//:grpc++",
@@ -131,7 +132,7 @@ cc_library(
         "otlp_http_log",
     ],
     deps = [
-        "//api",
+        "//api:api_with_abseil",
         "//ext/src/http/client/curl:http_client_curl",
         "//sdk:headers",
         "//sdk/src/common:base64",

It works by splitting the api cc_library into 2, instead of splitting attributes on that library based on the abseil condition, then libraries that "know" they will have abseil via grpc use the //api:api_with_abseil library instead. I think the potential downside is that both api_with_abseil and api_without_abseil could end up in the dependency tree of a single build, which sounds like it could lead to confusing issues.

[marcalff]

@lalitb @ThomsonTan @keith Please take another look.

I ended up removing the api:with_abseil option entirely from bazel, given that it is not optional.

If we take this path, I will also add notes about breaking changes in the changelog.

[marcalff]

Any idea about the link failures on bazel windows ?

[lalitb]

I ended up removing the api:with_abseil option entirely from bazel, given that it is not optional.

Would this mean that with bazel build, there won't be possibility of building with internal abseil snapshot, which is meant to be ABI safe ?

[keith]

That sounds good to me! But I don't know what the use case was where folks were opting out of it with bazel either. I don't recognize the windows link error. Might not be new if that configuration wasn't tested on CI before. I don't have a windows machine to debug. I wonder if there's a related abseil issue?

[marcalff]

I ended up removing the api:with_abseil option entirely from bazel, given that it is not optional.

Would this mean that with bazel build, there won't be possibility of building with internal abseil snapshot, which is meant to be ABI safe ?

Correct, although the concept of ABI itself does not really apply to bazel, which never installs a component somewhere, and never take dependencies from installed locations. Bazel builds tends to compile the entire dependency tree from source, making ABI compatibility a moot point.

I am abandoning this path anyway, because bazel still fails on some platforms (windows), even after a simplification.

I suspect there are many independent issues in the bazel build that accumulated other time, making it impossible hard to maintain, as each failure needs to be analyzed in depth and resolved.

[marcalff]

@lalitb @ThomsonTan @esigo Please take yet another look.

This change affects CMake only, the bazel build needs to be fixed independently with #2619.

Given how we have a bring your own dependency model, the discrepancy in CI between CMake and bazel looks acceptable to me.

[lalitb]

LGTM, as we have separate tracking issue for bazel build. Thanks.