Remove logger dependency on libccf - #7977
Conversation
Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>
Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>
|
@copilot that feels like it's going to lead to duplicated symbols, especially if applications include logger.h too. |
Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>
Moved the thread ID implementation back out of the header and into a single object compiled into |
Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>
…-dependency-libccf
The logger headers (ccf/ds/logger.h) call ccf::threading::get_current_thread_id, whose definition previously lived in libccf. Low-level libraries such as ccfcrypto use the logger but are themselves dependencies of ccf, creating an implicit circular dependency that was worked around with a per-app linker flag and by compiling the thread-id source into individual test/bench/tool binaries. Move the implementation into a new ccf_threading static library (cmake/threading.cmake), linked publicly by ccfcrypto so every logger user resolves the symbol through it. Repoint ccf_tasks and ccf_kv - which use the logger but not crypto - to depend on ccf_threading directly instead of ccfcrypto. Bump the version to 7.0.8 and add a CHANGELOG entry describing the build-graph change for downstream consumers that link CCF component libraries directly.
…-dependency-libccf # Conflicts: # CHANGELOG.md
There was a problem hiding this comment.
Pull request overview
This PR removes an implicit circular dependency where the framework logger relied on get_current_thread_id() coming from libccf, by moving thread-id ownership into a new foundational ccf_threading library and updating the build graph so low-level libraries (eg ccfcrypto) can resolve the symbol without linking libccf.
Changes:
- Introduces
ccf_threading(owningsrc/threading/thread_ids.cpp) and links it intoccfcrypto,ccf_kv, andccf_tasks. - Removes the previous
src/enclave/thread_local.cppcompilation/linker workarounds from apps, tests, benchmarks, and tools. - Adds a logger unit test covering thread-id assignment/reset and per-thread initialization.
Custom instructions used:
.github/copilot-instructions.md.github/instructions/reviewing.instructions.md
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/perf-system/submitter/CMakeLists.txt | Drops explicit compilation of the removed thread_local.cpp from perf submitter sources. |
| src/threading/thread_ids.cpp | Centralizes thread-id generator + per-thread storage in a standalone TU for reuse across libraries. |
| src/ds/test/logger.cpp | Adds a focused unit test exercising the thread-id API used by logger headers. |
| include/ccf/threading/thread_ids.h | Minor update (no API change) alongside the relocation of the implementation. |
| CMakeLists.txt | Wires in cmake/threading.cmake, removes thread_local.cpp from build/test targets, and links ccf_threading where needed. |
| cmake/threading.cmake | Adds the new minimal ccf_threading static library target. |
| cmake/crypto.cmake | Links ccf_threading into ccfcrypto so logger users resolve thread-id symbols without libccf. |
| cmake/common.cmake | Removes the previous blanket inclusion of thread_local.cpp in test/bench wrappers. |
| cmake/ccf_app.cmake | Removes the app linker workaround that force-resolved get_current_thread_id from ccf. |
| CHANGELOG.md | Documents the logger/build-graph change for downstream consumers. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Amaury Chamayou <amaury@xargs.fr>
Closes #7596.
logger.hpulled inget_current_thread_id()fromlibccf.a, creating an implicit circular dependency for libraries such asccfcryptothat are themselves dependencies ofccf.Thread ID ownership
src/threading/thread_ids.cpp, compiled into the newccf_threadinglibrary, so logger users share a single implementation without linkinglibccf.get_current_thread_id(),set_current_thread_id(), andreset_thread_id_generator().Build graph cleanup
src/enclave/thread_local.cpp.thread_local.cppinto tests, benchmarks, and tools.get_current_thread_idresolution fromccf.Coverage
Library dependency graphs
The graphs show every CCF library that directly uses the logger or thread-ID APIs and the internal CMake edges that connect them. Logger-independent or vendored library targets (
http_parser,quickjs, andevercbor) are omitted. Arrows point from a library to a library it depends on. Solid arrows are existing CMake links, dashed arrows are undeclared symbol dependencies, and thick arrows are the new public links.In the Before graph, the thread-ID symbols are drawn separately from
ccf / libccf.aeven though they were defined in that archive. This keeps both sides of the circular dependency visible without routing every dashed edge back through theccfnode.Before
For libraries that
ccflinks, the undeclared thread-ID symbol dependencies complete cycles back intolibccf.a.ccf_launcherhad the same undeclared dependency; CCF applications linked both libraries and required the linker workaround to forceget_current_thread_id()out oflibccf.a.After
ccf_threadingis now the leaf dependency that provides the thread-ID symbols. The three new public links give every other logger-using library a transitive path to it, so none depends back onccf.