feat(huffman): add LLAR formula - #148
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: drichardson/huffman recipe
A clean, well-structured Formula. It correctly follows the repo's LLAR conventions: the lowercase huffman_llar.gox filename stem, the expr! / lastErr! error idiom, the option filter, license installation, and Windows-aware linking (-lws2_32) all match the documented style. Cache-hit safety is handled well — onTest builds in a separate _llar_consumer tree and reads only from installDir, so it survives a skipped onBuild. I also confirmed against upstream that v1.2.3 is indeed the first tag with the CMake build, the single #ifdef WIN32 occurrence makes the strings.replace(..., 1) correct, and upstream ships no .pc file, so hand-building -lhuffman metadata is the right fallback (consistent with the cglm v0.7.2 precedent).
Only minor, non-blocking nits below.
| if slices.contains(target.require["os"], "windows") { | ||
| args = append(args, "-lws2_32") |
There was a problem hiding this comment.
[P3] onTest re-derives the Windows link flag instead of reusing build metadata
onBuild already computes the full link line (-lhuffman plus -lws2_32 on Windows) into ctx.setMetadata, and onTest re-derives the -lws2_32 decision from target.require["os"] again here. Re-deriving flags from installDir for cache-hit safety is the established pattern (cglm/json-c do the same), so this is acceptable — but the Windows link contract now lives in two places. If the metadata link line grows a new platform lib, the consumer test could silently link differently from what consumers actually get. Consider centralizing the platform link flags so both paths stay in sync.
| } | ||
|
|
||
| free(encoded); | ||
| return encoded_len == 0; |
There was a problem hiding this comment.
[P3] Non-obvious consumer assertion could use a one-line comment
return encoded_len == 0; makes the consumer exit non-zero when the encoder produced no output — a deliberate assertion that encoding yielded bytes. It reads like a bug at a glance. A short comment (e.g. // non-zero exit if encoding produced nothing) would make the intent clear to future maintainers.
|
|
||
| metadata := "-lhuffman" | ||
| if slices.contains(target.require["os"], "windows") { | ||
| metadata += " -lws2_32" |
There was a problem hiding this comment.
[P3] Document why -lws2_32 is appended on Windows
The recipe documents the Cygwin source patch nicely but doesn't note why -lws2_32 is needed (huffman.c pulls in <winsock2.h> under the Windows branch, and upstream's CMake links ws2_32). The cglm recipe documents its -lm metadata rationale inline; a one-line note here would match that depth and explain the platform dependency.
1 participant
Closes #90.
Adds the
drichardson/huffmanFormula for upstreamv1.2.3throughv1.2.7, using the upstream CMake install contract. The Formula preserves Conan Center's shared/fPIC defaults, Cygwin source fix, Windowsws2_32linkage, installed Unlicense, and an independent consumer test.The Conan Center snapshot and upstream tag/source refs were checked before implementation. Local tests and CI are intentionally not awaited per task instructions.