Skip to content

Move libcurl/libuv HTTP client into acyclic http_client component - #8285

Open
Amaury Chamayou (achamayou) wants to merge 1 commit into
achamayou-crispy-snifflefrom
achamayou-psychic-fishstick
Open

Move libcurl/libuv HTTP client into acyclic http_client component#8285
Amaury Chamayou (achamayou) wants to merge 1 commit into
achamayou-crispy-snifflefrom
achamayou-psychic-fishstick

Conversation

@achamayou

Copy link
Copy Markdown
Member

Third and top layer of a stack breaking cyclic source dependencies out of http. Makes the now-acyclic http source component protocol-focused by moving its libcurl/libuv client integration into a separate acyclic http_client component.

Relates to #3517 (does not close it).

Base: achamayou-crispy-sniffle (PR #8284), itself based on PR #8281 and PR #8278.

Changes:

  1. Moved the internal header src/http/curl.h to src/http_client/curl.h, preserving the ccf::curl namespace, APIs, behavior, libcurl error checking, and libuv lifecycle exactly. No forwarding header/shim left at the old path since it's an internal header outside include/ccf.
  2. Updated every in-tree include of http/curl.h to http_client/curl.h (host, node, snapshots, and the test).
  3. Moved src/http/test/curl_test.cpp to src/http_client/test/curl_test.cpp and updated its CMake registration. The curl_test target and e2e invocation name are unchanged.
  4. Updated scripts/source-dependencies.json:
    • Narrowed http from ["ccf-api", "crypto", "ds", "uv"] to ["ccf-api", "crypto", "ds"].
    • Added the new http_client component with ["ccf-api", "ds", "uv"], confirmed via scripts/check-source-dependencies.py (no violations).
  5. Updated .github/instructions/reviewing.instructions.md to reference the new curl.h path.

No public API or runtime behavior changes, no compatibility shim, no namespace rename, no unrelated curl refactor.

Verification:

  • scripts/check-source-dependencies.py: no violations. http now only depends on ccf-api, crypto, ds; http_client is acyclic and outside the cyclic core (no dependency on host/node/enclave).
  • Full ninja build succeeded (all targets, including ccf, ccf_launcher, curl_test, verify_attestation, and node/frontend test targets that include the header).
  • ./tests.sh -L unit: 58/58 passed.
  • ./curl_test run directly: 9/15 test cases pass; 6 fail due to no network connectivity in this sandbox (Could not connect to server), consistent with the same baseline failures already established on earlier layers in this environment - not a regression.
  • scripts/ci-checks.sh -f: all checks passed.

For the stack coordinator: please perform native stack registration; I have not registered stack metadata myself.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

src/http_client/curl.h is not self-contained due to missing standard library includes and has a signed/unsigned seek offset computation that can mis-handle negative offsets.

Pull request overview

This PR continues the acyclic-component refactor by moving the libcurl/libuv HTTP client integration out of the protocol-focused http component into a new http_client component, updating all in-tree includes and dependency-policy declarations accordingly.

Changes:

  • Moved the internal curl integration header to src/http_client/curl.h and updated all affected includes.
  • Relocated the curl unit test to src/http_client/test/curl_test.cpp and updated its CMake registration.
  • Updated scripts/source-dependencies.json and the reviewer guidance doc to reflect the new component boundary/path.

Custom instructions used:

  • .github/instructions/reviewing.instructions.md
File summaries
File Description
src/snapshots/fetch.h Switched include to http_client/curl.h.
src/node/recovery_decision_protocol.cpp Switched include to http_client/curl.h.
src/node/quote_endorsements_client.h Switched include to http_client/curl.h.
src/node/node_state.h Switched include to http_client/curl.h.
src/node/jwt_key_auto_refresh.h Switched include to http_client/curl.h.
src/host/run.cpp Switched include to http_client/curl.h.
src/http_client/curl.h Added new location for the curl/libuv integration header.
src/http_client/test/curl_test.cpp Updated include to http_client/curl.h in moved test.
CMakeLists.txt Updated curl_test source path to new location.
scripts/source-dependencies.json Declared new http_client component and narrowed http dependencies.
.github/instructions/reviewing.instructions.md Updated documentation reference to the new curl header path.
Review details
  • Files reviewed: 10/11 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Move src/http/curl.h to src/http_client/curl.h, and src/http/test/curl_test.cpp
to src/http_client/test/curl_test.cpp, updating all in-tree includes and the
CMake registration. No forwarding header is left at the old path since curl.h
is an internal header.

Narrow the http component's declared internal dependencies to
[ccf-api, crypto, ds] now that the libuv-dependent client code has moved out,
and add the new http_client component with [ccf-api, ds, uv].

This keeps http protocol-focused and acyclic, and keeps the new http_client
component acyclic and outside the cyclic core.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants