feat(poshlib): add LLAR formula - #141
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: add poshlib LLAR formula
Thanks for the new formula — it follows the shape of the existing recipes closely, correctly derives the consumer test flags from the install directory (so the test still passes on a cache hit), and constrains option values to ON/OFF. A few points worth addressing, the most important being the filter over-rejection which can silently drop otherwise valid target selections.
Non-blocking review — findings are inline.
| filter => { | ||
| for name, values in target.options { | ||
| if name != "shared" && name != "fPIC" { | ||
| return false | ||
| } | ||
| for value in values { | ||
| if value != "ON" && value != "OFF" { | ||
| return false | ||
| } | ||
| } | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
[P1] filter rejects targets that carry any option beyond shared/fPIC
The filter iterates every key in target.options and returns false for any name that isn't shared or fPIC. If the loader ever surfaces another standard key (e.g. os, arch, build type) in target.options, an otherwise valid selection is silently rejected as "no matching target" rather than failing with a clear cause.
The write-formula semantics guidance is explicit here: "Reject a selection only when the selected upstream revision proves it is unsupported" and "Defaults choose option values; they do not by themselves define every legal value." The sibling recp/cglm recipe follows the safer pattern — it validates only the values of the option(s) it cares about and never enumerates/rejects unknown keys.
Consider validating only the values of shared/fPIC instead of rejecting unrecognized option names.
| if shared { | ||
| 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.
[P2] Shared-build test: overwrites loader env and misses Windows DLL resolution
Two issues in the shared-library test path:
-
os.setenv("LD_LIBRARY_PATH", ...)/DYLD_LIBRARY_PATHoverwrite any existing value rather than prepending, clobbering loader paths the test process may need. PreferinstallDir/lib + ":" + os.getenv("LD_LIBRARY_PATH"). -
On Windows — the one platform special-cased with
-DPOSH_DLL— DLL resolution is governed byPATH, notLD_LIBRARY_PATH/DYLD_LIBRARY_PATH. So for the exact configuration that most needs runtime-path help (shared build on Windows), this block does nothing, andexec binarymay fail to locateposh.dllat run time.
| headerPath := filepath.join(ctx.SourceDir, "posh.h") | ||
| header := string(os.readFile(headerPath)!) | ||
| header = strings.replace(header, "defined _ARM", "defined _ARM || defined __arm64 || defined __arm64__ || defined __aarch64__", 1) | ||
| os.writeFile(headerPath, []byte(header), 0644)! |
There was a problem hiding this comment.
[P2] Header patch is not idempotent on a reused source tree
The replacement anchor "defined _ARM" is a prefix of the replacement text ("defined _ARM || defined __arm64 ..."). This edit mutates the pristine upstream posh.h in place. If onBuild ever runs again against an already-patched source tree (a cache-miss rebuild reusing the tree, e.g. across option combinations), strings.replace(..., 1) re-matches the already-injected defined _ARM and expands it a second time.
A guard makes it safe and skips the write on re-runs, e.g. if !strings.contains(header, "__aarch64__") { ... }. As a bonus, strings.replace silently no-ops if upstream ever changes the anchor, so the arm64 fix would vanish without error — asserting the content actually changed would surface that loudly. Acceptable given the pinned tag, but worth hardening.
| metadata := "-I" + filepath.join(installDir, "include") + " -L" + filepath.join(installDir, "lib") + " -lposh" | ||
| if shared && osName == "windows" { | ||
| metadata = "-DPOSH_DLL " + metadata | ||
| } |
There was a problem hiding this comment.
[P3] Metadata is hand-built rather than derived from an installed interface
metadata reconstructs -I/-L/-lposh (plus -DPOSH_DLL) by hand. The semantics guidance prefers deriving metadata from a valid installed package interface (pkg-config/CMake package files) and cautions against copying a package manager's package_info without comparing it to the actual installed result. poshlib's CMake install here doesn't emit a .pc/config, so hand-built flags may be the only option — but a short comment noting that, and that -DPOSH_DLL mirrors the CCI package_info for Windows+shared, would document the choice the way the cglm/json-c recipes do.
| defaults { | ||
| "shared": "OFF", | ||
| "fPIC": "ON", | ||
| } | ||
|
|
||
| filter => { | ||
| for name, values in target.options { | ||
| if name != "shared" && name != "fPIC" { | ||
| return false | ||
| } | ||
| for value in values { | ||
| if value != "ON" && value != "OFF" { | ||
| return false | ||
| } | ||
| } | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
[P3] Option/default semantics are undocumented vs. sibling recipes
The defaults (shared=OFF, fPIC=ON) and the fPIC-only-when-static decision have no explanatory comments. The exemplar recipes (recp/cglm, json-c) document why each option exists, that defaults mirror the CCI default_options (shared=False, fPIC=True), and why fPIC is meaningless for a shared build. Adding brief notes here — including that the onTest flags are rebuilt from installDir specifically so the test works on a cache hit — would match repo convention and help future maintainers.
1 participant
Adds the LLAR Formula for PhilipLudington/poshlib v1.3.002 from the Conan Center recipe.
Validation: git diff --cached --check.