feat(drmp3): add LLAR formula - #133
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: dr_mp3 recipe
Overall this is a clean, well-documented recipe. The version-normalization logic, header-macro parsing, relocatable .pc generation, and the build/test separation all look correct, and it improves on the sibling cglm recipe by validating the published pkg-config metadata in onTest. The hash→version mappings and their comments were cross-checked and are internally consistent (9497270…=0.6.32, dd762b…=0.6.34, 01d23d…=0.6.38).
A few non-blocking observations are inline below. None block merge.
| c := cmake.new(ctx.SourceDir, filepath.join(ctx.SourceDir, "_build"), installDir) | ||
| c.define "DRMP3_SRC_DIR", ctx.SourceDir | ||
| c.defineBool "BUILD_SHARED_LIBS", shared | ||
| c.defineBool "CMAKE_POSITION_INDEPENDENT_CODE", fPIC |
There was a problem hiding this comment.
fPIC and shared overlap. CMAKE_POSITION_INDEPENDENT_CODE is set from fPIC unconditionally, but when BUILD_SHARED_LIBS=ON the library is always built PIC. So (shared=ON, fPIC=OFF) and (shared=ON, fPIC=ON) produce identical output — an overlapping option combination. Conan's convention is to remove/force fPIC when shared is set. Consider constraining this in filter/defaults or documenting that fPIC is a no-op when shared, to avoid two dimensions selecting the same artifact.
| cflags += " -DDRMP3_DLL" | ||
| } | ||
| if noSimd { | ||
| cflags += " -DDR_MP3_NO_SIMD" |
There was a problem hiding this comment.
Confirm -DDR_MP3_NO_SIMD belongs in consumer Cflags. In upstream dr_mp3.h, DR_MP3_NO_SIMD appears to only affect code inside the DR_MP3_IMPLEMENTATION section — it does not guard any public declarations — whereas DR_MP3_NO_STDIO does gate public APIs (e.g. drmp3_init_file). If that holds, consumers don't need -DDR_MP3_NO_SIMD to match the compiled library, and it could be dropped from the .pc Cflags (and made a PRIVATE define in CMakeLists). Worth verifying against the pinned header before changing. Harmless if left as-is.
| return "v2." + strings.trimPrefix(strings.trimPrefix(version, "wav-"), "0.") | ||
| } | ||
| if strings.hasPrefix(version, "mp3-") { | ||
| return "v3." + strings.trimPrefix(strings.trimPrefix(version, "mp3-"), "0.") |
There was a problem hiding this comment.
trimPrefix(version, "0.") is fragile for a future non-0.x line. This is correct today because every dr_libs tag is 0.x.y. But if upstream ever ships e.g. mp3-1.0.0, trimPrefix("1.0.0", "0.") is a no-op and yields v3.1.0.0, which is invalid semver and would break semver.Compare for the whole set. A guarding comment or a more robust transform would future-proof this. (Applies to the flac/wav/mp3 branches on lines 19/22/25.)
| exec "cc", "-std=c99", "@"+flagsFile, consumer, "-o", binary | ||
| lastErr! | ||
|
|
||
| os.setenv("LD_LIBRARY_PATH", filepath.join(installDir, "lib"))! |
There was a problem hiding this comment.
Minor: os.setenv overwrites rather than prepends. Setting LD_LIBRARY_PATH / DYLD_LIBRARY_PATH to installDir/lib replaces any pre-existing value, which could drop other library directories the shared-library test needs. Prepending to the existing value would be more robust. The value itself is recipe-controlled, so this is robustness, not security.
1 participant
Summary
mackron/dr_libsLLAR Formula from the Conan drmp3 recipedrmp3.pcmetadata and validate an installed C consumerValidation
llar test -v ./mackron/dr_libs@9497270f581f43e6b795ce5d98d8764861fb6a50llar test -v ./mackron/dr_libs@01d23df76776faccee3bc456f685900dcc273b4cllar test -v ./mackron/dr_libs@mp3-0.7.3pkg-config --cflags --libs drmp3lookup