Skip to content

feat(log.c): add LLAR formula - #138

Open
MeteorsLiu wants to merge 2 commits into
xgo-dev:mainfrom
MeteorsLiu:issue/101-log-c
Open

feat(log.c): add LLAR formula#138
MeteorsLiu wants to merge 2 commits into
xgo-dev:mainfrom
MeteorsLiu:issue/101-log-c

Conversation

@MeteorsLiu

Copy link
Copy Markdown
Collaborator

Summary

  • add the rxi/log.c Formula for upstream commit f9ea34994bd58ed342d2245cd4110bb5c6790153
  • translate the Conan CMake build with shared, fPIC, and color options
  • install the public header, library, and license, publish verified consumer flags, and run an independent C consumer test

Evidence

The CCI snapshot ffe30df101afd4dc95aac2f14b25bf345e64d7be contains only cci.20200620; upstream exposes only master at 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)
  • repeated command exercised the cache-hit onTest path (exit 0)
  • git diff --cached --check (clean)

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.txt rewritten unconditionally (onBuild): the file is written on every build, so a warm _build dir will see a changed timestamp and force a full CMake reconfigure. Negligible for one-shot builds; only matters if incremental rebuilds are expected.
  • defaults scalars vs slice consumption: defaults declares "shared": "OFF" (scalars) while filter (for value in values) and onBuild/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 (old onBuild (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"))!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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{

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 -lNAMElibNAME.{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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant