feat(libde265): add LLAR formula - #135
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: feat(libde265): add LLAR formula
Nice, well-structured addition. The formulas follow the repo's per-version model, capture the license into licenses/COPYING, use a real compile-and-run consumer test that works on cache hits, and handle LD_LIBRARY_PATH/DYLD_LIBRARY_PATH cross-platform for shared tests. The upstream option mappings (shared/fPIC/sse/ENABLE_SDL) were verified accurate against both tags' CMakeLists.txt, and the $${pcfiledir} doubled-$ is the correct Go+ escape for emitting a literal ${pcfiledir}.
Findings below are non-blocking. The two version files share most logic, so any fix to the pkg-config or filter blocks should be applied to both v1.0.12 and v1.0.17.
No performance or security concerns of note.
| @@ -0,0 +1,4 @@ | |||
| { | |||
| "path": "strukturag/libde265", | |||
There was a problem hiding this comment.
Indentation inconsistent with repo convention. All existing versions.json files (madler/zlib, recp/cglm, json-c/json-c) use tab indentation; this file uses 2 spaces. Reformat with tabs to match.
| // for the static C consumer used by this Formula. | ||
| pcPath := filepath.join(installDir, "lib", "pkgconfig", "libde265.pc") | ||
| pc := string(os.readFile(pcPath)!) | ||
| pc = strings.replace(pc, "prefix="+installDir, "prefix=$${pcfiledir}/../..", 1) |
There was a problem hiding this comment.
Fragile prefix rewrite may silently leave the absolute build path embedded. strings.replace(pc, "prefix="+installDir, ...) requires the generated prefix= line to be a byte-for-byte match. If CMake normalizes the path (trailing slash, symlink/case resolution) or emits a ${prefix}-style line, the replace is a no-op and the published .pc keeps the absolute install path — defeating the stated relocatability and leaking the build-host path. Consider anchoring on the line starting with prefix= and rewriting the whole line, then asserting the replacement actually occurred. Same on v1.0.17 line 53.
| } | ||
| if !shared { | ||
| for i, line in lines { | ||
| if privateLibs != "" && line.hasPrefix("Libs:") { |
There was a problem hiding this comment.
Folding Libs.private/Cflags.private into the public Libs:/Cflags: makes the published .pc non-standard. Private deps should stay private; this permanently mutates the installed metadata for all consumers. The json-c formula solves the same static-linking need cleanly by letting pkg-config do it (pkg-config --static). If pkgconfig.lookup supports a static query, prefer that and leave the .pc unmodified. Note also that upstream libde265.pc.in has no Cflags.private field, so the Cflags.private branch is effectively dead code here. Same on v1.0.17 line 67.
| shared := target.options["shared"][0] == "ON" | ||
| fPIC := target.options["fPIC"][0] == "ON" | ||
|
|
||
| cmakeLists := filepath.join(ctx.SourceDir, "CMakeLists.txt") |
There was a problem hiding this comment.
Missing rationale comments that v1.0.12 carries for identical logic. v1.0.12 documents why the CMAKE_POSITION_INDEPENDENT_CODE line is stripped from the upstream CMakeLists.txt and why the pkg-config file is rewritten; v1.0.17 performs the same non-obvious mutations with no comments. Since both tags' CMakeLists.txt contain that set(...) line (verified) and the strings.replace(..., 1) silently no-ops if it ever disappears, the rationale is equally load-bearing here. Replicate the comments to match v1.0.12 and the repo's documentation standard (see cglm/json-c).
| exec "cc", "-std=c11", "@"+flagsFile, consumer, "-o", binary | ||
| lastErr! | ||
| if target.options["shared"][0] == "ON" { | ||
| os.setenv("LD_LIBRARY_PATH", filepath.join(installDir, "lib"))! |
There was a problem hiding this comment.
os.setenv overwrites LD_LIBRARY_PATH/DYLD_LIBRARY_PATH instead of prepending. This discards any existing loader search paths the test environment relied on. Prefer prepending installDir/lib and preserving the prior value. Same on v1.0.17 lines 107-108.
| "sse": "ON", | ||
| } | ||
|
|
||
| filter => { |
There was a problem hiding this comment.
filter accepts any option key, not just the known set. Unlike cglm (which validates the specific shared key), this loops over all of target.options and only checks values are ON/OFF. A typo like shred/Sse would pass the filter, then target.options["shared"][0] etc. proceed with defaults while the intended option is silently ignored. Consider validating against the known keys (shared, fPIC, sse). Same on v1.0.17 line 16.
1 participant
Summary
Verification
Closes #103