feat(libccd): add LLAR formula - #179
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: danfis/libccd v2.1 recipe
Solid new recipe that closely follows the established repo conventions (compare strukturag/libde265, ebiggers/libdeflate, tidwall/neco): relocatable pkgconfig rewrite, license copy, static-linking flag augmentation, and a consumer test all mirror existing patterns. versions.json matches the peer schema, and the filter correctly whitelists option names (an improvement over the looser libde265 filter).
A few consistency/robustness notes are inline below — none are blocking. The most impactful are the test consumer not forcing an actual library link, and [0] indexing that contradicts the recipe's own multi-value filter contract.
No security concerns found. LD_LIBRARY_PATH/DYLD_LIBRARY_PATH are overwritten rather than prepended, but that is confined to the shared-build test phase and is byte-for-byte the same as the reference recipe — not a regression.
|
|
||
| int main(void) { | ||
| ccd_t ccd; | ||
| CCD_INIT(&ccd); |
There was a problem hiding this comment.
The consumer test only uses CCD_INIT, which is a header-only macro (field initialization). It compiles and links even if the libccd library itself fails to link, so it doesn't validate the Libs:/-lm/CCD_STATIC_DEFINE flags this recipe adds in onBuild. Peer consumers call a real exported function to force a link (e.g. de265_new_decoder in libde265, crc32c::Crc32c in crc32c). Consider calling an actual exported symbol such as ccdVec3Dist2 so the test exercises the link.
|
|
||
| onBuild ctx => { | ||
| installDir := ctx.outputDir | ||
| shared := target.options["shared"][0] == "ON" |
There was a problem hiding this comment.
The filter (lines 27-39) iterates for value in values, so it accepts targets that carry multiple values for an option (e.g. shared=[ON, OFF]), rejecting only unknown names/values. But onBuild/onTest read only [0] and silently ignore the rest, so a passing multi-value target would build one arbitrary variant. Peer recipes with this same filter shape use membership instead of position: slices.contains(target.options["shared"], "ON") (see libdeflate, neco). slices is already imported. Either switch to slices.contains or tighten the filter to require single-valued options. Same [0] pattern recurs at line 103.
| if line.hasPrefix("Cflags:") && !strings.contains(line, "CCD_STATIC_DEFINE") { | ||
| lines[i] = line + " -DCCD_STATIC_DEFINE" | ||
| } | ||
| if slices.contains(target.require["os"], "linux") && line.hasPrefix("Libs:") && !strings.contains(line, " -lm") { |
There was a problem hiding this comment.
-lm is appended only when require["os"] contains linux, but libccd's static build needs libm on other non-Windows platforms too (e.g. freebsd, which this repo targets in neco). If macOS/BSD targets are in scope, the static consumer link could fail. If libccd is intentionally linux-only for now, consider gating with !windows (as microtar does) or documenting the platform restriction so the assumption is explicit.
| os.writeFile(filepath.join(licenseDir, "BSD-LICENSE"), os.readFile(filepath.join(ctx.SourceDir, "BSD-LICENSE"))!, 0o644)! | ||
|
|
||
| // Upstream installs ccd.pc with CMAKE_INSTALL_PREFIX-expanded paths. | ||
| // Keep its verified flags while making the installed metadata relocatable. |
There was a problem hiding this comment.
This comment describes only the prefix relocation, but the code below also augments upstream flags for static builds — appending -DCCD_STATIC_DEFINE to Cflags: (74-75) and -lm to Libs: (77-78). "Keep its verified flags" understates that. Relatedly, CCD_HIDE_ALL_SYMBOLS = !shared (line 54) is uncommented despite being coupled to the -DCCD_STATIC_DEFINE re-add (upstream injects it at build time only, so a static consumer must define it too, or hit visibility/dllimport mismatches). The sibling libde265 recipe comments its non-obvious CMake tweaks; a brief note on why -DCCD_STATIC_DEFINE/-lm are added for static builds would help future maintainers.
1 participant
Translate Conan Center libccd to LLAR.
Closes #64