feat(whereami): add LLAR formula - #150
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: add gpakosz/whereami LLAR formula
Overall this is a clean, well-formed addition that follows the sibling formulas (Cglm_llar.gox, Jsonc_llar.gox) closely. Nice touches: pinning to a full commit SHA, vendoring a CMakeLists.txt (upstream ships none), dual-license installation (LICENSE.MIT + LICENSE.WTFPLv2), and running the consumer test from an independent tree so it also works on a cache hit. versions.json matches the sibling schema. No performance or security concerns.
A few points worth a look, left inline. None are blocking.
Design note (non-blocking): the filter (lines 50-62) rejects a target if it carries any option name other than shared/fPIC, whereas the cglm sibling only validates the values of the option it recognizes. If the toolchain layer may inject additional option keys (build type, arch, …), this stricter filter could reject otherwise-valid targets. If the strict behavior is intentional, consider a one-line comment saying so.
Maintainability (non-blocking): unlike the cglm/json-c siblings, this formula has almost no explanatory comments. The two most valuable to add: (1) why a CMakeLists.txt is vendored at all — upstream whereami ships no build system; and (2) why consumer flags are hand-built rather than via pkg-config (whereami installs no .pc).
| length = wai_getModulePath(NULL, 0, NULL); | ||
| if (length <= 0) { | ||
| free(path); | ||
| return 1; | ||
| } | ||
| free(path); | ||
| return 0; |
There was a problem hiding this comment.
The wai_getModulePath block only queries the length and then discards it — the module path is never retrieved into a buffer and never printed, and path was already fully used above. As written it reads like dead/leftover logic. If the intent is to exercise both APIs, allocate + fill + printf the module path (mirroring the executable-path block); otherwise, dropping this block would make the test's intent clearer.
| 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.
These setenv calls overwrite any existing LD_LIBRARY_PATH/DYLD_LIBRARY_PATH rather than prepending. For the shared-lib test that's usually fine (the injected path is trusted installDir/lib), but if the test binary relies on other shared libs resolved via a pre-set value, this could break/flake. Consider prepending the prior value, e.g. installLib + ":" + os.getenv("LD_LIBRARY_PATH"). Low priority.
1 participant
No description provided.