formula: add rscada/libmbus - #218
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: rscada/libmbus formula
Solid, well-structured formula that closely follows the established bdwgc/libatomic_ops template — options + filter, license copy, relocatable pkg-config rewrite, and a pkg-config-driven onTest consumer. Security and performance passes found nothing to change. A few maintainability/consistency notes below; none are blocking.
versions.json: uses 2-space indentation, while sibling manifests (bdwgc/libatomic_ops,recp/cglm) use tabs. Pure formatting nit.- The
filter/target.options[...][0]indexing pattern (no per-key default fallback) matches the mergedlibatomic_opsformula exactly, so it is assumed safe under the same DSL guarantee thatdefaultsbackfilltarget.options; not flagged.
|
|
||
| licenseDir := filepath.join(installDir, "licenses") | ||
| os.mkdirAll(licenseDir, 0o755)! | ||
| os.writeFile(filepath.join(licenseDir, "LICENSE"), os.readFile(filepath.join(ctx.SourceDir, "LICENSE"))!, 0o644)! |
There was a problem hiding this comment.
[P2] Only LICENSE is copied; confirm upstream ships no COPYING
The sibling bdwgc/libatomic_ops copies both LICENSE and COPYING into licenses/. libmbus copies only LICENSE. If the upstream tarball also ships a COPYING (or other license file), it will be omitted from the package. Please confirm LICENSE is the only license file the release ships.
| os.mkdirAll(filepath.join(ctx.SourceDir, "m4"), 0o755)! | ||
| if slices.contains(target.require["os"], "darwin") { | ||
| glibtoolize! "--copy", "--force" | ||
| } else { | ||
| libtoolize! "--copy", "--force" | ||
| } | ||
| autoreconf! "-fi" |
There was a problem hiding this comment.
[P3] Non-obvious autotools bootstrap steps are undocumented
The m4/ pre-creation (line 62) and the glibtoolize vs libtoolize darwin branch (lines 63-67) are non-obvious workarounds a future maintainer wouldn't infer (Homebrew renames the GNU tool; AC_CONFIG_MACRO_DIRS needs m4/ to exist). The sibling libatomic_ops documents its comparable non-obvious step (pkg-config relocation) with a comment. A one-line comment here would match repo norms.
| os.writeFile(flagsFile, []byte(pkgconfig.lookup("libmbus")!), 0o644)! | ||
|
|
||
| binary := filepath.join(testDir, "consumer") | ||
| cc! sourcePath, "-o", binary, "@"+flagsFile |
There was a problem hiding this comment.
[P3] Test compile omits an explicit -std flag used by the sibling
The sibling libatomic_ops onTest compiles the consumer with an explicit -std=c11. Here cc! is invoked without a -std= flag, so the consumer relies on the compiler's default standard. Minor consistency point — add -std=c11 (or whatever libmbus's headers require) if a specific standard is expected.
| func normalize(version string) string { | ||
| if strings.hasPrefix(version, "v") { | ||
| return version | ||
| } | ||
| return "v" + version | ||
| } |
There was a problem hiding this comment.
[P2] Comparator lacks rationale comment; normalization may be dead code
This comparator is byte-for-byte identical to recp/cglm/Cglm_cmp.gox but drops the block comment explaining why normalization is needed. Every other custom comparator in the repo documents its deviation (cglm's mixed v/bare tags, ChaiScript's Release-* aliases, even ruckig's one-liner).
Upstream rscada/libmbus tags are uniformly bare (0.10.2, 0.10.1, 0.9.0, ...), so the hasPrefix(version, "v") early-return branch is effectively dead and normalize just unconditionally prepends v. Consider either (a) a plain semver.Compare wrapper with a short note like ruckig's, or (b) keeping this form but adding a one-line comment that tags are bare and v is prepended only to satisfy Go semver syntax.
| pcPath := filepath.join(installDir, "lib", "pkgconfig", "libmbus.pc") | ||
| pc := string(os.readFile(pcPath)!) | ||
| pc = strings.replace(pc, "prefix="+installDir, "prefix=$${pcfiledir}/../..", 1) | ||
| pc = strings.replace(pc, "exec_prefix="+installDir, "exec_prefix=$${prefix}", 1) | ||
| pc = strings.replace(pc, "libdir="+filepath.join(installDir, "lib"), "libdir=$${prefix}/lib", 1) | ||
| pc = strings.replace(pc, "includedir="+filepath.join(installDir, "include"), "includedir=$${prefix}/include", 1) | ||
| os.writeFile(pcPath, []byte(pc), 0o644)! |
There was a problem hiding this comment.
[P3] pkg-config relocation lacks explanatory comment
This .pc rewrite (swapping the build-time absolute prefix/exec_prefix/libdir/includedir for a relocatable $${pcfiledir}-relative form) is functionally identical in intent to bdwgc/libatomic_ops lines 117-126, which prefaces it with a comment explaining the goal. The $${pcfiledir}/../.. construct is non-obvious; a matching comment would aid maintainability. (Logic itself is correct — prefix= is rewritten before exec_prefix=, each key appears once, count of 1 is right.)
1 participant
Closes issues/32