chore: upgrade corrade to cci.20260327 to fix macOS build#769
chore: upgrade corrade to cci.20260327 to fix macOS build#769leoparente wants to merge 10 commits intodevelopfrom
Conversation
Pin corrade to commit 22e7ffc6 (2026-03-27) via a local conan recipe, replacing the old 2020.06 release which fails to compile on modern macOS. Update PluginManager API usage across all plugin headers to match the new corrade API (StringView constructor params, Array<String> search paths, StringView pluginInterface). Update CoreRegistry.cpp to handle StringView/StringIterable return types from pluginList() and provides(). Remove the -include vector macOS workaround from CI and add a conan export step for the local recipe to all build workflows. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fcd624b2de
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Upgrades Corrade to a newer CCI snapshot to restore compatibility with modern macOS toolchains, and updates the codebase to match Corrade PluginManager API changes.
Changes:
- Pin Corrade to
corrade/cci.20260327via a local Conan recipe underconan/corrade/. - Update plugin base classes and derived plugin constructors to use
Corrade::Containers::StringView, and adjustpluginInterface()/pluginSearchPaths()return types. - Update
CoreRegistryand CI workflows to accommodate Corrade string/iterable API changes and to export the local Corrade recipe in CI.
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/inputs/pcap/PcapInputModulePlugin.h | Update plugin constructor to accept StringView. |
| src/inputs/netprobe/NetProbeInputModulePlugin.h | Update plugin constructor to accept StringView. |
| src/inputs/mock/MockInputModulePlugin.h | Update plugin constructor to accept StringView. |
| src/inputs/flow/FlowInputModulePlugin.h | Update plugin constructor to accept StringView. |
| src/inputs/dnstap/DnstapInputModulePlugin.h | Update plugin constructor to accept StringView. |
| src/InputModulePlugin.h | Update PluginManager-facing API types (StringView, Array<String>). |
| src/handlers/pcap/PcapHandlerModulePlugin.h | Update plugin constructor to accept StringView. |
| src/handlers/netprobe/NetProbeHandlerModulePlugin.h | Update plugin constructor to accept StringView. |
| src/handlers/net/v2/NetHandlerModulePlugin.h | Update plugin constructor to accept StringView. |
| src/handlers/net/v1/NetHandlerModulePlugin.h | Update plugin constructor to accept StringView. |
| src/handlers/mock/MockHandlerModulePlugin.h | Update plugin constructor to accept StringView. |
| src/handlers/input_resources/InputResourcesHandlerModulePlugin.h | Update plugin constructor to accept StringView. |
| src/handlers/flow/FlowHandlerModulePlugin.h | Update plugin constructor to accept StringView. |
| src/handlers/dns/v2/DnsHandlerModulePlugin.h | Update plugin constructor to accept StringView. |
| src/handlers/dns/v1/DnsHandlerModulePlugin.h | Update plugin constructor to accept StringView. |
| src/handlers/dhcp/DhcpHandlerModulePlugin.h | Update plugin constructor to accept StringView. |
| src/handlers/bgp/BgpHandlerModulePlugin.h | Update plugin constructor to accept StringView. |
| src/HandlerModulePlugin.h | Update PluginManager-facing API types (StringView, Array<String>). |
| src/CoreRegistry.cpp | Adapt logging/map keys to Corrade string/view types via STL conversions. |
| src/AbstractPlugin.h | Update base plugin constructor/search paths to Corrade container types. |
| conanfile.py | Bump Corrade requirement/tool-requirement to cci.20260327. |
| conan/corrade/conanfile.py | Add local Corrade Conan recipe for the pinned CCI snapshot. |
| conan/corrade/conandata.yml | Define Corrade source URL + sha256 for cci.20260327. |
| conan/corrade/cmake/conan-corrade-vars.cmake | Add Corrade CMake module vars + Corrade::rc imported target wiring. |
| .github/workflows/build-release.yml | Export local Corrade recipe and update Conan cache key. |
| .github/workflows/build-develop.yml | Export local Corrade recipe, remove macOS workaround flags, update Conan cache keys. |
| .github/workflows/build_debug.yml | Export local Corrade recipe and update Conan cache key. |
| .github/workflows/build_cross.yml | Export local Corrade recipe for cross builds. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…d verification - Use Corrade::InPlaceInit (not Containers::InPlaceInit, which is deprecated) - Wrap StringView returns with std::string() for spdlog/fmt logging and map keys - Add PointerStl.h for Pointer<T> → unique_ptr<T> implicit conversion - Add Macros.h + AbstractManager.h to static_plugins.h for CORRADE_AUTOMATIC_INITIALIZER - Add StringStl.h + std::string() wraps in main.cpp for pluginList() logging - Use _s literal suffix in pluginInterface() to set Global StringViewFlag (required by new corrade at runtime) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Fix build_cross.yml: conan export path was wrong (src/conan/ → conan/) - Fix unit-tests-mac: add tools.apple:sdk_path to conan profile so CMakeToolchain uses the full SDK path instead of bare 'macosx', resolving 'initializer_list' not found compile error - Add missing conan export step to build-win64, build-app-image-x64 jobs - Fix entrypoint.sh: copy conan/ directory and run conan export before cmake, fixing Docker-based arm64/debug builds - Expand all conan cache keys to include conan/corrade/** so changes to conandata.yml or cmake helpers invalidate cache correctly - Set required_conan_version = ">=2.0" in local Conan recipe (recipe uses Conan 2 imports; >=1.52.0 would silently accept Conan 1) - Fix find_program in conan-corrade-vars.cmake: PATHS ENV PATH syntax (was split across lines causing ambiguity) and add NOTFOUND guard Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
leoparente
left a comment
leoparente
left a comment
There was a problem hiding this comment.
All review comments addressed in commit 18b8bc5:
Copilot: Export pinned Corrade recipe in every Conan entrypoint — Fixed. Added conan export step to build-win64 (build-release.yml) and build-app-image-x64 (build-develop.yml). Fixed .github/actions/build-cpp/entrypoint.sh to copy conan/ directory and run conan export before cmake (covers all Docker-based arm64/debug jobs).
Copilot: find_program malformed in conan-corrade-vars.cmake — Partially addressed. The PATHS ENV PATH syntax is actually valid CMake (ENV followed by variable name is a path modifier), so no change needed there. Added the recommended NOTFOUND guard before get_filename_component/add_executable to fail early with a clear message when cross-compiling without corrade-rc on PATH.
Copilot: required_conan_version = ">=1.52.0" should be >=2.0 — Fixed.
Copilot: Cache keys should include conan/corrade/** — Fixed in all workflow files (build-develop.yml, build-release.yml, build_debug.yml, build_cross.yml).
Codex P1: build_cross.yml conan export path wrong — Fixed (src/conan/corrade/ → conan/corrade/). This was causing the actual CI failure.
Additional fix (CI failure): unit-tests-mac was failing with initializer_list not found because Conan's CMakeToolchain sets CMAKE_OSX_SYSROOT=macosx (bare string) when building corrade from source. Fixed by appending tools.apple:sdk_path to the conan profile after conan profile detect, so the toolchain uses the resolved SDK path.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 18b8bc53b6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The repo is checked out into \$GITHUB_WORKSPACE/src (actions/checkout with path: src), so the conan/corrade recipe is at src/conan/corrade/, not conan/corrade/. Update both the export path and the cache key hashFiles pattern accordingly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Fixed in commit 0bf6279: the checkout uses |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0bf6279439
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- build-release.yml prebuild-package: add export before cmake configure - build-release.yml release: add export before cmake configure - build_cross.yml pkvisor-cli: install conan and export before cmake configure - code-ql.yml: export after fresh clone, before cmake configure Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Fixed in commit d51f546: added
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d51f546f5e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
.conanrc sets conan_home=./build (relative). The pkvisor job checks out into src/ and runs conan install with working-directory=src, so its conan home resolves to src/build/. The export step was running from the workspace root, writing to a different conan home (build/ instead of src/build/), making the recipe invisible to conan install. Fix: add working-directory: src to the export step so both steps share the same conan home. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Root cause found for cross-compilation failure: Fixed in fc16118: added |
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
musl only provides the XSI strerror_r (returns int). Corrade's ErrorString.cpp falls into the GNU branch (expects char*) when _GNU_SOURCE is defined by GCC even on musl targets, causing a compile error: "invalid conversion from int to const char*". Add a portability patch that extends the existing POSIX branch condition to also cover non-glibc Linux (detected via __linux__ && !__GLIBC__), so musl builds use the int-returning XSI variant that actually exists. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Fixed cross-compile error in commit 1b7698c. Root cause: Fix: Added a patch to the local Corrade recipe ( |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b7698cd1e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Replace broken patch file with replace_in_file() in conanfile.py for the musl strerror_r fix — patch-ng context lines were at wrong offset; replace_in_file does an exact string search and is more robust - Add #include <Corrade/Containers/StringStl.h> to AbstractPlugin.cpp and Policies.cpp — both use std::string(plugin()) / std::string(StringView) but were missing the header that provides the STL interop conversion Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Fixed in commit 328b63c: Broken patch (strerror_r musl fix): Replaced the patch file with Missing |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 328b63c6ff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…terop Taps::info_json() assigns plugin() and pluginInterface() (both StringView) directly to JSON values. The conversion worked transitively on some compilers, but the explicit include makes the dependency clear and avoids fragile transitive reliance on CORRADE_BUILD_DEPRECATED pulling it in. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
P1 (Taps.h JSON serialization) — Fixed in commit 2670aee: added P2 (local builds require manual conan export) — Won't fix / by design. The vendored recipe is intentional: |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2670aee429
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The vendored corrade/cci.20260327 recipe must be exported before any conan install/cmake invocation on a clean cache. Update: - docker/Dockerfile: COPY conan/ and run conan export before cmake - README.md: add conan export step to the local build instructions Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Fixed in commit 93710a0:
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Summary
22e7ffc6(2026-03-27) via a local conan recipe atconan/corrade/, replacing2020.06which fails to compile on modern macOSconst std::string&→const Corrade::Containers::StringView,pluginInterface()return type, andpluginSearchPaths()return typeCoreRegistry.cppto handleStringView/StringIterablereturn types frompluginList()andprovides()-include vectormacOS workaround from CI; adds aconan exportstep for the local recipe to all four build workflowsTest plan
unit-tests-mac) builds and passesunit-tests-linux) still builds and passesbuild-win64) still buildsbuild_cross) still works🤖 Generated with Claude Code