feat(cgltf): add LLAR formula - #140
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: jkuhlmann/cgltf v1.5 formula
Solid, well-structured formula that follows the repo's package layout (versions.json, per-version CMakeLists.txt + *_llar.gox). The synthetic CMake build for a header-only library, license installation, and the transform-based consumer test are all reasonable. versions.json matches convention. A few points worth addressing before merge, noted inline.
Summary of findings
- Unguarded
strings.indexslicing in the header-stripping loop — panics if a marker is ever absent or reordered (robustness; flagged by all reviewers). - User-facing "removed by conan" text baked into shipped headers — inaccurate for an LLAR package.
cgltf_writeis built and installed but never exercised by the consumer test.- No explanatory comments, unlike the reference
recp/cglmformula, for several non-obvious steps (synthetic build, header stripping, hand-built metadata).
Optional/for discussion: metadata is exposed as hand-built -I/-L/-lcgltf flags and re-derived independently in onTest. That mirrors recp/cglm's fallback path and is fine given cgltf installs no .pc; consider a one-line comment recording that this is a deliberate fallback.
Non-blocking review.
| headerContent := string(os.readFile(headerPath)!) | ||
| begin := strings.index(headerContent, markerBegin) | ||
| end := strings.index(headerContent, markerEnd) | ||
| implementation := headerContent[begin:end] |
There was a problem hiding this comment.
Guard the marker lookups before slicing. strings.index returns -1 when a marker is absent, so headerContent[begin:end] will panic (negative low bound) or slice incorrectly if either marker is missing — or if begin > end. For v1.5 both markers exist and are correctly ordered, so this works today, but since the whole point of this step is silent header rewriting, any future upstream reformat (or extending fromVer) would fail confusingly instead of cleanly. Suggest checking begin >= 0 && end > begin and failing with a clear error before slicing.
begin := strings.index(headerContent, markerBegin)
end := strings.index(headerContent, markerEnd)
implementation := headerContent[begin:end]
|
|
||
| markerBegin := "/*\n *\n * Stop now, if you are only interested in the API." | ||
| markerEnd := "/* cgltf is distributed under MIT license:" | ||
| replacement := "/**\n * Implementation removed by conan during packaging.\n * Don't forget to link libs provided in this package.\n */\n\n" |
There was a problem hiding this comment.
This replacement text is written into the shipped cgltf.h / cgltf_write.h that consumers see, and it states the implementation was "removed by conan during packaging" — but this is an LLAR package; Conan isn't involved. The string looks copied from the Conan Center recipe. Suggest wording that's accurate for this project, e.g. "Implementation removed by LLAR during packaging."
| cmakeLists := ctx.Proj.readFile("v1.5/CMakeLists.txt")! | ||
| os.writeFile(filepath.join(ctx.SourceDir, "CMakeLists.txt"), cmakeLists, 0o644)! | ||
| os.writeFile(filepath.join(sourceDir, "cgltf.c"), []byte("#define CGLTF_IMPLEMENTATION\n#include \"cgltf.h\"\n"), 0o644)! | ||
| os.writeFile(filepath.join(sourceDir, "cgltf_write.c"), []byte("#define CGLTF_WRITE_IMPLEMENTATION\n#include \"cgltf_write.h\"\n"), 0o644)! |
There was a problem hiding this comment.
cgltf_write.c (and cgltf_write.h) is compiled into the cgltf library and installed, but the consumer test only defines CGLTF_IMPLEMENTATION and never exercises any cgltf_write API. So the write half of the shipped interface is untested. Either add a small CGLTF_WRITE_IMPLEMENTATION / write-API call to the consumer so the installed write header + object code are validated, or drop cgltf_write.* from the build to keep the formula minimal if write support isn't intended.
| @@ -0,0 +1,156 @@ | |||
| import ( | |||
There was a problem hiding this comment.
The reference recp/cglm/v0.7.2/Cglm_llar.gox documents its build contract, option mapping, metadata fallback, and test rationale; this formula has no explanatory comments. A few genuinely non-obvious steps would benefit from a short note:
- Synthetic build (lines 88–94): cgltf is header-only and ships no CMake build, so this formula manufactures a
CMakeLists.txt+.ctranslation units that#define ..._IMPLEMENTATION. - Header stripping (lines 109–120): the implementation is now compiled into
libcgltf, so theCGLTF_IMPLEMENTATIONblock is excised from the shipped headers to avoid duplicate-symbol conflicts for consumers. - Options (lines 67–70):
shared→BUILD_SHARED_LIBS,fPIC→CMAKE_POSITION_INDEPENDENT_CODE.
1 participant
Closes #98.
Adds the jkuhlmann/cgltf Formula from CCI snapshot ffe30df101afd4dc95aac2f14b25bf345e64d7be. The v1.5 threshold serves the CCI recipe range through v1.15, with shared/fPIC defaults, generated implementation sources, installed license, stripped implementation bodies, and an independent C++ consumer.
Complete upstream v-tag ordering was checked with LLAR's GNU comparator; no custom comparator is needed.
Validation: git diff --check and Formula filename/path checks completed. Per request, local LLAR builds and CI were not awaited.