feat(tgc): add LLAR formula - #156
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: add orangeduck/tgc formula
Solid, minimal recipe — the SHA-pinned fromVer, empty deps, license capture, the shared/fPIC matrix with a strict filter, applying CMAKE_POSITION_INDEPENDENT_CODE only for static builds, and the distinct consumer test tree (_llar_consumer) that derives paths from installDir (so cache-hit tests work) all line up with the repo conventions.
One issue is worth fixing before merge, plus a couple of minor notes below.
Main issue — lib vs lib64 install-path mismatch
CMakeLists.txt installs via GNUInstallDirs (${CMAKE_INSTALL_LIBDIR}), which resolves to lib64 on many 64-bit distros (Fedora/RHEL/openSUSE). But the formula hardcodes -L<installDir>/lib in the metadata (line 75), the test link flags (line 94), and the LD_LIBRARY_PATH/DYLD_LIBRARY_PATH (lines 102–103). On a lib64 platform the library installs under lib64 while every consumer path points at lib, so linking/loading fails and the published metadata is wrong. The sibling json-c recipe guards against exactly this: c.define "CMAKE_INSTALL_LIBDIR", "lib" with the comment "Keep a standard lib/ layout … without lib64 handling." Recommend pinning CMAKE_INSTALL_LIBDIR to lib in onBuild (or installing directly to lib/include without GNUInstallDirs). See inline comments.
Minor notes
- Missing explanatory comments. Unlike
cglm/json-c, this recipe has no comments. The non-obvious decisions deserve a line each: why it injects its ownCMakeLists.txt(upstream tgc ships no CMake build), why PIC is set only for static, and thelib/lib64handling once fixed. - Optional — pkg-config metadata.
formula-semantics.mdprefers deriving metadata from an installed.pc. Hand-built flags are acceptable here (upstream ships no.pc, matching the cglm fallback precedent), so this is optional: having the suppliedCMakeLists.txtalso install a relocatabletgc.pcand driving both metadata and the test throughpkgconfig.use/pkgconfig.lookupwould keep metadata and test in sync.
No security, performance, or dependency concerns found.
| include(GNUInstallDirs) | ||
| install(TARGETS tgc | ||
| RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR} | ||
| LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR} |
There was a problem hiding this comment.
GNUInstallDirs makes ${CMAKE_INSTALL_LIBDIR} resolve to lib64 on many 64-bit distros, but the formula hardcodes -L.../lib (see tgc_llar.gox:75, :94, :102-103). On those platforms the archive lands in lib64 and the consumer link/load fails. Pin the layout the way json-c does — c.define "CMAKE_INSTALL_LIBDIR", "lib" in onBuild — or install directly to lib/include here.
|
|
||
| metadata := []string{ | ||
| "-I" + filepath.join(installDir, "include"), | ||
| "-L" + filepath.join(installDir, "lib"), |
There was a problem hiding this comment.
This -L.../lib (and the same at line 94) assumes a fixed lib layout, but the CMakeLists installs via GNUInstallDirs, which can produce lib64. Either pin CMAKE_INSTALL_LIBDIR=lib in this onBuild before configuring, or derive the -L path from the actual install layout, so metadata and the consumer stay correct on lib64 platforms.
|
|
||
| if shared { | ||
| 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.
These overwrite any existing LD_LIBRARY_PATH/DYLD_LIBRARY_PATH rather than prepending. Benign in an isolated CI runner, but if the toolchain relies on a pre-existing value the shared-build consumer run could break — prefer prepending installDir/lib to the current value. (Also affected by the lib/lib64 issue above.)
|
|
||
| static void example_function() { | ||
| char *message = tgc_alloc(&gc, 64); | ||
| strcpy(message, "No More Memory Leaks!"); |
There was a problem hiding this comment.
Minor: this example uses strcpy but includes only <tgc.h>. It compiles today because tgc.h transitively includes <string.h>, so this is not a bug — but relying on a transitive include is fragile under stricter defaults. Adding #include <string.h> to consumerSource makes the example self-contained.
1 participant
Closes #83
Adds the
orangeduck/tgcFormula from the Conan Center snapshotffe30df101afd4dc95aac2f14b25bf345e64d7be.e193133e43af0862f51edfb9b518e212ea084412as the exact source version.