Add recp/cglm LLAR Formula - #116
Conversation
Translate the Conan Center cglm recipe into an idiomatic LLAR Formula serving recp/cglm from v0.8.5. The upstream CMake build contract (options CGLM_SHARED/CGLM_STATIC/CGLM_USE_TEST, GNUInstallDirs layout, cglm.pc) is consistent across v0.8.5..v0.9.6, so one Formula covers the range. - No dependencies (cglm has none); versions.json deps empty. - Options shared (default OFF, matching Conan) and header_only (default OFF), both of which change the installed output. filter rejects the unsupported shared=ON+header_only=ON combination. - Library build uses the CMake helper; header-only installs the public headers directly, mirroring the Conan recipe. - Metadata comes from the installed cglm.pc via pkg-config. - onTest compiles and runs a consumer modeled on the Conan test_package, deriving flags from the installed output so it also passes on cache hits. Co-authored-by: fennoai[bot] <231223108+fennoai[bot]@users.noreply.github.com> Co-authored-by: MeteorsLiu <17515813+MeteorsLiu@users.noreply.github.com>
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: recp/cglm LLAR Formula
Solid, well-structured Formula. It follows house conventions correctly: imports and receiverless helpers precede id; CMake configure/build/install are called without result checks; the single-context defaults/filter/onBuild/onTest API is used correctly; capout validates lastErr before reading output; the cache-hit test path deliberately reads the install dir instead of build metadata; and versions.json (path/deps) matches id. Security and performance passes found nothing to raise.
One substantive finding worth addressing before merge, plus a few minor ones (inline).
Metadata is missing -lm (verified against upstream v0.8.5). onBuild derives the consumer metadata purely from pkg-config --cflags --libs cglm, and the comment claims this yields -lm on Linux/FreeBSD. But upstream cglm.pc.in is Libs: -L${libdir} -lcglm @LIBS@, and @LIBS@ is never assigned in CMakeLists.txt (configure_file(... @ONLY) leaves it empty). So the shipped metadata is -L... -lcglm with no -lm — while cglm headers call sqrtf (include/cglm/mat3.h), which needs libm on Linux where it is a separate library. onTest hardcodes -lm, so the test links successfully with flags the published metadata never provides and cannot catch the gap. Recommend appending -lm to the onBuild metadata explicitly (or confirming the target platforms bundle libm) and correcting the comment, so the shipped metadata and onTest agree.
| // Derive consumer flags from the installed pkg-config file so metadata | ||
| // stays synchronized with the actual build (e.g. -lm on Linux/FreeBSD). | ||
| c.use installDir | ||
| capout => { | ||
| exec "pkg-config", "--cflags", "--libs", "cglm" | ||
| } | ||
| if lastErr != nil { | ||
| panic lastErr | ||
| } | ||
| ctx.setMetadata strings.trimSpace(output) |
There was a problem hiding this comment.
Verified against upstream v0.8.5: this metadata will not contain -lm, contradicting the comment on line 132.
cglm.pc.in is Libs: -L${libdir} -lcglm @LIBS@, and @LIBS@ is never set anywhere in CMakeLists.txt (the configure_file(... @ONLY) at line 156 leaves it empty). So pkg-config --cflags --libs cglm returns -I... -L... -lcglm only. Since cglm headers use sqrtf (include/cglm/mat3.h), a consumer that pulls those symbols will fail to link on Linux (libm is separate) using the shipped metadata.
Suggest appending -lm explicitly after the pkg-config output, e.g. ctx.setMetadata strings.trimSpace(output) + " -lm", and fixing the comment to reflect that pkg-config alone does not supply it.
| // header-only install has just headers). | ||
| flags := []string{"-I" + filepath.join(installDir, "include")} | ||
| if hasLibrary(installDir) { | ||
| flags = append(flags, "-L"+filepath.join(installDir, "lib"), "-lcglm", "-lm") |
There was a problem hiding this comment.
onTest hardcodes -lm here, but the onBuild metadata (line 140) is derived solely from pkg-config, which does not include -lm (see comment on the pkg-config block). The test therefore links with flags a real consumer would not receive from the published metadata, so it cannot detect the missing -lm. Once the metadata is fixed to include -lm, this stays consistent; until then the two link recipes diverge.
| return false | ||
| } | ||
| for _, entry := range entries { | ||
| if strings.contains(entry.name(), "cglm") { |
There was a problem hiding this comment.
The unanchored substring match strings.contains(entry.name(), "cglm") is fragile: it also matches non-library artifacts (e.g. a cglm.pc or cglm-config if the lib layout ever changes) and assumes the library file always contains the literal cglm. Since the header-only vs. library distinction is already owned by the formula via target.options["header_only"], consider gating the link-flag decision in onTest on slices.contains(target.options["header_only"], "ON") (exact and cache-independent) rather than scanning lib/. If a filesystem probe is still wanted, match a libcglm prefix plus a known extension instead of an unanchored substring.
| // - shared: build the shared library (ON) instead of the static one (OFF). | ||
| // - header_only: install headers only and build nothing. | ||
| // The upstream CMake option CGLM_USE_TEST is always disabled; it builds the | ||
| // upstream test suite, not the installed interface. |
There was a problem hiding this comment.
Minor doc precision: "CGLM_USE_TEST is always disabled" is only true on the CMake path (line 126). In header-only mode CMake is never invoked (lines 111-119 return early), so the option is not passed at all rather than disabled. The net effect (test suite never built) holds; consider wording like "disabled whenever CMake runs; the header-only path skips CMake entirely."
Closes #61.
Translates the Conan Center
cglmrecipe into an idiomatic LLAR Formula atrecp/cglm, serving upstream tags fromv0.8.5.What it does
recp/cglm/versions.json—path: recp/cglm, emptydeps(cglm has no dependencies).recp/cglm/v0.8.5/Cglm_llar.gox— one Formula fromfromVer "v0.8.5". The upstream CMake build contract (CGLM_SHARED/CGLM_STATIC/CGLM_USE_TEST, GNUInstallDirs layout,cglm.pc, headers underinclude/cglm) is verified consistent acrossv0.8.5..v0.9.6, so a single Formula covers the Conan-served range.Translation decisions
shared(defaultOFF, matching the Conan recipe's static default) →CGLM_SHARED/CGLM_STATIC.header_only(defaultOFF) → installs only the public headers, matching Conan'sheader_onlypackaging.fPICis dropped: it does not change the installed interface and PIC handling is left to the toolchain.filterrejects only the genuinely unsupported combinationshared=ON+header_only=ON(a header-only package installs no library).cglm.pcviapkg-config.onTestcompiles and runs a consumer modeled on the Conantest_package.c(glm_mat4_det+glm_mat4_mulv), deriving flags from the installed output (not build-result metadata) so it also passes on cache hits.Validation
llar test -v(installed fromgoplus/llar@main, as CI does) onlinux/amd64:v0.9.6default (static) — fresh + cache-hitv0.9.6--option shared=ONv0.9.6--option header_only=ON— fresh + cache-hitv0.9.1(mid-range) defaultv0.8.5(fromVerboundary) defaultshared=ON+header_only=ONfilterEach passing consumer printed the expected
det=1.000000andresult=1.000000 2.000000 3.000000 1.000000.