feat(ctrl-c): add LLAR formula - #142
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
FennoAI Review — evgenykislov/ctrl-c v1.0.0
Clean, well-structured recipe that follows the repo's core conventions: forced lib/ layout, cache-hit-safe onTest deriving flags from the install dir, and consistent -I/-L/-lctrl-c flags between build metadata and the test. No security concerns. A few findings below.
Documentation gap (repo convention): Sibling recipes (recp/cglm, json-c/json-c) consistently comment their non-obvious choices — why CMAKE_INSTALL_LIBDIR is forced to lib, the shared/fPIC defaults' mapping to the Conan recipe, the filter intent, and the cache-hit rationale for deriving test flags from the install dir. This recipe has essentially no comments. Adding short rationale comments (especially on the lib override and the LICENSE copy, which is novel vs. siblings) would match the established convention and protect future editors.
| c.define "CTRL_C_SRC_DIR", ctx.SourceDir | ||
| c.define "CMAKE_INSTALL_LIBDIR", "lib" | ||
| c.defineBool "BUILD_SHARED_LIBS", shared | ||
| c.defineBool "CMAKE_POSITION_INDEPENDENT_CODE", fPIC |
There was a problem hiding this comment.
[P1] shared=ON with fPIC=OFF is allowed but produces an invalid build
A consumer can request shared=ON, fPIC=OFF. A shared library requires position-independent code, but this passes CMAKE_POSITION_INDEPENDENT_CODE=OFF while BUILD_SHARED_LIBS=ON, which will typically fail to link on most toolchains.
Suggest either forcing PIC on for the shared case, e.g. fPIC := shared || slices.contains(target.options["fPIC"], "ON"), or rejecting the invalid combination in filter.
| os.setenv("LD_LIBRARY_PATH", filepath.join(installDir, "lib"))! | ||
| os.setenv("DYLD_LIBRARY_PATH", filepath.join(installDir, "lib"))! |
There was a problem hiding this comment.
[P3] LD_LIBRARY_PATH/DYLD_LIBRARY_PATH overwrite instead of prepend
For the shared build the test sets LD_LIBRARY_PATH/DYLD_LIBRARY_PATH to the install lib dir, discarding any existing value. Prepending (installDir/lib + ":" + os.getenv(...)) is safer if toolchain/runtime libs live elsewhere. Low priority — usually harmless in a sandboxed runner.
| fPIC := slices.contains(target.options["fPIC"], "ON") | ||
| c := cmake.new(ctx.SourceDir, filepath.join(ctx.SourceDir, "_build"), installDir) | ||
| c.define "CTRL_C_SRC_DIR", ctx.SourceDir | ||
| c.define "CMAKE_INSTALL_LIBDIR", "lib" |
There was a problem hiding this comment.
[P2] Undocumented CMAKE_INSTALL_LIBDIR override
CMAKE_INSTALL_LIBDIR is forced to lib, and the hardcoded -L.../lib flags at lines 64 and 83 depend on this. Since CMakeLists.txt includes GNUInstallDirs (which selects lib64 on some distros), a future edit removing this override would silently break the metadata and test flags on lib64 systems. json-c documents exactly this choice; a one-line comment here would prevent that regression.
1 participant
Closes #96.
Adds the
evgenykislov/ctrl-cLLAR Formula for upstreamv1.0.0from the Conan Center snapshotffe30df101afd4dc95aac2f14b25bf345e64d7be.shared/fPICoptions.libctrl-c, and MIT license with direct consumer metadata.v1.0.0), so no comparator is needed.Validation:
git diff --cached --check(clean). Local LLAR/CI tests were intentionally not awaited per request.