feat(log.c): add LLAR formula - #138
Conversation
There was a problem hiding this comment.
Review summary
Reviewed the new rxi/log.c formula plus the madler/zlib migration to the new formula API (onBuild ctx =>, ctx.outputDir property, ctx.setMetadata, panic-style !). The migration is clean and internally consistent, and versions.json (empty deps, no onRequire) is correct for both.
A few things worth addressing in the new log.c formula are noted inline. Additional non-blocking notes:
- Generated
CMakeLists.txtrewritten unconditionally (onBuild): the file is written on every build, so a warm_builddir will see a changed timestamp and force a full CMake reconfigure. Negligible for one-shot builds; only matters if incremental rebuilds are expected. defaultsscalars vs slice consumption:defaultsdeclares"shared": "OFF"(scalars) whilefilter(for value in values) andonBuild/onTest(slices.contains(target.options["shared"], "ON")) treat each option as a[]string. This is only correct if the matrix/options model promotes scalar defaults into single-element slices — worth a maintainer confirming, since a missing option silently degrades to the "OFF" path rather than the declared default.- Docs drift (out of scope to fix here): this new API diverges from every example in
.claude/skills/write-formula/SKILL.md(oldonBuild (ctx, proj, out),out.addErr/out.setMetadata,ctx.outputDir()returning an error). Undocumented constructs used here (defaults,filter,onTest,target.options) mean an author following SKILL.md would generate stale-style formulas. Consider a follow-up to update SKILL.md.
| lastErr! | ||
|
|
||
| if shared { | ||
| os.setenv("LD_LIBRARY_PATH", filepath.join(installDir, "lib"))! |
There was a problem hiding this comment.
[P2] LD_LIBRARY_PATH/DYLD_LIBRARY_PATH overwritten instead of prepended
os.setenv("LD_LIBRARY_PATH", ...) and the DYLD_ line replace any existing value rather than prepending. log.c has no runtime deps so this works today, but the pattern is fragile if copied to a formula whose shared test binary also needs a dependency's lib dir on the search path — that dir would be dropped and the test would fail to load its libs. Consider prepending, e.g. filepath.join(installDir, "lib") + ":" + os.getenv("LD_LIBRARY_PATH"). As the first formula in the repo to touch these vars, it sets the convention.
|
|
||
| int main(void) { | ||
| log_trace("Hello %s", "world"); | ||
| log_trace(log_level_string(0)); |
There was a problem hiding this comment.
[P3] Non-literal format string in generated consumer test
log_trace(...) forwards its first argument as the printf-style format to log_log(..., const char *fmt, ...). Passing the runtime return of log_level_string(0) as the format string is a format-string anti-pattern. It's benign in practice (log_level_string(0) returns "TRACE", no %), but it can trigger -Wformat-security/-Werror and would break if the argument ever contained %. Prefer log_trace("%s", log_level_string(0));.
| 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.
[P3] Duplicated include/lib/link flag construction between onBuild and onTest
The -I include, -L lib, and -llog.c flags are built independently here (published metadata) and again in onTest. If the library name or output layout ever changes, the two sites can silently drift — the self-test could keep passing while consumers relying on the metadata break, or vice versa. Consider deriving the onTest flags from a shared helper or from the metadata itself.
| flags := []string{ | ||
| "-I" + filepath.join(installDir, "include"), | ||
| "-L" + filepath.join(installDir, "lib"), | ||
| "-llog.c", |
There was a problem hiding this comment.
[P3] CMake target named log.c (embedded dot) is fragile
Naming the target log.c yields liblog.c.a/liblog.c.so and the -llog.c link flag. This is technically correct (the linker maps -lNAME → libNAME.{a,so}), but a base name containing a .c suffix is unusual and some tooling (pkg-config generation, Windows import-lib naming) may not handle the embedded dot gracefully. Upstream ships only drop-in src/log.c/src/log.h with no canonical library name, so log + -llog would be more conventional and robust. If log.c is deliberate, a short comment would help.
Summary
rxi/log.cFormula for upstream commitf9ea34994bd58ed342d2245cd4110bb5c6790153shared,fPIC, andcoloroptionsEvidence
The CCI snapshot
ffe30df101afd4dc95aac2f14b25bf345e64d7becontains onlycci.20200620; upstream exposes onlymasterat the selected commit and has no tags or other heads, so no comparator is added.Validation
/tmp/llar-main-bin test -v ./@f9ea34994bd58ed342d2245cd4110bb5c6790153 --os darwin --arch arm64(exit 0)onTestpath (exit 0)git diff --cached --check(clean)