feat(mikktspace): add LLAR formula - #145
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: mmikk/MikkTSpace formula
The formula is functional and follows the general shape of the existing formulas (CMake build → consumer smoke test → link-flag metadata). The vendored CMakeLists.txt giving the header-only-ish upstream a clean install target with GNUInstallDirs and WINDOWS_EXPORT_ALL_SYMBOLS is a tidy approach, and setting LD_LIBRARY_PATH/DYLD_LIBRARY_PATH for the shared-build test is a nice touch the static-only formulas didn't need.
A few things worth addressing before merge — the main one is the unchecked [0] slice indexing on target.options, which is a latent crash if a target ever arrives without those options set. The rest are consistency/maintainability items. See inline comments.
Non-inline note — missing explanatory comments. Unlike the neighboring recp/cglm and json-c formulas, which document their non-obvious decisions, this formula ships with no comments. The most valuable additions would explain: why a raw commit SHA is pinned in fromVer (MikkTSpace publishes no release tags), the shared/fPIC option semantics, and the license-via-header handling. This is a consistency-with-repo-standard note, not a blocker.
| shared := target.options["shared"][0] == "ON" | ||
| fPIC := target.options["fPIC"][0] == "ON" |
There was a problem hiding this comment.
[P1] Unchecked [0] index on target.options may crash
target.options["shared"][0] / target.options["fPIC"][0] index element 0 of a slice that may be empty. filter only rejects unknown option names and non-ON/OFF values — it never guarantees shared/fPIC are present, so a target with those options absent passes the filter and then triggers an index-out-of-range here (and again at line 91 in onTest).
The recp/cglm formula avoids this by using slices.contains, which is safe on an empty/absent slice and honors the default:
shared := slices.contains(target.options["shared"], "ON")
fPIC := slices.contains(target.options["fPIC"], "ON")| sourcePath := filepath.join(testDir, "consumer.c") | ||
| os.writeFile(sourcePath, []byte(consumerSource), 0o644)! | ||
| binary := filepath.join(testDir, "consumer") | ||
| shared := target.options["shared"][0] == "ON" |
There was a problem hiding this comment.
[P2] onTest re-derives -lm from options; duplicates onBuild logic
onTest recomputes shared and the -lm linkage from target.options/target.require (lines 91, 99-101), duplicating the identical logic in onBuild (lines 77-79) and the CMake check_function_exists(pow ...) probe. There are now three places that must agree on whether libm is needed. onBuild already encodes the correct consumer flags via ctx.setMetadata; consider having onTest consume that instead of re-deriving it (this also matches how cglm/json-c derive test flags from the installed output). Same [0] indexing concern as above applies at line 91.
| @@ -0,0 +1,4 @@ | |||
| { | |||
| "path": "mmikk/MikkTSpace", | |||
There was a problem hiding this comment.
[P3] versions.json uses 2-space indent; siblings use tabs
recp/cglm/versions.json, json-c/json-c/versions.json, and madler/zlib/versions.json are all tab-indented; this file uses 2 spaces. Re-indent with tabs for consistency.
|
|
||
| licenseDir := filepath.join(installDir, "licenses") | ||
| os.mkdirAll(licenseDir, 0o755)! | ||
| os.writeFile(filepath.join(licenseDir, "LICENSE"), os.readFile(filepath.join(ctx.SourceDir, "mikktspace.h"))!, 0o644)! |
There was a problem hiding this comment.
[P3] LICENSE file is the full mikktspace.h header
This writes the entire mikktspace.h (C source included) as licenses/LICENSE. This is accurate — upstream MikkTSpace ships no standalone LICENSE file and the full zlib license text lives in the header's top comment — but a LICENSE file containing C source reads like a mistake to anyone auditing licenses. Either extract just the license comment block, or add a one-line comment noting "upstream ships no LICENSE file; the zlib license text lives in mikktspace.h." (Note the other three formulas don't capture a license at all.)
| lastErr! | ||
|
|
||
| if shared { | ||
| os.setenv("LD_LIBRARY_PATH", filepath.join(installDir, "lib"))! |
There was a problem hiding this comment.
[P3] setenv overwrites LD_LIBRARY_PATH instead of prepending
os.setenv replaces LD_LIBRARY_PATH/DYLD_LIBRARY_PATH with a single path rather than prepending the install lib dir to the existing value. If the shared consumer ever needs a library reachable only via a pre-existing LD_LIBRARY_PATH, the test could fail to locate it. Minor/functional (not a security issue — the written path is trusted); worth prepending installDir/lib + ":" + os.getenv("LD_LIBRARY_PATH") to be safe.
1 participant
Adds an LLAR Formula for mmikk/MikkTSpace from the Conan Center cci.20200325 recipe.
Validation: git diff --cached --check passed. Local formula tests and CI were intentionally not run per request.