[BUILD] Narrow ext:headers to public API and create internal targets - #4634
Open
ParthibanRajasekaran wants to merge 6 commits into
Open
ParthibanRajasekaran wants to merge 6 commits into
ParthibanRajasekaran wants to merge 6 commits into
Conversation
Replace glob pattern with explicit list of stable headers that form the public HTTP client API. Aligns Bazel with CMakeLists.txt which has maintained this narrower surface for 2+ years. Public surface: - http_client.h - http_client_factory.h - http_client_factory_curl.h - url_parser.h These are the only headers users should depend on directly. Internal headers (curl, detail, server) are implementation details that may change and are moved to internal targets. Fixes issue open-telemetry#4625. See CHANGELOG.md for breaking change notice in v2.0.0.
Curl client internals (http_client_curl.h, http_operation_curl.h, http_time_util.h) are implementation details reached through the public http_client_factory_curl.h abstraction. Move to internal target. Used by: - ext/src/http/client/curl library - ext/test/http tests - ext/test/w3c_tracecontext_http_test_server Adds ws2_32 linkage on Windows (required by http_operation_curl.h). Part of v2.0.0 breaking change to align Bazel with CMake.
Embedded HTTP server headers (http_server.h, socket_tools.h) are test and example infrastructure, not part of the public API. Move to internal target with limited visibility. These headers should eventually move to test_common per issue open-telemetry#4332. Adds ws2_32 linkage on Windows (required by socket_tools.h). Part of v2.0.0 breaking change.
All 13 direct dependents updated to use correct internal targets: - ext/test/http, ext/test/w3c_tracecontext_http_test_server depend on curl:implementation_headers and server:server_headers - examples/http depends on server:server_headers - exporters/otlp (4 locations), exporters/elasticsearch, exporters/zipkin depend on http_client_detail for factory helper - examples/custom_http_client and test_common unchanged (use public only) Verified: - All 13+ targets build on Linux, macOS, Windows - Transitive dependencies correct (zipkin through test_common) - examples/custom_http_client builds unchanged (public surface complete) Part of v2.0.0 breaking change to align Bazel with CMake.
Bazel target //ext:headers narrowed to 4 public headers, matching CMakeLists.txt's install manifest for 2+ years. Internal headers (curl, detail, server) moved to separate targets. This aligns Bazel with CMake and prevents accidental exposure of implementation details. Users depending on removed headers must migrate to new internal targets. See CHANGELOG.md for upgrade path. Fixes issue open-telemetry#4625.
Author
|
@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. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR narrows the public Bazel target
//ext:headersto match CMakeLists.txt's explicit install list, addressing issue #4625. The split creates separate internal targets for implementation details, improving build system consistency and preventing accidental API surface expansion.Changes
Narrow public API (
//ext:headers): 4 stable headers onlyCreate internal targets:
//ext/src/http/client/curl:implementation_headers- Curl internals (used by curl client, tests, w3c server)//ext:http_client_detail- Factory helper (used by exporters)//ext:server_headers- Server infrastructure (used by tests and examples)Update 13+ dependents: All targets updated to use correct internal targets
Document breaking change: v2.0.0 CHANGELOG entry with migration examples
Verification
Breaking Changes
This is a v2.0.0 major version bump (not backward compatible). The 6 headers below are no longer exposed in the public API:
opentelemetry/ext/http/client/curl/http_client_curl.hopentelemetry/ext/http/client/curl/http_operation_curl.hopentelemetry/ext/http/client/curl/http_time_util.hopentelemetry/ext/http/client/detail/default_factory.hopentelemetry/ext/http/server/http_server.hopentelemetry/ext/http/server/socket_tools.hMigration: Use internal targets instead (see CHANGELOG.md for examples)
Alignment
Closes #4625