feat(streamvbyte): add LLAR formula - #129
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fast-pack/streamvbyte formula
Solid formula that follows the repo's established patterns well — the shared/static option, filter, cache-hit-safe flag derivation from installDir (not build metadata), and panic-on-real-os.stat-errors are all done correctly, matching the cglm/json-c analogs. Security review found nothing of concern (all exec calls use array-form args; the patch write and LD_LIBRARY_PATH/DYLD_LIBRARY_PATH are scoped to the build tree).
Two verified correctness issues undercut the PR's claimed v0.5.0–v3.0.0 range, plus a couple of consistency nits. Inline comments below.
Summary of findings
- [P1] macOS RPATH patch hunk header targets the wrong line → the patch fails to apply on the exact (macOS shared) path it's meant to fix.
- [P1] Install dir is hardcoded to
lib, but v2.0.0/v3.0.0 install to${CMAKE_INSTALL_LIBDIR}(oftenlib64) → link failure and a silent wrong-lstreamvbyte_staticfallback on 64-bit distros. - [P2] Static-archive-name detection is duplicated verbatim in
onBuildandonTest. - [P3] Filename casing and
versions.jsonindentation diverge from repo convention.
| patchPath := filepath.join(ctx.SourceDir, "_llar_macos_rpath.patch") | ||
| patchText := `--- CMakeLists.txt | ||
| +++ CMakeLists.txt | ||
| @@ -1,1 +1,1 @@ |
There was a problem hiding this comment.
The macOS RPATH patch will fail to apply — wrong hunk line number.
The hunk header hardcodes @@ -1,1 +1,1 @@, asserting set(CMAKE_MACOSX_RPATH OFF) is line 1 of CMakeLists.txt. Verified against upstream: at v0.5.0 line 1 is cmake_minimum_required(VERSION 3.3) and the RPATH setting is on line 2. With patch --batch --forward, the context won't match at line 1, so the hunk is rejected and lastErr! (line 67) then aborts the build — on exactly the macOS shared-build path this block is meant to support.
Since content is already read into memory (line 56), a simpler and robust fix is an in-memory replace instead of shelling out to patch:
content = strings.replaceAll(content, "set(CMAKE_MACOSX_RPATH OFF)", "set(CMAKE_MACOSX_RPATH ON)")
os.writeFile(filepath.join(ctx.SourceDir, "CMakeLists.txt"), []byte(content), 0o644)!
| // libstreamvbyte_static.a; v0.5.3 and newer name it libstreamvbyte.a. | ||
| library := "streamvbyte" | ||
| if !shared { | ||
| _, err := os.stat(filepath.join(installDir, "lib", "libstreamvbyte.a")) |
There was a problem hiding this comment.
lib is hardcoded, but v2.0.0/v3.0.0 install to ${CMAKE_INSTALL_LIBDIR} (often lib64).
Verified against upstream: v0.5.0–v1.0.0 install with DESTINATION lib, but v2.0.0 and v3.0.0 include(GNUInstallDirs) and install with ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR} (no override). On common 64-bit distros (Fedora/RHEL/openSUSE) that resolves to lib64.
Consequences on those platforms for v2.0.0/v3.0.0 — contradicting the PR's claimed v0.5.0–v3.0.0 range:
- The
os.stat(installDir/lib/libstreamvbyte.a)check (lines 82/109) fails, so the code silently falls back tolibrary = "streamvbyte_static"— butstreamvbyte_statichasn't existed as a target since v0.5.2, producing a wrong-lstreamvbyte_staticflag. - The
-L.../lib(lines 91/122) andLD_LIBRARY_PATH/DYLD_LIBRARY_PATH(lines 129/130) paths point at the wrong directory, so the consumer link/run fails.
Consider resolving the actual libdir (e.g. probe both lib and lib64, or set CMAKE_INSTALL_LIBDIR=lib explicitly in the cmake configure as json-c does) rather than assuming lib.
|
|
||
| shared := slices.contains(target.options["shared"], "ON") | ||
| // Keep the cache-hit consumer aligned with the installed archive name. | ||
| library := "streamvbyte" |
There was a problem hiding this comment.
Duplicated static-archive-name detection between onBuild and onTest.
The os.stat(libstreamvbyte.a) → fallback-to-streamvbyte_static block (lines 80–88) is repeated verbatim in onTest (lines 107–115). The two copies must stay in lockstep, and any fix to the detection (e.g. the lib64 issue above) has to be applied in both places. Extracting a small helper, e.g. staticLibName(installDir, shared), would remove the drift risk.
| @@ -0,0 +1,4 @@ | |||
| { | |||
| "path": "fast-pack/streamvbyte", | |||
There was a problem hiding this comment.
versions.json uses 2-space indentation; the repo convention is tabs.
The three existing versions.json files (recp/cglm, madler/zlib, json-c/json-c) indent with a tab. Re-indent with tabs for consistency. (Related nit: this formula file is named streamvbyte_llar.gox lowercase, while the existing formulas are capitalized — Zlib_llar.gox, Cglm_llar.gox, Jsonc_llar.gox; rename to Streamvbyte_llar.gox unless the loader keys off the id directive rather than the filename.)
1 participant
Add an LLAR Formula for
fast-pack/streamvbyteacross the verified upstream range fromv0.5.0throughv3.0.0.The implementation includes:
fromVeratv0.5.0after checking the older CMake contracts.fromVer, and cache-hit consumer behavior.This completes the closed streamvbyte translation with explicit build-layout compatibility across the upstream release range.
Closes #113