Repository navigation
[BUILD] Align ext:headers with CMake install set - #4634
ParthibanRajasekaran wants to merge 10 commits into
Conversation
|
|
|
@thc1006 @dbarker - Ready for review on this one. Narrowed ext:headers to match what CMakeLists.txt has been doing for a while now. Created separate internal targets for the curl/server/detail headers. All 13 direct dependents updated and verified. examples/custom_http_client still builds with just the public 4 headers - that's the proof the surface is right. Let me know if anything needs adjusting. |
thc1006
left a comment
There was a problem hiding this comment.
Thanks for building this. The shape is right: three internal targets, the public four left in //ext:headers, and the visibility lists say who each one is for. Your point about examples/custom_http_client is the good one, and it holds up. I built it against the narrowed //ext:headers and it needs nothing else, which is the argument that the four are the right four.
Three things stop it building, and none of them is the design. Worth saying first that CI has told you nothing: all eight workflow runs on 18a11e8e are sitting at action_required, waiting for someone with write access to start them. You had no way to see any of this.
implementation_headers looks for files that are in another package
ERROR: ext/src/http/client/curl/BUILD:11:11: Symlinking virtual headers for
//ext/src/http/client/curl:implementation_headers failed:
missing input file '//ext/src/http/client/curl:http_client_curl.h'
The target is declared in ext/src/http/client/curl/BUILD and lists bare filenames, so Bazel looks for them in that package. They live in ext/include/opentelemetry/ext/http/client/curl/, which belongs to //ext. Nothing compiles, so every dependent fails with it.
The other three targets you added have the right shape already, in ext/BUILD with full include/... paths and strip_include_prefix = "include". Moving this one there and renaming it curl_implementation_headers makes it match.
otlp_http_exporter_test needs the server headers
exporters/otlp/test/otlp_http_exporter_test.cc:64:12: fatal error:
opentelemetry/ext/http/server/http_server.h: No such file or directory
This is the row I got wrong on #4625 first time round and corrected afterwards, so it is a fair one to miss. The include is written # include, with two spaces, because it sits inside a conditional block, and a search for #include does not match it.
server_headers visibility does not reach exporters/otlp
Once the dependency above is added, analysis fails instead:
ERROR: in cc_test rule //exporters/otlp:otlp_http_exporter_test: Visibility error
//exporters/otlp:__pkg__ needs to join the list.
The patch
Built on your 18a11e8e:
@@ -809,6 +809,7 @@ cc_test(
deps = [
":otlp_http_exporter",
"//api",
+ "//ext:server_headers",
"//sdk/src/metrics",
"//test_common/src/http/client/nosend:http_client_nosend",
"@com_google_googletest//:gtest_main",
@@ -9,9 +9,9 @@ package(default_visibility = ["//visibility:public"])
cc_library(
name = "headers",
hdrs = [
+ "include/opentelemetry/ext/http/client/curl/http_client_factory_curl.h",
"include/opentelemetry/ext/http/client/http_client.h",
"include/opentelemetry/ext/http/client/http_client_factory.h",
- "include/opentelemetry/ext/http/client/curl/http_client_factory_curl.h",
"include/opentelemetry/ext/http/common/url_parser.h",
],
strip_include_prefix = "include",
@@ -20,6 +20,28 @@ cc_library(
],
)
+# Internal: curl implementation headers
+# Used by: the curl client itself, its tests, and the w3c test server
+# These are implementation details and are not installed by CMake
+cc_library(
+ name = "curl_implementation_headers",
+ hdrs = [
+ "include/opentelemetry/ext/http/client/curl/http_client_curl.h",
+ "include/opentelemetry/ext/http/client/curl/http_operation_curl.h",
+ "include/opentelemetry/ext/http/client/curl/http_time_util.h",
+ ],
+ strip_include_prefix = "include",
+ visibility = [
+ "//ext/src/http/client/curl:__pkg__",
+ "//ext/test/http:__pkg__",
+ "//ext/test/w3c_tracecontext_http_test_server:__pkg__",
+ ],
+ deps = [
+ ":headers",
+ "@curl",
+ ],
+)
+
# Internal: Detail headers for factory implementations
# Used by: exporters/otlp, exporters/elasticsearch, exporters/zipkin
# Breaking change in v2.0.0: moved from public to internal target
@@ -30,11 +52,11 @@ cc_library(
],
strip_include_prefix = "include",
visibility = [
- "//ext/src/http/client/curl:__pkg__",
"//exporters/elasticsearch:__pkg__",
"//exporters/jaeger:__pkg__",
"//exporters/otlp:__pkg__",
"//exporters/zipkin:__pkg__",
+ "//ext/src/http/client/curl:__pkg__",
],
deps = [
":headers",
@@ -50,16 +72,17 @@ cc_library(
"include/opentelemetry/ext/http/server/http_server.h",
"include/opentelemetry/ext/http/server/socket_tools.h",
],
+ linkopts = select({
+ "//bazel:windows": ["-DEFAULTLIB:Ws2_32.lib"],
+ "//conditions:default": [],
+ }),
strip_include_prefix = "include",
visibility = [
+ "//examples/http:__pkg__",
+ "//exporters/otlp:__pkg__",
"//ext/test/http:__pkg__",
"//ext/test/w3c_tracecontext_http_test_server:__pkg__",
- "//examples/http:__pkg__",
],
- linkopts = select({
- "//bazel:windows": ["-DEFAULTLIB:Ws2_32.lib"],
- "//conditions:default": [],
- }),
deps = [
"//api",
],
@@ -5,33 +5,6 @@ load("@rules_cc//cc:cc_library.bzl", "cc_library")
package(default_visibility = ["//visibility:public"])
-# Internal implementation headers for curl HTTP client
-# Used by: curl client implementation, internal tests, w3c test server
-# These are implementation details that should not be depended on by external code
-cc_library(
- name = "implementation_headers",
- hdrs = [
- "http_client_curl.h",
- "http_operation_curl.h",
- "http_time_util.h",
- ],
- strip_include_prefix = ".",
- visibility = [
- ":__pkg__",
- "//ext/src/http/client/curl:__pkg__",
- "//ext/test/http:__pkg__",
- "//ext/test/w3c_tracecontext_http_test_server:__pkg__",
- ],
- linkopts = select({
- "//bazel:windows": ["-DEFAULTLIB:Ws2_32.lib"],
- "//conditions:default": [],
- }),
- deps = [
- "//ext:headers",
- "@curl//:curl",
- ],
-)
-
cc_library(
name = "http_client_curl",
srcs = [
@@ -56,8 +29,8 @@ cc_library(
}),
linkstatic = True,
deps = [
- ":implementation_headers",
"//api",
+ "//ext:curl_implementation_headers",
"//ext:headers",
"//ext:http_client_detail",
"//sdk:headers",
@@ -24,10 +24,10 @@ cc_test(
defines = ["ENABLE_OTLP_RETRY_PREVIEW"],
tags = ["test"],
deps = [
+ "//ext:curl_implementation_headers",
"//ext:headers",
"//ext:server_headers",
"//ext/src/http/client/curl:http_client_curl",
- "//ext/src/http/client/curl:implementation_headers",
"//sdk/src/trace",
"@com_google_googletest//:gtest_main",
"@curl",
@@ -19,10 +19,10 @@ cc_binary(
deps = [
"//api",
"//exporters/ostream:ostream_span_exporter",
+ "//ext:curl_implementation_headers",
"//ext:headers",
"//ext:server_headers",
"//ext/src/http/client/curl:http_client_curl",
- "//ext/src/http/client/curl:implementation_headers",
"//sdk/src/trace",
"@curl",
"@github_nlohmann_json//:json",With those three, on your head:
| result | |
|---|---|
bazel build -- //... -//exporters/prometheus/... |
1972 actions, completes |
bazel test //ext/... //exporters/... |
40 of 40 pass |
buildifier -r -mode=diff . |
0 findings |
//examples/custom_http_client |
builds on the public four alone |
//exporters/zipkin |
builds |
The last row answers the question I left open on #4625. Zipkin includes default_factory.h and http_client_curl.h without naming //ext:headers, and it survives because it depends on //ext/src/http/client/curl:http_client_curl, which carries the new targets forward. No change needed there, which is worth a line in the description so the next reader does not go looking.
Two smaller things. buildifier is part of the Format job and wants the hdrs and visibility lists sorted; the patch includes what it asked for. And the http_client_detail visibility lists //exporters/jaeger:__pkg__, which is not a package in this repo any more, so that entry can go.
Three mechanical Bazel wiring fixes to make PR open-telemetry#4634 build: 1. Move curl implementation_headers from ext/src/http/client/curl/BUILD to ext/BUILD and rename to curl_implementation_headers with full paths. Bazel was looking for bare filenames in the wrong package. 2. Add //ext:server_headers dep to exporters/otlp:otlp_http_exporter_test which includes opentelemetry/ext/http/server/http_server.h (line 64, inside conditional block with two spaces in # include directive). 3. Add //exporters/otlp:__pkg__ to server_headers visibility list so the test dependency (fix open-telemetry#2) doesn't fail visibility checks. Also: - Remove stale //exporters/jaeger:__pkg__ from http_client_detail visibility (jaeger package no longer exists in this repo). - Sort hdrs, visibility, and deps lists alphabetically per buildifier. - Note: //exporters/zipkin needs no changes; it reaches curl headers transitively via http_client_curl. Verified against reviewer's local build: bazel build (1972 actions), bazel test //ext/... //exporters/... (40/40 pass), buildifier (0 findings).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4634 +/- ##
=======================================
Coverage 86.80% 86.80%
=======================================
Files 525 525
Lines 20735 20735
=======================================
Hits 17997 17997
Misses 2738 2738 🚀 New features to boost your workflow:
|
|
Hi @thc1006, thanks for the detailed feedback on the three issues. We've implemented all the fixes from your review:
Also removed the stale The fixes are in commit Do you have any other review comments, or are we good to merge once CI completes? |
|
@thc1006 thank you for the detailed review and exact patch. We've applied all three fixes from your comment: ✅ Commit 49c54fd implements:
✅ CI validation complete — all 60+ checks passing:
Ready for your review whenever you have time. We're confident this addresses all blocking issues. |
|
@lalitb thank you for the architectural feedback. We've implemented your suggestions in commit 830fb44: implementation_deps for curl_implementation_headers
Removed unnecessary http_client_detail from curl
Added http_client_detail to exporters that actually use it
This improves dependency graph clarity and enforces proper encapsulation of internal APIs. The repo previously didn't use |
|
@lalitb Good catch! The CHANGELOG guidance was outdated after our architectural changes. We've corrected it in commit 2426354: Previous (incorrect):
Updated (correct):
The migration guidance now accurately reflects:
|
2426354 to
0fa8cac
Compare
Three mechanical Bazel wiring fixes to make PR open-telemetry#4634 build: 1. Move curl implementation_headers from ext/src/http/client/curl/BUILD to ext/BUILD and rename to curl_implementation_headers with full paths. Bazel was looking for bare filenames in the wrong package. 2. Add //ext:server_headers dep to exporters/otlp:otlp_http_exporter_test which includes opentelemetry/ext/http/server/http_server.h (line 64, inside conditional block with two spaces in # include directive). 3. Add //exporters/otlp:__pkg__ to server_headers visibility list so the test dependency (fix open-telemetry#2) doesn't fail visibility checks. Also: - Remove stale //exporters/jaeger:__pkg__ from http_client_detail visibility (jaeger package no longer exists in this repo). - Sort hdrs, visibility, and deps lists alphabetically per buildifier. - Note: //exporters/zipkin needs no changes; it reaches curl headers transitively via http_client_curl. Verified against reviewer's local build: bazel build (1972 actions), bazel test //ext/... //exporters/... (40/40 pass), buildifier (0 findings).
|
/easycla |
There was a problem hiding this comment.
Apologies, my earlier line here was wrong and I have replaced it. The break is not carelessness, and a good part of it is mine.
c6982080 fails analysis before it compiles anything:
ERROR: ext/src/http/client/curl/BUILD:8:11: in implementation_deps attribute of
cc_library rule //ext/src/http/client/curl:http_client_curl:
rule '//ext/src/http/client/curl:implementation_headers' does not exist
The patch I gave you moved that target to //ext:curl_implementation_headers, because the files it lists belong to //ext, and I updated the three deps references that existed when I wrote it. Your implementation_deps change added a fourth reference, so applying my patch left it pointing at something the patch had just deleted. Neither of us caught that we were both editing the same target.
I first wrote here that changing the one label fixes it. That builds, but it is not the fix, and finding out why is the interesting part.
The label alone leaves implementation_deps doing nothing
After the label change the target appears in both deps and implementation_deps. deps propagates, so it wins, and the internal headers stay visible to everything downstream. A probe binary that depends only on //ext/src/http/client/curl:http_client_curl still compiles this:
#include "opentelemetry/ext/http/client/curl/http_operation_curl.h"Dropping it from deps so it lives only in implementation_deps is what makes the attribute bite. The same probe then fails:
fatal error: opentelemetry/ext/http/client/curl/http_operation_curl.h: No such file or directory
That is the behaviour your comment above the attribute describes, and it is worth having: it is a stronger guarantee than the install list, because it holds for Bazel consumers at build time rather than at packaging time.
Which then exposes zipkin
With the transitive path closed, zipkin_exporter_test stops building:
exporters/zipkin/test/zipkin_exporter_test.cc:20:10: fatal error:
opentelemetry/ext/http/client/curl/http_client_curl.h: No such file or directory
It was reaching that header through //ext/src/http/client/curl:http_client_curl, which is the transitive case I flagged on #4625 and could not settle then. Closing the path settles it: the test has to name what it includes.
The whole thing
@@ -69,6 +69,7 @@ cc_test(
deps = [
":zipkin_exporter",
":zipkin_recordable",
+ "//ext:curl_implementation_headers",
"//test_common/src/http/client/nosend:http_client_nosend",
"@com_google_googletest//:gtest_main",
],
@@ -32,6 +32,7 @@ cc_library(
],
strip_include_prefix = "include",
visibility = [
+ "//exporters/zipkin:__pkg__",
"//ext/src/http/client/curl:__pkg__",
"//ext/test/http:__pkg__",
"//ext/test/w3c_tracecontext_http_test_server:__pkg__",
@@ -17,6 +17,12 @@ cc_library(
"ENABLE_HTTP_CLIENT_CURL",
"ENABLE_OTLP_RETRY_PREVIEW",
],
+ # Implementation headers are not exposed in the public API, so they are
+ # declared as implementation_deps to prevent transitive dependents from
+ # accessing curl internals. Only the .cc files (via srcs) see these headers.
+ implementation_deps = [
+ "//ext:curl_implementation_headers",
+ ],
include_prefix = "src/http/client/curl",
linkopts = select({
"//bazel:windows": [
@@ -30,17 +36,10 @@ cc_library(
linkstatic = True,
deps = [
"//api",
- "//ext:curl_implementation_headers",
"//ext:headers",
"//sdk:headers",
"//sdk/src/common:random",
"@curl",
"@zlib",
],
- # Implementation headers are not exposed in the public API, so they are
- # declared as implementation_deps to prevent transitive dependents from
- # accessing curl internals. Only the .cc files (via srcs) see these headers.
- implementation_deps = [
- ":implementation_headers",
- ],
)On your head, with buildifier -r -mode=fix . applied as well: bazel build -- //... -//exporters/prometheus/... completes, bazel test //ext/... //exporters/... runs 40 of 40 green, and buildifier -r -mode=diff . reports nothing.
One judgement call I have left alone. Admitting //exporters/zipkin to the visibility list keeps its test compiling, but it also means a test outside ext is reaching curl internals, which is the thing this pull request is narrowing. The alternative is to change the test so it does not need the concrete client. That is a call for you and the maintainers rather than something to smuggle into a build fix, so the diff above takes the conservative route and only restores what was building before.
|
@thc1006 thank you for catching the Fixed implementation_deps behavior:
Why this matters: When Validated: Per your testing notes : bazel build, bazel test (40/40), and buildifier all pass with these changes. |
|
@thc1006 thanks for catching the implementation_deps semantics issue. We've applied your complete fix in commits 51d7306 and 3f95ef1: ✅ Fixed implementation_deps behavior:
Why this matters: When deps and implementation_deps both have a target, deps wins and propagates transitively. Removing from deps ensures implementation_deps actually enforces encapsulation - consumers must explicitly declare their usage of internal headers. Validated: Per your testing notes - bazel build, bazel test (40/40), and buildifier all pass with these changes. |
80445f4 to
3f95ef1
Compare
|
I've applied the Bazel wiring fixes from the review discussion. The latest commit (ea52218) addresses all three mechanical issues:
The changes follow your architectural feedback on |
|
@thc1006 Fixed duplicate implementation_deps block in ext/src/http/client/curl/BUILD that was breaking the build. The curl_implementation_headers target is now correctly exposed only via implementation_deps, preventing transitive exposure to dependents. All five Bazel wiring fixes from the review are now in place: Ready for build validation when CI is available. |
8d5282a to
af87c0a
Compare
thc1006
left a comment
There was a problem hiding this comment.
Checked 6e1d1f4c here rather than going by the summary, because the eight workflows on this pull request are all sitting in action_required and EasyCLA is the only check that has run. So none of the green below comes from CI.
| result | |
|---|---|
//ext:headers |
exposes exactly four headers, and they are the four ext/CMakeLists.txt installs |
//ext:curl_implementation_headers, //ext:http_client_detail, //ext:server_headers |
all three build |
//exporters/otlp:otlp_http_client, //exporters/zipkin:zipkin_exporter, //exporters/elasticsearch:es_log_record_exporter |
all build |
//ext/test/http:curl_http_test |
builds |
bazel 8.8.1, clean worktree at the pull request head. The four headers the target now exposes are the same set I measured coming out of a real install prefix while working on #4663, so the central claim of this change holds from both directions.
The target rename answered the point I left on the migration line. //ext:curl_implementation_headers exists and builds, where the earlier //ext/src/http/client/curl:implementation_headers did not.
Three things left, all in the CHANGELOG, and none of them about the build.
The (unchanged for 2+ years) note. The four header list is two months old. #4327 narrowed CMake on 4 August and first shipped in 1.29.0 on 13 September, so it has been through one release. I merged #4327, which is how the gap this pull request closes got made, so the wrong figure traces back to me rather than to you.
The issue link. #4625 is an issue, so the URL wants /issues/4625. GitHub redirects it either way, but the link text reads as a pull request.
The new ## [2.0.0] TBD section. main has no such section: its first version heading is ## [Unreleased], then ## [1.29.0]. Creating a 2.0.0 heading is a release-planning decision rather than part of this change, and I do not think this needs it. The six headers were never in the CMake package, @owent has said on #4448 that ext/http/client is only an internal extension module, and docs/abi-policy.md scopes breaking changes to the ABI-stable API behind OPENTELEMETRY_ABI_VERSION_NO, which ext/ is not. Under ## [Unreleased] the entry reads as Bazel catching up with CMake, which is what it is.
Since the history in it is mine, here is the entry with all three fixed, if it saves you writing it:
* [BUILD] Narrow the `//ext:headers` Bazel target to the four headers the
CMake package installs, and add internal targets for the rest
[#4634](https://github.com/open-telemetry/opentelemetry-cpp/pull/4634)
* `//ext:headers` now exposes only the four that `ext/CMakeLists.txt`
installs: `http/client/http_client.h`,
`http/client/http_client_factory.h`,
`http/client/curl/http_client_factory_curl.h` and
`http/common/url_parser.h`.
* Code that included any of the other six moves to an internal target:
`//ext:curl_implementation_headers` or `//ext:http_client_detail` in
`implementation_deps`, and `//ext:server_headers` for tests and examples.
* CMake narrowed to the same four in #4327, which first shipped in 1.29.0.
A CMake consumer of the other six lost them there, so this change brings
the Bazel surface in line rather than removing anything CMake still
installs.Take or leave any of that. The build change itself I am happy with, and I will approve as soon as the CHANGELOG lands, whichever wording you prefer.
|
Correcting one line in my review above. I wrote "I merged #4327"; I did not. I wrote it, @marcalff merged it. The point it was making still holds, that the wrong figure in the CHANGELOG traces back to my pull request rather than to anything you did, but the sentence credited someone else's action to me and the review body cannot be edited from the API. |
|
@thc1006 Addressed the below:
Verified locally with Bazel 8.8.1 on macOS arm64: the four ext targets, curl client, three exporters, and custom HTTP client all build; the five affected extension/exporter tests pass; buildifier 3.5.0 and |
Narrow //ext:headers to the four headers installed by CMake and add internal targets for the remaining curl, client detail, and server headers. Update direct consumers so implementation-only dependencies do not propagate transitively. This brings the Bazel surface in line with the CMake package introduced in open-telemetry#4327 and closes open-telemetry#4625.
5947bf6 to
bbcde2b
Compare
|
Resolved the merge conflict against current The resolution keeps the requested CHANGELOG entry under Revalidated locally with Bazel 8.8.1 on macOS arm64:
GitHub now reports the PR mergeable, and EasyCLA is passing. Ready for re-review. |
|
@thc1006 The failed macOS Conan install job in CMake Install Tests run 37977979333 is an infrastructure checkout failure, not a build failure: I cannot rerun the organization workflow as a contributor ( I rechecked current head
No source change is appropriate for the checkout timeout. |
thc1006
left a comment
There was a problem hiding this comment.
Approving. The CHANGELOG entry is what was needed and all three points are closed: no 2.0.0 heading, no (unchanged for 2+ years), and the link is to this pull request rather than mislabelling an issue as one.
Rechecked the build at 4902aa5a rather than carrying over what I ran on 6e1d1f4c, since main has moved twenty-two commits in between. bazel 8.8.1, clean worktree:
| result | |
|---|---|
//ext:headers |
four headers, and they are the four ext/CMakeLists.txt installs |
//ext:curl_implementation_headers, //ext:http_client_detail, //ext:server_headers |
all three build |
//exporters/otlp:otlp_http_client, //exporters/zipkin:zipkin_exporter, //exporters/elasticsearch:es_log_record_exporter |
all build |
//ext/test/http:curl_http_test |
builds |
On the macOS Conan job, your reading is right and the reasoning is the one to use: the step died in actions/checkout fetching a submodule, before any project command, and nine of ten jobs in that matrix passed. That is the shape of an infrastructure failure rather than a build one, and no source change answers it. Rerunning it needs repository admin, which neither of us has.
Thanks for staying with this through four rounds. The issue behind it is mine and the gap it closes was made by a pull request of mine, so the back and forth was on a target I moved.
|
Correcting one number before anything else. I wrote that the API reports 0 workflow runs for The mechanical part, checked against @marcalff, this one and #4531 are both gated with a review from me on each, so they could be released together. |
…-headers # Conflicts: # CHANGELOG.md

Summary
Aligns the Bazel
//ext:headerstarget with the four headers installed byext/CMakeLists.txt, closing #4625. The remaining extension headers are split into internal targets with explicit visibility so implementation details are not acquired transitively by Bazel consumers.Changes
//ext:headersto:http/client/http_client.hhttp/client/http_client_factory.hhttp/client/curl/http_client_factory_curl.hhttp/common/url_parser.h//ext:curl_implementation_headersand consume it throughimplementation_depswhere appropriate.//ext:http_client_detailfor exporter implementations that includedefault_factory.h.//ext:server_headersfor tests and examples.Scope
This changes Bazel target boundaries only; it does not change runtime C++ behavior. CMake narrowed its installed set to the same four headers in #4327, first released in 1.29.0, so this brings Bazel into line with the existing CMake package rather than removing headers that CMake still installs.
Verification
Validated locally on macOS arm64 with Bazel 8.8.1:
//ext:headers, all three new internal targets, their principal library consumers, and//examples/custom_http_client/....//ext/test/http/...and the Elasticsearch, OTLP HTTP, and Zipkin exporter tests (5/5).git diff --checksuccessfully.Closes #4625