feat(miniaudio): add LLAR formula - #136
Conversation
There was a problem hiding this comment.
Review: feat(miniaudio): add LLAR formula
Solid, well-structured recipe that handles a genuinely tricky library (single-header vs. compiled split-source) and mostly follows the conventions of the existing cglm/json-c/zlib recipes. Deriving the version from the header, the cross-target os override, and explicit license installation are all nice touches. A few correctness/robustness points are worth addressing before merge — see inline comments.
Minor notes (no inline):
miniaudio_llar.gox:79—extrasis copied intoinclude/in every mode, so in the compiled build bothinclude/miniaudio.h(split) andinclude/extras/miniaudio_split/{miniaudio.h,miniaudio.c}end up shipped. This mirrors the upstream CCI recipe, but it does put an implementation.cinside the public include path; worth confirming it's intended.versions.json— uses 2-space indentation while the otherversions.jsonfiles in the repo (recp/cglm,madler/zlib) use tabs. Cosmetic only.
Verified as correct (no action): all CCI commit-ref → version mappings in miniaudio_cmp.gox match upstream/CCI, and the mode-agnostic test consumer works for both modes (the split header ignores MINIAUDIO_IMPLEMENTATION, so the compiled build still links -lminiaudio).
| case "0.11.15", "0.11.16", "0.11.17", "0.11.18", "0.11.19", "0.11.20", "0.11.21", "0.11.22", "0.11.23", "0.11.24", "0.11.25": | ||
| return "v" + version | ||
| } | ||
| return version |
There was a problem hiding this comment.
[P1] normalize() mis-orders any version not in the hard-coded list
Versions not matched by the switch fall through to return version unchanged (line 25). For a bare numeric tag outside the enumerated 0.11.15..0.11.25 set (e.g. a future 0.11.26) the value has no v prefix, so semver.Compare treats it as invalid and orders it below every normalized version — silently mis-selecting "latest". The recp/cglm/Cglm_cmp.gox comparator avoids this by normalizing via a prefix rule instead of an exhaustive enumeration. Consider a rule-based normalize (add v to bare numeric tags, map only the known untagged commit refs) so new upstream releases sort correctly without editing this file. At minimum, document that new versions must be appended here or they will sort incorrectly.
| 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] LD_LIBRARY_PATH/DYLD_LIBRARY_PATH overwritten instead of prepended
Both setenv calls replace any inherited value rather than prepending the install lib dir to it. If the test binary or toolchain relies on paths already in LD_LIBRARY_PATH (e.g. sibling formula outputs or sandbox runtime libs), they are dropped for the test and it may fail or resolve the wrong library. Prepend to the existing value, e.g. libDir + os.pathListSeparator + os.getenv("LD_LIBRARY_PATH"). These are also only needed for the shared build; note DYLD_LIBRARY_PATH is frequently stripped on macOS under SIP, so the shared-lib test may be fragile there regardless.
| version := strings.join([]string{ | ||
| versionParts["MA_VERSION_MAJOR"], | ||
| versionParts["MA_VERSION_MINOR"], | ||
| versionParts["MA_VERSION_REVISION"], | ||
| }, ".") |
There was a problem hiding this comment.
[P2] Version parts are not validated before use
version is assembled from MA_VERSION_MAJOR/MINOR/REVISION with no guard that all three were found. If any #define is absent (e.g. a future header layout change), the corresponding map lookup returns "" and version becomes something like 0..39 or ... That malformed string is then written into the CMake VERSION property (line 71 -> set_target_properties, which will error) and into the .pc Version: field (line 116). Since the header is untrusted build input, validate that each part is non-empty and numeric before assembling, and fail fast otherwise. (Minor: the parse loop also scans the entire multi-MB header; a break once all three are found avoids needless work.)
| onBuild ctx => { | ||
| installDir := ctx.outputDir | ||
| headerOnly := slices.contains(target.options["header_only"], "ON") | ||
| shared := slices.contains(target.options["shared"], "ON") && !headerOnly |
There was a problem hiding this comment.
[P2] header_only=ON + shared=ON collapses to header-only silently
shared is forced false whenever headerOnly is true, but the filter (lines 21-33) accepts any ON/OFF combination. So a target requesting header_only=ON, shared=ON passes the filter as a distinct, cacheable configuration yet produces the exact same header-only artifact as header_only=ON, shared=OFF — a consumer that asked for a shared library gets a header-only interface with no diagnostic. Either reject the contradictory combination in filter, or normalize the option set so the two do not present as separate variants.
| // The CCI recipe serves every listed release with the same split-source | ||
| // interface. The oldest ref is a commit because upstream did not tag 0.10.39. |
There was a problem hiding this comment.
[P3] Comment overstates split-source as the universal interface
"serves every listed release with the same split-source interface" is inaccurate for the default: with header_only=ON (the default) the served header is the single-file top-level miniaudio.h copied on line 63, not the split source. Split source is only used in the compiled (!headerOnly) path. Similarly, the miniaudio_cmp.gox header comment claims it maps "every current upstream tag", but the string branch is a fixed list capped at 0.11.25 (see the P1 note). Recommend clarifying both comments.
| lastErr! | ||
| } else { | ||
| cmakeLists := ctx.Proj.readFile("8bf157f10e278302f8a6c1c9cd1065f2bea26dd2/CMakeLists.txt")! | ||
| os.writeFile(filepath.join(ctx.SourceDir, "CMakeLists.txt"), cmakeLists, 0o644)! |
There was a problem hiding this comment.
[P3] onBuild writes CMakeLists.txt into ctx.SourceDir
The compiled path writes a generated CMakeLists.txt directly into the (potentially shared/cached) source tree. The cglm/json-c recipes keep all generated build inputs in a dedicated scratch dir and never write project files back into SourceDir. Since one fromVer source tree serves many option/os variants, writing into it makes builds order-dependent and can contaminate reuse across variants. Consider writing the generated CMakeLists into the _build scratch dir instead.
Summary
Test Plan
llar testgit diff --checkpassesgo test ./...has one unrelated pre-existing libpng/zlib Apple Clang failure