formula: msgpack/msgpack-c - #205
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
Adds an XGo/LLGo build definition for msgpack-c (c-6.1.0) plus a version comparator and versions.json. The recipe closely mirrors the established google/double-conversion sibling: shared/fPIC options, cmake build, relocatable pkg-config rewrite, and an embedded consumer test. versions.json matches the sibling schema exactly, and the comparator is functionally correct for the real tag set.
A few consistency/robustness points below. None are blocking.
Reviewed: code quality, performance, security, documentation accuracy. No security or performance concerns found.
| // publish relocatable pkg-config metadata under Conan's msgpack name. | ||
| pcDir := filepath.join(installDir, "lib", "pkgconfig") | ||
| pcPath := filepath.join(pcDir, "msgpack-c.pc") | ||
| pc := string(os.readFile(pcPath)!) |
There was a problem hiding this comment.
[P2] Unconditional .pc read can hard-fail the build
The upstream-generated msgpack-c.pc is read unconditionally with os.readFile(pcPath)!. The closest sibling recipe (google/double-conversion/v3.3.0/doubleconversion_llar.gox) deliberately guards this path with os.stat/os.isNotExist and synthesizes a relocatable .pc when upstream does not install one. If msgpack-c's CMake does not emit lib/pkgconfig/msgpack-c.pc (different name/case, or a different libdir), this recipe panics instead of degrading gracefully. Consider mirroring the double-conversion pattern: stat first, synthesize the .pc if missing, otherwise rewrite the generated one.
| import "strings" | ||
|
|
||
| // The repository publishes both C tags (`c-X.Y.Z`) and C++ tags (`cpp-*`), | ||
| // plus a few unprefixed historical tags. GNU version comparison ranks |
There was a problem hiding this comment.
[P3] Comparator comment cites GNU version rules, but code uses semver
The comment justifies the design via GNU version comparison ("letters above digits, so cpp-* would outrank c-*"), but the comparator actually uses semver.Compare on normalized strings. The guarantee that non-c- tags sort below c- releases comes purely from the "v0.0.0-" + version prerelease mapping under semver, not from any GNU letter-vs-digit rule. The functional behavior is correct for the real tag set, but the stated rationale describes a comparator the code does not use and could mislead a future reader. Also worth a one-line note that ordering among non-c- tags is undefined-by-design (they all collapse to v0.0.0-<tag> and sort by raw prerelease string).
|
|
||
| onBuild ctx => { | ||
| installDir := ctx.outputDir | ||
| shared := target.options["shared"][0] == "ON" |
There was a problem hiding this comment.
[P3] Prefer slices.contains over direct [0] indexing for options
target.options["shared"][0] / target.options["fPIC"][0] (also at line 118) use direct index access. The sibling doubleconversion_llar.gox uses slices.contains(target.options["shared"], "ON"), which is the established convention here and is empty/order-safe. This is safe today only because defaults always seeds both keys. The absence of a slices import (present in the sibling) is a tell that this diverged from the pattern. Recommend matching the convention for consistency and safety.
| func normalize(version string) string { | ||
| if strings.hasPrefix(version, "c-") { | ||
| return "v" + strings.trimPrefix(version, "c-") | ||
| } | ||
| return "v0.0.0-" + version | ||
| } | ||
|
|
There was a problem hiding this comment.
This library distinguishes between C and C++ based on version numbers. This scenario has not yet been considered by llar and requires discussion — do not merge.
1 participant
Closes #51