formula: dougbinks/enkiTS - #201
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
FennoAI Review — enkiTS formula
Nice, clean addition. The build/test formula closely mirrors the established streamvbyte and neco formulas, and the filter control flow is correct. Findings below, plus one note that couldn't be placed inline.
Note (body-only): In the .pc Libs, the non-linux/freebsd branch unconditionally appends -lc++ (enkits_llar.gox:83). That's right for macOS/clang but wrong for a MinGW/libstdc++ target. neco adds nothing in its else branch to avoid this assumption — consider matching that or adding a comment on the intended target set.
| @@ -0,0 +1,11 @@ | |||
| import "strings" | |||
|
|
|||
| // Complete tag set: v1.0 .. v1.12. GNU compares the first digit after "v1.", | |||
There was a problem hiding this comment.
[P2] Comparator is a no-op and its comment is inaccurate
normalize strips the v prefix, and compareVer immediately re-adds it ("v"+normalize(...)), so for v1.12 the round-trip returns the input unchanged — this comparator is behaviorally identical to the default semver.Compare(a.Version, b.Version).
The comment's premise is also wrong: semver.Compare parses 1.12 as minor 12 and 1.9 as minor 9, so it already orders v1.12 > v1.9 correctly. The described "GNU compares the first digit" failure does not apply here. Unlike Cglm_cmp.gox (adds v only when missing) or chaiscript_cmp.gox (remaps alias tags), enkiTS has a uniform v1.0 .. v1.12 tag set with no irregularity.
Consider removing enkits_cmp.gox entirely (rely on the default comparator), or if a custom one is required, drop the strip/re-add and fix the comment.
| shared := target.options["shared"][0] == "ON" | ||
| fPIC := target.options["fPIC"][0] == "ON" |
There was a problem hiding this comment.
[P2] Unsafe [0] index on target.options may panic
target.options["shared"][0] / target.options["fPIC"][0] index element [0] directly, which panics if the option slice is ever empty. Every sibling formula (streamvbyte, neco) uses the panic-safe slices.contains(target.options["shared"], "ON") instead — and this file already imports slices. Same pattern recurs in onTest (line 125). defaults guarantees a value in the normal flow, but the slices.contains form is the safer, consistent convention.
| os.writeFile(filepath.join(licenseDir, "License.txt"), os.readFile(filepath.join(ctx.SourceDir, "License.txt"))!, 0o644)! | ||
|
|
||
| version := "" | ||
| for line in string(os.readFile(filepath.join(ctx.SourceDir, "CMakeLists.txt"))!).split("\n") { |
There was a problem hiding this comment.
[P3] CMakeLists.txt read twice (second read is post-mutation)
CMakeLists.txt is read into source (line 47), mutated, and written back (line 51); then re-read here to extract the version. The version line (project(... VERSION ...)) is untouched by the ARCHIVE/LIBRARY replace, so you can iterate over the already-in-memory source (for line in source.split("\n")) and drop the second os.readFile. streamvbyte reads once and reuses.
| source := string(os.readFile(cmakeLists)!) | ||
| // v1.12 names only ARCHIVE, so ELF shared objects and Windows DLLs are | ||
| // omitted from install. Keep the export while installing every artifact. | ||
| source = strings.replace(source, " ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR})", " ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}\n LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}\n RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR})", 1) |
There was a problem hiding this comment.
[P3] Brittle CMakeLists replace can silently no-op
This strings.replace(..., 1) matches an exact literal with hardcoded 8-space indentation. If upstream reformats the install rule, the replace silently no-ops and shared objects / DLLs are omitted from the install with no error. streamvbyte guards its equivalent mutation with a strings.contains(...) check. Consider asserting the replacement actually occurred (compare pre/post and panic if unchanged) so an upstream format change fails loudly.
| fPIC := target.options["fPIC"][0] == "ON" | ||
|
|
||
| cmakeLists := filepath.join(ctx.SourceDir, "CMakeLists.txt") | ||
| source := string(os.readFile(cmakeLists)!) |
There was a problem hiding this comment.
[P3] Magic -DENKITS_TASK_PRIORITIES_NUM=3 is unexplained
The .pc Cflags hardcode -DENKITS_TASK_PRIORITIES_NUM=3, but this value is not passed to the library build via c.define. If the built library used a different (default) value, consumers compiling with =3 risk a compile-time-constant/ABI mismatch. Add a brief comment explaining why 3, and confirm it matches the value the installed library was built with.
| func normalize(version string) string { | ||
| return strings.trimPrefix(version, "v") | ||
| } | ||
|
|
||
| compareVer (a, b) => { | ||
| return semver.Compare("v"+normalize(a.Version), "v"+normalize(b.Version)) | ||
| } |
There was a problem hiding this comment.
Seems like LLM got dumb
1 participant
Closes #31