formula: add TsudaKageyu/minhook - #215
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: TsudaKageyu/minhook v1.3.4
Thanks for the addition. The formula follows repo conventions well (structure, license copy, relocatable pkgconfig, ! error idioms, comparator normalization matching cglm/ChaiScript). No performance or security concerns were found.
The one substantive theme, corroborated across reviewers, is that the build/test path is inconsistent about OS: the pkgconfig -l name is hardcoded to the Windows .x64/.x32 spelling for all targets, while the onTest step compiles and runs with Unix cc/exec/LD_LIBRARY_PATH. Please confirm which platform CI actually targets and gate accordingly. Details inline.
| // WIN32 Release/RelWithDebInfo/MinSizeRel postfixes are .x64 / .x32; | ||
| // Debug adds a trailing d. Non-Windows CMake uses libminhook, but this | ||
| // package is a Windows x86/x86_64 API hooking library. | ||
| lib := "minhook.x64" |
There was a problem hiding this comment.
[P1] Lib name hardcoded to Windows .x64/.x32 for all targets
The comment on the lines above states "Non-Windows CMake uses libminhook", yet lib is unconditionally set to minhook.x64/minhook.x32 with no OS check (the branch on line 45 only inspects arch). Upstream's CMakeLists.txt applies the empty-prefix .x64/.x32 postfix only inside its if(WIN32) block; on non-Windows it produces libminhook.a/.so.
This name flows into the generated pkgconfig Libs: -lminhook.x64 (line 74). On a Linux/macOS build the linker will look for libminhook.x64.*, which does not exist, so the onTest link on line 96 fails. Since onTest uses cc/exec/LD_LIBRARY_PATH (Unix constructs), a non-Windows target seems expected. Please gate the lib name on target.require["os"] (default -lminhook off Windows), or restrict the formula to Windows in filter and validate on a Windows runner.
| "shared": "OFF", | ||
| } | ||
|
|
||
| filter => { |
There was a problem hiding this comment.
[P2] filter accepts all OS targets for a Windows-only library
The filter only validates the shared option and accepts every OS target. For a strictly Windows library, consider rejecting non-Windows targets by inspecting target.require["os"] (see the os-aware patterns in rxi/microtar and tidwall/neco). As written the formula advertises support on platforms where it cannot build/run, which is the root cause of the lib-name mismatch above.
| if target.options["shared"][0] == "ON" { | ||
| 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.
[P2] Shared build sets only LD_LIBRARY_PATH/DYLD, not Windows PATH
For a shared Windows build the runtime DLL is located via the loader PATH (and is typically installed under bin), not LD_LIBRARY_PATH/DYLD_LIBRARY_PATH. The sibling rxi/microtar formula prepends installDir/bin to PATH when target.require["os"] contains windows. If a shared Windows test is intended, mirror that pattern so the DLL is found at runtime.
| shared := target.options["shared"][0] == "ON" | ||
|
|
||
| // WIN32 Release/RelWithDebInfo/MinSizeRel postfixes are .x64 / .x32; | ||
| // Debug adds a trailing d. Non-Windows CMake uses libminhook, but this |
There was a problem hiding this comment.
[P3] Comment mentions Debug 'd' postfix but only Release is built
The comment says "Debug adds a trailing d", but c.buildType "Release" (line 53) is hardcoded and no d-suffixed name is ever produced. This is accurate about upstream but can mislead a reader into thinking a Debug code path exists here. Consider trimming the Debug mention or noting that only Release is built.
| // Upstream mixes unprefixed GNU-style tags (1.2.1.1, 1.2.1A) with v-prefixed | ||
| // releases (v1.3.4). Strip an optional v, then compare as Go semver. | ||
| func normalize(version string) string { | ||
| return "v" + strings.trimPrefix(version, "v") |
There was a problem hiding this comment.
[P3] normalize does not guard against invalid semver tags
normalize unconditionally does "v" + trimPrefix(version, "v"). For the upstream GNU-style tags the comment cites (e.g. 1.2.1A), this yields v1.2.1A, which is not valid Go semver — semver.Compare treats invalid versions as lowest, so such tags sort below everything. Not a problem for selecting the current v1.3.4 default, but worth a note if those older tags ever need to order correctly.
1 participant
Closes issues/15