Skip to content

feat(miniaudio): add LLAR formula - #136

Open
MeteorsLiu wants to merge 1 commit into
xgo-dev:mainfrom
MeteorsLiu:issue/104-miniaudio
Open

feat(miniaudio): add LLAR formula#136
MeteorsLiu wants to merge 1 commit into
xgo-dev:mainfrom
MeteorsLiu:issue/104-miniaudio

Conversation

@MeteorsLiu

Copy link
Copy Markdown
Collaborator

Summary

  • add the mackron/miniaudio formula with CCI and upstream version ordering
  • support header-only, static, and shared builds with relocatable pkg-config metadata
  • add independent C consumer validation and install extras/license files

Test Plan

  • all 19 visible refs pass llar test
  • exact/default/cache-hit selection checks pass
  • git diff --check passes
  • full LLAR go test ./... has one unrelated pre-existing libpng/zlib Apple Clang failure

@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: 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:79extras is copied into include/ in every mode, so in the compiled build both include/miniaudio.h (split) and include/extras/miniaudio_split/{miniaudio.h,miniaudio.c} end up shipped. This mirrors the upstream CCI recipe, but it does put an implementation .c inside the public include path; worth confirming it's intended.
  • versions.json — uses 2-space indentation while the other versions.json files 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).

Comment on lines +22 to +25
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

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.

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

Comment on lines +173 to +174
os.setenv("LD_LIBRARY_PATH", filepath.join(installDir, "lib"))!
os.setenv("DYLD_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

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.

Comment on lines +49 to +53
version := strings.join([]string{
versionParts["MA_VERSION_MAJOR"],
versionParts["MA_VERSION_MINOR"],
versionParts["MA_VERSION_REVISION"],
}, ".")

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] 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

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] 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.

Comment on lines +11 to +12
// 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.

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] 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)!

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] 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.

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