feat(tiny-bignum-c): add LLAR formula - #154
Conversation
There was a problem hiding this comment.
Review: kokke/tiny-bignum-c recipe
Nice, focused addition. The recipe closely mirrors the cglm analog and gets the important cache-hit rule right: onTest derives consumer flags from installDir and reads shared from target.options, so it works even when onBuild is skipped on a cache hit. Verified against the pinned upstream commit cb8b539: LICENSE, bn.c, and bn.h all exist, the -ltiny-bignum-c link name matches the CMake target, and struct bn is fixed at 128 bytes so the consumer's char buf[8192] cannot overflow.
Findings below are non-blocking suggestions and consistency notes — nothing critical.
Suggestions (see inline):
onTestasserts only the exit status, not the computed result —factorial(100)silently overflows the fixed 1024-bit width, so a wrong value still exits 0.- Metadata uses handwritten
-I/-L/-lflags; since the recipe already synthesizes its ownCMakeLists.txt, installing a relocatable.pcand querying pkg-config would match the repo's preferred convention (permitted fallback either way). os.setenvfor the loader path is process-wide and clobbers any existing value.filterrejects targets carrying any unrecognized option name (stricter than thecglmanalog).- Missing the build-contract comments every sibling recipe carries.
| exec binary | ||
| lastErr! |
There was a problem hiding this comment.
[P2] onTest checks exit status only, not the computed result
The consumer runs factorial(100) and prints it, but the test only checks lastErr! after exec binary — any exit-0 passes. factorial(100) far exceeds the fixed 1024-bit struct bn width, so bignum_mul silently truncates and prints a wrong value while still exiting 0. The repo's formula-semantics.md ("Validation Range") states: "Inspect the installed files and consumer result; parsing and build completion alone are not sufficient." Consider capturing output (capout) and asserting an expected value to verify correctness, not just that the binary links and runs.
| lastErr! | ||
|
|
||
| if shared { | ||
| os.setenv("LD_LIBRARY_PATH", filepath.join(installDir, "lib"))! |
There was a problem hiding this comment.
[P2] os.setenv clobbers loader path process-wide
os.setenv("LD_LIBRARY_PATH", ...) / DYLD_LIBRARY_PATH overwrite (not prepend) any existing value and mutate the whole formula process rather than scoping the variable to the exec binary child. If the runner reuses the process for other recipes, the leaked/clobbered loader path could affect them. Prefer scoping the env to the child command via the gsh command-environment mechanism, and preserve any prior value.
| os.mkdirAll(licenseDir, 0o755)! | ||
| os.writeFile(filepath.join(licenseDir, "LICENSE"), os.readFile(filepath.join(ctx.SourceDir, "LICENSE"))!, 0o644)! | ||
|
|
||
| flags := []string{ |
There was a problem hiding this comment.
[P2] Prefer installing a relocatable .pc over handwritten metadata flags
Metadata is built from handwritten -I/-L/-ltiny-bignum-c flags. The handwritten fallback is explicitly permitted by formula-semantics.md when there's no installed package metadata, so this is not a blocker. However, since this recipe already synthesizes its own CMakeLists.txt, it could have that CMake install a relocatable .pc (prefix derived from ${pcfiledir}) and set metadata from a full pkg-config --cflags --libs lookup — the repo's preferred convention, matching cglm/json-c. That would also let the consumer test validate the published metadata (per the "Consumer Test" section) instead of independently reconstructing the same flags.
| } | ||
|
|
||
| filter => { | ||
| for name, values in target.options { |
There was a problem hiding this comment.
[P3] filter rejects any unrecognized option name (stricter than cglm)
This filter returns false when target.options contains any name other than shared/fPIC. The cglm analog only validates the values of the option it owns (shared) and is permissive toward other keys. formula-semantics.md ("Matrix And Options") suggests rejecting "only when the selected upstream revision proves it is unsupported." Rejecting on the mere presence of an unknown key is stricter than that guidance and the analog — confirm this is intended, or narrow the check to validate only the option values you own.
| return true | ||
| } | ||
|
|
||
| onBuild ctx => { |
There was a problem hiding this comment.
[P3] Missing build-contract comments (convention gap)
Both sibling recipes (cglm, json-c) carry explanatory comments describing the build contract, option meanings, and why onTest derives flags from the install dir for cache-hit safety. This recipe has essentially none. The most useful additions: note that upstream ships no build system (hence the synthesized CMakeLists.txt), the meaning of the shared/fPIC options, and the cache-hit rationale for reconstructing flags in onTest.
Summary
kokke/tiny-bignum-cFormula at upstream commitcb8b53925ae79e5fb8f6a2f21b371fb2c3c8e7ecshared/fPICoptions, license install, andbn.hconsumer checkValidation
git diff --checkllar testnot run per request; CI should be cancelled after PR creation