[BUILD] Remove WITH_ABSEIL

[owent]

Fixes #3116

Maybe also fixes #3309

Changes

Please provide a brief description of the changes here.

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

[netlify]

✅ Deploy Preview for opentelemetry-cpp-api-docs canceled.

Name	Link
🔨 Latest commit	d8f8163
🔍 Latest deploy log	https://app.netlify.com/sites/opentelemetry-cpp-api-docs/deploys/67ec6a7f54d52700083d1188

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.55%. Comparing base (6175aa0) to head (d8f8163).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3318      +/-   ##
==========================================
- Coverage   89.56%   89.55%   -0.01%     
==========================================
  Files         210      210              
  Lines        6502     6502              
==========================================
- Hits         5823     5822       -1     
- Misses        679      680       +1     

Files with missing lines	Coverage Δ
api/include/opentelemetry/nostd/variant.h	66.67% <ø> (ø)
sdk/src/common/base64.cc	82.93% <ø> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[marcalff]

The WITH_ABSEIL removal itself looks ok.

Not sure why the file opentelemetry-cpp-config.cmake.in was reformated.

Should we change the formatting scripts to ignore this file instead ?

[owent]

The WITH_ABSEIL removal itself looks ok.

Not sure why the file opentelemetry-cpp-config.cmake.in was reformated.

Should we change the formatting scripts to ignore this file instead ?

Maybe the problem is the max length of cmake is default set to 80. I use #[[ ... ]] to prevent cmake-format to change comments by temporary. Can we change the setting the reformat all cmake files in another PR?

[marcalff]

In terms of stability and risk mitigation, there is:

• [RELEASE] Release opentelemetry-cpp 1.20.0 #3328
• [INSTALL] add cmake components to the package #3220
• [BUILD] Remove WITH_ABSEIL #3318 (this PR)

I suspect there will be minor merge conflicts between the WITH_ABSEIL removal and the CMake COMPONENTS refactoring, so this raise the question in which order to merge (for one, the cmake.in template has moved)

@owent Can this PR wait after the opentelemetry-cpp 1.20 release, so we merge all cmake changes at once ?

@dbarker FYI

[marcalff]

LGTM, thanks for the cleanup.

[dbarker]

I'm okay merging this before the components pr and working through the conflicts, they should be manageable.

[ThomsonTan]

Why is absl only required for WITH_OTLP_GRPC, not for all cases?

[owent]

otel-cpp's API no longer depends on abseil-cpp's headers, eliminating path conflicts between internal and external versions of abseil-cpp.

Some components use protobuf, which may include abseil-cpp headers with overlapping paths from the internal version. This necessitates abseil-cpp when using a newer version of protobuf.

The protobuf package file automatically calls find_package(absl CONFIG REQUIRED) and imports it when needed. However, some gRPC versions do not import abseil-cpp, so it only needs to be imported when working with gRPC.

[owent]

In terms of stability and risk mitigation, there is:

• [RELEASE] Release opentelemetry-cpp 1.20.0 #3328
• [INSTALL] add cmake components to the package #3220
• Remove WITH_ABSEIL #3318 (this PR)

I suspect there will be minor merge conflicts between the WITH_ABSEIL removal and the CMake COMPONENTS refactoring, so this raise the question in which order to merge (for one, the cmake.in template has moved)

@owent Can this PR wait after the opentelemetry-cpp 1.20 release, so we merge all cmake changes at once ?

@dbarker FYI

OK, this is a small PR and it can be merged last, I will resolve the conflicts when the time comes.

[marcalff]

All, there is more to this, as it just turned out.

Ubuntu 20.04 is being removed in 3 days, so we MUST upgrade ci as we are still using it.

When upgrading CI, the abseil builds now fails, even when the abseil version used is the same.

From a quick look, this sounds due to a compiler version change (gcc 9 to 11), confusing the abseil code to think it can use std::in_place_t, which is a C++17 construct. It seems the abseil code does not honor the fact we build in C++14 then, and assumes std::in_place_t is ok to use.

Long story short:

• I am very reluctant to debug and fix this upgrade to support some abseil new combination we have not been exposed to before, just for 3 days, given WITH_ABSEIL is going to be removed anyway
• I am now considering to merge WITH_ABSEIL removal now, before shipping opentelemetry-cpp 1.20, to avoid the issue entirely that comes with the ubuntu ci upgrade from 20.04

@open-telemetry/cpp-approvers @dbarker Any thoughts ?

[dbarker]

@marcalff The main risk I see of removing WITH_ABSEIL before the release is the lack of cmake install tests in ci and there is an indication that community vcpkg portfile is requiring it since this PR microsoft/vcpkg#30720. It may be covering up some other issues in the cmake files.

Setting WIITH_ABSEIL=ON ensures find_dependency(absl) is called when a user calls find_package(opentelemetry-cpp CONFIG). This PR will remove that.

Potential issues that may be uncovered:

• There may be unintended linking to absl targets to opentelemetry-cpp targets
• There is an issue I found with calling find_dependency(Protobuf) in opentelemetry-config.cmake on modern versions of protobuf (> 3.22.0) as they require using the CONFIG search mode to find/include the abseil dependency properly, causing abseil link errors. This has existed for a few releases now.

Adding a simple cmake install test based on vcpkg installed dependencies, and a conditional in the opentelemetry-cpp-config.cmake on the protobuf version to use the correct cmake search mode could mitigate the risk.

That said, I'd lean towards releasing this and #3220 together due to the test coverage added.

Would disabling the 20.04 ci runners before the 3 days, then create the release without this PR, and work on the ci upgrade post release work?

[ThomsonTan]

As @dbarker mentioned, I agree on merging this after the release if it could break vcpkg port in potential.

[dbarker]

I've added a comment here #3330 (comment). I'm optimistic that the ci upgrade can proceed by adding the CXX_STANDARD=14 env var to the failing jobs.

[marcalff]

Would disabling the 20.04 ci runners before the 3 days, then create the release without this PR, and work on the ci upgrade post release work?

Yes, this is the safest path I think.

[marcalff]

Do not merge (now) tag: To merge after opentelemetry-cpp 1.20 is released