[BUILD] Compile each installed ext header on its own in the install tests

[thc1006]

Follow-up to #4310.

The gap

Three of the nine headers installed under opentelemetry/ext are compiled by the install tests today: common/url_parser.h, client/http_client.h and client/curl/http_client_factory_curl.h. The rest ship to consumers without any job compiling them. #4299 was that situation: file_http_server.h carried a _WIN32-only error from the day it was added and nothing in the tree compiled it.

Why the headers are not just added to the existing test sources

Several headers in one translation unit cannot show that each of them stands on its own, because an include earlier in the file supplies whatever a later header is missing. On this tree, dropping <string> or <cstdint> from common/url_parser.h fails when that header is compiled by itself and still passes when all nine are compiled together. That is the same masking that hid the missing <algorithm> in #4299, where socket_tools.h happened to provide it.

What this adds

otel_add_header_probes() compiles each listed header in a translation unit of its own, where the header is the only include and appears twice so that a broken include guard is reported wherever a second inclusion would be ill-formed. The target is an OBJECT library, so the probes are compiled and not linked, and UNITY_BUILD is off so a unity build cannot merge them back into a single translation unit. Headers are listed explicitly rather than globbed, so renaming or removing one fails here.

The three ext components list all nine installed ext headers between them, each under the target whose usage requirements it needs.

What the probes found

The curl probes failed on both Conan jobs with fatal error: curl/curl.h: No such file or directory. http_client_curl.h and http_operation_curl.h are installed and include <curl/curl.h>, and their class definitions name CURL and curl_slist, while CURL::libcurl is a private dependency of opentelemetry_http_client_curl. The installed target therefore exports it as $<LINK_ONLY:CURL::libcurl>, which keeps the link requirement and leaves out curl's include directories.

Making that dependency public would reach every user of the OTLP HTTP exporter, since the curl target is linked publicly up through opentelemetry_exporter_otlp_http, and the factory design keeps the client implementation out of that interface deliberately. The probe target links CURL::libcurl itself instead, which the ext_http_curl install test already requires, so the headers are still compiled and the package exports what it did before.

That leaves the underlying oddity for #1980: those two headers are installed and expose curl types, yet by the factory design nobody is meant to include them directly.

The Windows component list

ext_http was missing from the Windows cmake.install.test component list while ext_http_curl was present. The component is installed there, since curl is enabled whenever an HTTP exporter is, so its component test simply never ran on Windows.

Verification

Built and installed the package, then ran the ext component install tests against the install prefix:

• 100% tests passed, 0 tests failed out of 9 across the three components.
• All nine probe objects are produced, one per header.
• Removing #include <string> from the installed url_parser.h makes ctest report component-ext_common-build-test ***Failed, and only ..._url_parser_h.cc fails, with 'string' in namespace 'std' does not name a type. Restoring it passes again. That the installed copy is what breaks also confirms the probes consume the installed package rather than the source tree.

Limits worth knowing

• ext_common installs the whole ext header tree with a wildcard, while the client and curl probes only exist where those components do. In a configuration without curl, such as the Windows DLL install test, all nine headers are still installed and only the three ext_common headers are probed. Aligning header installation with component ownership is the fix for that, and it is [CMake Build] Install necessary header files only? #1980.
• Listing a header here says that it is installed, not that it is supported public API. detail/default_factory.h describes itself as internal and the server headers are a test server. They are probed because they ship, and [CMake Build] Install necessary header files only? #1980 may well stop installing some of them.
• What is new for MSVC is that each ext server header is compiled standalone from the install tree. curl_http_test.cc already includes http_server.h, so MSVC does compile that header today as part of the curl tests.
• The ext_common probes also run wherever that component is tested, which is the Windows DLL install test and the opentracing shim install test.
• Out of scope: the api, sdk and exporter components, include-what-you-use, template or class instantiation, link and runtime checks, and a completeness check that would fail when a newly installed header is missing from a list.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.14%. Comparing base (98445d3) to head (96bd128).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4315      +/-   ##
==========================================
- Coverage   81.15%   81.14%   -0.01%     
==========================================
  Files         446      446              
  Lines       18922    18922              
==========================================
- Hits        15355    15353       -2     
- Misses       3567     3569       +2     

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

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[dbarker]

Thanks for looking into this. While technically the right approach from a pure CMake perspective (the opentelemetry_http_client_curl target exposes the curl.h header and so ideally we make CURL public too), the change has some unintended consequences to be aware of.

Curl is now privately linked to avoid leaking the dependency to users of the otlp http exporter. Creation of the curl client in the OTLP http client/exporter is through the http client factory intentionally to avoid exposing the client implementation details.

It is important to follow the target linking path up to the exporter to see how the exporter links to the clients now:

1. opentelemetry_http_client target links opentelemetry_http_client_curl publicly
2. opentelemetry_exporter_otlp_http_client links opentelemetry_http_client publicly
3. opentelemetry_exporter_otlp_http links opentelemetry_exporter_otlp_http_client publicly

Please see docs/cpp-sdk-factory-design.md for more of the design intent behind the factories.

[thc1006]

That design intent helps, thank you. I traced the header back to its own target but not the target forward to the exporter, so I missed that making curl public would reach every user of the OTLP HTTP exporter.

Reverted. The probe target links CURL::libcurl itself now, which the ext_http_curl install test already requires, so both curl headers are still compiled and the package exports what it did before.

What is left is an observation rather than a change: http_client_curl.h and http_operation_curl.h are installed and name CURL and curl_slist in their class definitions, while the factory design means nobody should be including them directly. That pairing looks like the case #1980 describes, and I can note it there if it is useful.

[dbarker]

What is left is an observation rather than a change: http_client_curl.h and http_operation_curl.h are installed and name CURL and curl_slist in their class definitions, while the factory design means nobody should be including them directly. That pairing looks like the case #1980 describes, and I can note it there if it is useful.

That is a correct observation, and it's reasonable to assume the vast majority of users never include detail headers like http_operation_curl.h. Removing this one from the install is technically possible as http_client_factory_curl.h only depends on the abstract http_client_factory.h, and nothing else in the installed header includes it. However, since it has shipped in the package for years, removal would be a breaking change and needs to be discussed and documented. That may best belong in a new issue/PR that contributes to #1980 by defining a header install manifest for each ext component.

This case is one of several in the codebase where a target installs headers that reference a dependency's types without exporting that dependency publicly. Ideally the factory targets keep implementation details and their dependencies fully private and every header a target installs is compilable using only that target's exported usage requirements. This can be fixed with minimal CMake refactoring by moving the implementation into separate targets linked privately to the factory targets. To this point it hasn't been an issue that has taken priority.

[dbarker]

Hi @thc1006, Thanks a lot for the thorough investigation and documentation of the header issues in ext. The probe mechanism is well-built. My concern is sequencing and long-term approach to address testing headers in isolated TUs.

Several of the nine headers being probed are ones that should be removed from the install when addressing #1980 (we shouldn't install internal detail files hidden from public headers, test only files, etc.). Probing them implies they should be a supported public interface, when they are simply installed by accident.

After we have install manifests, we can lean on CMake's native FILE_SET HEADERS + VERIFY_INTERFACE_HEADER_SETS (available in CMake 3.24), so we're maintaining a header manifest rather than custom probe infrastructure. The manifest then drives both install and verification. It verifies from the build tree rather than the installed prefix, but I think that's okay and the right trade for using a supported CMake feature.

I propose the following next steps:

1. Split the ext_http Windows component-list fix out of this PR. It's a good standalone fix and I'm happy to review it immediately.
2. Create a PR defining an explicit install manifest for the ext headers (replacing the glob), removing headers that were never meant to be public. This is the direct fix for issues like #4299 and contributes to #1980.
3. Create an issue to track raising the CMake minimum to 3.24. Once that minimum version is set, we can adopt target_sources(FILE_SET HEADERS) + VERIFY_INTERFACE_HEADER_SETS, with a dedicated CI job that compiles every declared public header in its own TU. The manifest from step 2 becomes the FILE_SET contents, so nothing from that work is thrown away.
4. Close this PR in favor of the above.

Does this sound reasonable?

Would you be interested in taking on steps 1 and 2?

[dbarker]

http_server.h is a case of a file that should not be included in the installed package. It is only used in tests

[thc1006]

That sequencing is better than what I had, and yes to steps 1 and 2.

The point I had wrong is the one you make first. I treated "installed" as the contract worth testing, and wrote in the description that listing a header says it ships rather than that it is supported. Probing http_server.h still reads as an endorsement whatever the description says, and a manifest that stops installing it is the actual fix rather than a test that pins the accident in place.

VERIFY_INTERFACE_HEADER_SETS is also clearly the right destination. It does what the probe target does, without the project carrying the machinery.

One thing worth recording for step 3, since it cost me a day to find. The probes ran against the installed prefix, and that is how the curl failure surfaced: http_client_curl.h and http_operation_curl.h compile fine from the build tree, where curl's include directories are present, and fail from the install tree because CURL::libcurl is exported as $<LINK_ONLY:...>. Build-tree verification cannot see that class of problem, which is the same class you describe in the other thread. Not an argument against the trade you are making, just a gap to be aware of when the FILE_SET job lands.

Step 1 is up as #4326. I will follow with the ext install manifest, and I will open the CMake 3.24 issue unless you would rather write that one yourself.

Closing this in favour of that plan.