formula: Auburn/FastNoise2 - #204
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Auburn/FastNoise2 v1.1.1 recipe
Solid new recipe that closely follows the established sibling conventions (intel/libipt, Amanieu/asyncplusplus): the filter/defaults/options contract, relocatable pkg-config .pc generation, license copy, fail-fast version guard, and a real API smoke test in onTest. Security review found no concerns — file permissions, exec! argv usage, and os.setenv all match the repo baseline.
The one issue worth acting on before merge is the missing CMake build type, which for a SIMD-critical library ships an unoptimized artifact and also renders the FastNoiseD detection logic unreachable. A few minor consistency/documentation nits are noted inline.
| shared := target.options["shared"][0] == "ON" | ||
| fPIC := target.options["fPIC"][0] == "ON" | ||
|
|
||
| c := cmake.new(ctx.SourceDir, filepath.join(ctx.SourceDir, "_build"), installDir) |
There was a problem hiding this comment.
[P1] No CMake build type set — ships an unoptimized SIMD library
The build never sets a build type (no c.buildType "Release"), and upstream's CMakeLists.txt does not default CMAKE_BUILD_TYPE. On single-config generators (Make/Ninja) that means the compiler runs at -O0 with no -DNDEBUG. For FastNoise2 — a template/SIMD library whose entire value is runtime performance — this ships a dramatically slower artifact. Sibling recipes such as Amanieu/asyncplusplus/v1.0/asyncplusplus_llar.gox:42 set c.buildType "Release" right after cmake.new. Recommend adding c.buildType "Release" here.
| libName := "FastNoise" | ||
| for entry in os.readDir(filepath.join(installDir, "lib"))! { | ||
| if entry.isDir { | ||
| continue | ||
| } | ||
| name := entry.name | ||
| if name.hasPrefix("libFastNoiseD.") || name.hasPrefix("FastNoiseD.") { | ||
| libName = "FastNoiseD" | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
[P2] FastNoiseD debug-suffix detection is effectively dead code
Upstream applies the D suffix via set_target_properties(... DEBUG_POSTFIX D), which only takes effect for a Debug configuration. Because this recipe never selects a Debug build type, the installed library is always libFastNoise.*, so this loop can never match and libName stays "FastNoise". As written it reads as if it handles a real case but is unreachable. Either drop the loop for simplicity, or — if Debug is meant to be supported — drive the build type from an option and add a comment explaining when this branch triggers. (Closely tied to the missing-build-type finding above.)
| includedir=$${prefix}/include | ||
|
|
||
| Name: FastNoise2 | ||
| Description: Modular node graph based noise generation library using SIMD, C++17 and templates |
There was a problem hiding this comment.
[P2] pkg-config Description misstates the upstream tagline
Upstream describes itself as "Modular node based noise generation library using SIMD, focused on performance, modern C++17...". This line invents "node graph based" and appends "and templates," wording upstream does not use. The Description: field is operator-facing (pkg-config --description). Recommend matching the upstream README tagline verbatim.
| version := "" | ||
| for line in string(os.readFile(filepath.join(ctx.SourceDir, "CMakeLists.txt"))!).split("\n") { | ||
| if line.hasPrefix("project(FastNoise2 VERSION ") { | ||
| version = line.trimPrefix("project(FastNoise2 VERSION ").split(")")[0] |
There was a problem hiding this comment.
[P3] Fragile version parse via split(")")[0]
For the exact v1.1.1 line project(FastNoise2 VERSION 1.1.1) this yields 1.1.1 correctly. But it assumes nothing follows the version before ). A future tag like project(FastNoise2 VERSION 1.1.1 LANGUAGES CXX) would capture 1.1.1 LANGUAGES CXX into the Version: field. intel/libipt tokenizes with strings.fields for robustness. Low risk while fromVer pins a single version — worth hardening or adding a note on the assumption.
1 participant
Closes #53