feat(libschrift): add LLAR formula - #143
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: tomolt/libschrift recipe
A clean, well-structured new recipe that closely follows sibling conventions (cglm's defaults/filter/matrix pattern, json-c's CMAKE_INSTALL_LIBDIR "lib" layout, and a separate _llar_consumer test tree that correctly survives cache hits). Verified good: sft_version() matches upstream schrift.h v0.10.1, upstream ships a root LICENSE (ISC), and versions.json matches the sibling format.
The main theme across findings is that consumer link flags are hand-built and duplicated rather than derived from installed package metadata. formula-semantics.md prefers installing a .pc file and driving both metadata and the consumer test through a pkg-config lookup. Since this recipe already injects its own CMakeLists.txt, it is well positioned to configure_file-generate a schrift.pc; doing so would collapse the three flag-related findings below into one fix and remove the drift-prone OS/-lm logic.
Two smaller notes:
CMakeLists.txt:9—WINDOWS_EXPORT_ALL_SYMBOLS ONauto-exports every non-static symbol rather than the curatedsft_*API. Acceptable for a build recipe, but a.def/explicit export list would be tighter.- Comments/docs — unlike its siblings (
cglmlines 35-48,json-clines 14-34), this recipe has no explanatory comments. The single most surprising aspect — injecting aCMakeLists.txtbecause upstream ships only a Makefile with no CMake support — is entirely undocumented, as are theshared/fPICoption and-lmrationale. A short build-contract comment block would match repo convention. (The lowercaselibschrift_llar.goxfilename is correct per current docs; don't "fix" it to match the olderCglm_llar.gox-style siblings.)
Inline findings below. All are non-blocking.
| if osName == "linux" || osName == "freebsd" { | ||
| metadata += " -lm" | ||
| } | ||
| ctx.setMetadata metadata |
There was a problem hiding this comment.
Metadata is hand-built rather than derived from an installed pkg-config file. formula-semantics.md states that for C/C++ library metadata you should install a valid .pc under lib/pkgconfig and set metadata from the full pkg-config lookup, treating hand-built -I/-L/-l fragments as a fallback (see how cglm documents its fallback at lines 74-89). Since this recipe injects its own CMakeLists.txt, it can configure_file-generate a schrift.pc and use the pkg-config helper for metadata — that keeps the consumer flags synchronized with the actual install and removes the duplicated OS/-lm logic. If a hand-built string is kept intentionally (upstream ships no pkg-config), add a one-line comment saying so, as the sibling recipe does.
| osName = osValues[0] | ||
| } | ||
| metadata := "-I" + filepath.join(installDir, "include") + " -L" + filepath.join(installDir, "lib") + " -lschrift" | ||
| if osName == "linux" || osName == "freebsd" { |
There was a problem hiding this comment.
The -lm decision here is gated on osName == "linux" || "freebsd", while CMakeLists.txt:14-15 links libm whenever find_library(LIBM m) succeeds (any platform). These are two independent encodings of "does this platform need libm." For the default static build the consumer-side -lm is the load-bearing one (static archives don't record their deps), so on another Unix where libm exists but isn't in this list, the static library would build but consumers would fail to link. Deriving metadata from pkg-config (see the note at setMetadata) would keep these in sync automatically; otherwise consider aligning the two conditions and adding a comment on the intended platform contract.
| } | ||
| if osName == "linux" || osName == "freebsd" { | ||
| args = append(args, "-lm") | ||
| } |
There was a problem hiding this comment.
onTest reconstructs -I/-L/-lschrift/-lm independently (mirroring the onBuild logic at lines 64-72) rather than consuming the published metadata / installed .pc. Per formula-semantics.md, a test that rebuilds the flags does not validate the published metadata — the test can pass while the advertised metadata is wrong, so a real consumer would still break with green CI. Prefer compiling the consumer with the same flags the Formula publishes (ideally via the pkg-config lookup).
|
|
||
| if slices.contains(target.options["shared"], "ON") { | ||
| os.setenv("LD_LIBRARY_PATH", filepath.join(installDir, "lib"))! | ||
| os.setenv("DYLD_LIBRARY_PATH", filepath.join(installDir, "lib"))! |
There was a problem hiding this comment.
LD_LIBRARY_PATH/DYLD_LIBRARY_PATH are overwritten with only the install dir, dropping any pre-existing value. In this isolated test the risk is low, but if the environment already sets these (e.g. a toolchain lib dir) they'd be clobbered. Prefer prepending: installDir/lib + : + the existing value when one is present.
1 participant
Adds a LLAR Formula for tomolt/libschrift.
Validation: git diff --cached --check passed. Local formula tests and CI were intentionally not run per request.