feat(farmhash): add LLAR formula - #137
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: google/farmhash recipe
Solid new recipe that closely follows the conventions of sibling recipes (recp/cglm, json-c). Strengths worth calling out:
onTestcorrectly derives include/lib flags frominstallDirrather than build metadata, so it works on a cache hit whereonBuildis skipped (same pattern documented in cglm/json-c).- License is copied to
licenses/COPYINGwith propermkdirAlland error propagation. - The
filterallowlists exactly the known option names and restricts values toON/OFF, so untrusted option strings never reach a path, exec argument, or CMake string define — a clean trust boundary. - The CMakeLists
__builtin_expectdetection faithfully transcribes the upstreamMakefile.amprobe; theif(NOT ...)chain is logically correct.
The inline comments below are non-blocking suggestions (mostly documentation/consistency with sibling recipes). No correctness blockers found.
| defaults { | ||
| "shared": "OFF", | ||
| "fPIC": "ON", | ||
| "no_builtin_expect": "OFF", |
There was a problem hiding this comment.
Document the options, especially no_builtin_expect. Sibling recipes document each option at its declaration (cglm explains shared/CGLM_USE_TEST; json-c explains shared/fPIC and the static/fPIC interaction). This recipe declares shared, fPIC, and no_builtin_expect with no comments, and no_builtin_expect is the most non-obvious option in the repo. A short comment here clarifying that OFF = auto-detect (compiler probe) and ON = force-disable would match repo convention.
|
|
||
| include(GNUInstallDirs) | ||
|
|
||
| if(NOT FARMHASH_NO_BUILTIN_EXPECT) |
There was a problem hiding this comment.
Non-obvious no_builtin_expect semantics are correct but undocumented. Tracing the logic: with no_builtin_expect=ON the probe is skipped (if(NOT ON) is false), leaving FARMHASH_HAS_BUILTIN_EXPECT empty, so line 18's if(NOT FARMHASH_HAS_BUILTIN_EXPECT) is true and the macro is defined — force-disable works. With OFF (default) the probe runs and auto-detects. So OFF means "auto-detect," not "builtin is used." This ON=force / OFF=autodetect contract, plus the fact that the macro is exported PUBLIC (propagates to consumers), is worth a brief comment either here or in the .gox.
| os.writeFile(filepath.join(ctx.SourceDir, "CMakeLists.txt"), cmakeLists, 0o644)! | ||
|
|
||
| shared := slices.contains(target.options["shared"], "ON") | ||
| fPIC := slices.contains(target.options["fPIC"], "ON") |
There was a problem hiding this comment.
fPIC is redundant when shared=ON. Shared libraries are always position-independent, so CMAKE_POSITION_INDEPENDENT_CODE has no effect when shared=ON. The current filter accepts all four shared/fPIC combinations, producing two functionally identical variants for shared=ON. Consider rejecting the redundant combination in filter (as mature Conan-style recipes do) to avoid building duplicate packages.
| lastErr! | ||
|
|
||
| if slices.contains(target.options["shared"], "ON") { | ||
| os.setenv("LD_LIBRARY_PATH", filepath.join(installDir, "lib"))! |
There was a problem hiding this comment.
os.setenv overwrites LD_LIBRARY_PATH/DYLD_LIBRARY_PATH instead of prepending. This unconditionally replaces any existing value rather than prepending (installDir/lib:$LD_LIBRARY_PATH). For a test process this is mostly a robustness concern, but silently dropping an operator-provided library search path can change which shared objects resolve at runtime. Consider prepending to the existing value.
| onBuild ctx => { | ||
| installDir := ctx.outputDir | ||
|
|
||
| cmakeLists := ctx.Proj.readFile("0d859a811870d10f53a594927d0d0b97573ad06d/CMakeLists.txt")! |
There was a problem hiding this comment.
Version hash literal is duplicated. The commit hash 0d859a... appears in both fromVer (line 28) and this readFile path. On a future version bump both must change in lockstep or the build silently reads the wrong CMakeLists. If the toolchain exposes a version-dir-relative path helper, prefer deriving this path from ctx rather than re-embedding the literal.
1 participant
Closes #100.
Adds the
google/farmhashFormula for source ref0d859a811870d10f53a594927d0d0b97573ad06d, including the Conan-exported CMake entrypoint, shared/fPIC/no_builtin_expect options, installed license, consumer metadata, and an independent C++ consumer test.Validated locally with LLAR
b4164750da04f08c179965eb2f7b5087b1fc260c:/tmp/llar-b416 test -v ./google/farmhash@0d859a811870d10f53a594927d0d0b97573ad06d --os linux --arch amd64onTest.