feat(bzip3): add LLAR formula - #132
Conversation
36d2470 to
756d7dd
Compare
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: iczelia/bzip3 1.3.2 formula
The recipe closely follows the repo's idioms (lowercase slices.contains, pkgconfig.use + pkgconfig.lookup("bzip3")! for full cflags-and-libs metadata, a separate _llar_consumer test tree, and cache-hit-safe metadata derived from the installed .pc rather than build state). One correctness issue in the consumer test's link step is worth addressing; the rest are minor.
Reviewers run: code-quality, performance, security, documentation-accuracy.
- Performance: no issues —
configure/build/installrun once, the test builds only the tiny consumer, and the cache-hit path relies solely on the persisted install dir. - Security: no issues — file permissions (0755/0644) are standard, scratch paths are derived deterministically from
ctx.SourceDir(no shared/tmp), options are strictly whitelisted to ON/OFF before use, and no secrets are touched.
Additional note (no inline anchor): when with_util=ON, upstream installs the bzip3 CLI into bin/, but onTest only ever exercises the library. Per the skill's validation guidance ("each retained output-changing option"), consider adding a path that runs the installed CLI when with_util=ON, or document why it is intentionally left unverified.
|
|
||
| tc := cmake.new(testDir, testBuild, "") | ||
| tc.use installDir | ||
| tc.define "CMAKE_C_FLAGS", metadata |
There was a problem hiding this comment.
Consumer test may not link libbzip3 reliably. The generated CMakeLists.txt (lines 80-83) has only add_executable(consumer consumer.c) — no find_package and no target_link_libraries. The library is wired in exclusively through CMAKE_C_FLAGS here (-I... -L... -lbzip3).
CMake injects CMAKE_C_FLAGS into the compile <FLAGS> slot, which appears before <OBJECTS> on the executable link line. So the effective order is roughly cc -lbzip3 ... consumer.c.o -o consumer. With GNU ld's single-pass resolution, a -lbzip3 that precedes the object referencing its symbols (bz3_new, bz3_version, bz3_free) can be discarded, yielding undefined-reference errors. Whether it links is toolchain/order-dependent, which makes the test unreliable.
bzip3 1.3.2 installs a CMake package export (bzip3-config, namespace bzip3::, target bz3), so the robust fix mirrors the json-c recipe:
find_package(bzip3 REQUIRED CONFIG)
add_executable(consumer consumer.c)
target_link_libraries(consumer PRIVATE bzip3::bz3)with the already-present tc.use installDir supplying CMAKE_PREFIX_PATH (this also makes CMAKE_C_FLAGS/CMAKE_BUILD_RPATH unnecessary). Alternatively, if the intent is specifically to validate the pkg-config metadata, compile directly with cc placing the full metadata string after the source/object (the pattern recp/cglm uses), not via CMAKE_C_FLAGS.
| tc := cmake.new(testDir, testBuild, "") | ||
| tc.use installDir | ||
| tc.define "CMAKE_C_FLAGS", metadata | ||
| tc.define "CMAKE_BUILD_RPATH", filepath.join(installDir, "lib") |
There was a problem hiding this comment.
CMAKE_BUILD_RPATH is set unconditionally, but for the default shared: "OFF" build there is no shared object to locate at runtime, so the rpath is dead weight; it only matters when shared=ON. If the link step is fixed via the installed CMake package (see comment above), CMake's imported-target machinery handles rpath for shared libraries automatically and this line can be dropped.
| if (state == 0) { | ||
| return 1; | ||
| } | ||
| if (bz3_version() == 0) { |
There was a problem hiding this comment.
Weak assertion: bz3_version() returns const char * (upstream: BZIP3_API const char * bz3_version(void);) and is always non-NULL, so if (bz3_version() == 0) is effectively a dead branch — it compiles (pointer vs 0) but never validates anything meaningful. Consider checking the returned string's contents (e.g. non-empty) if the goal is to exercise the symbol, or drop the check. Minor.
| c.defineBool "BUILD_SHARED_LIBS", shared | ||
| c.defineBool "CMAKE_POSITION_INDEPENDENT_CODE", fPIC | ||
| c.defineBool "BZIP3_ENABLE_PTHREAD", withThread | ||
| // 1.3.2 defaults this upstream optimization to ON, while the Conan |
There was a problem hiding this comment.
The comment attributes the rationale to the Conan wrapper ("the Conan project wrapper leaves it disabled") but this recipe is not Conan and unconditionally sets BZIP3_ENABLE_ARCH_NATIVE to false. Consider stating the recipe's own decision directly, e.g. "upstream 1.3.2 defaults this ON; we disable it here for portable, reproducible builds." Minor doc-accuracy nit.
1 participant
Summary
iczelia/bzip3LLAR Formula from upstream1.3.2shared,fPIC,with_thread, andwith_utilbuild choicesbzip3.pcmetadata through the complete pkg-config lookupcorpustag below numbered releases so an unpinned build selects1.5.3Closes #106
Validation
llar test --verbose ./iczelia/bzip3 --os darwin --arch arm64(unpinned latest selects1.5.3)llar test --verbose ./iczelia/bzip3@1.3.2 --os darwin --arch arm64llar test --verbose ./iczelia/bzip3@1.4.0 --os darwin --arch arm64llar test --verbose ./iczelia/bzip3@1.5.3 --os darwin --arch arm64(fresh build)1.5.3selection to exercise the cache-hit consumer path1.3.1and explicitcorpusare rejected below the1.3.2Formula boundary