feat(libatomic_ops): add LLAR formula - #139
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: bdwgc/libatomic_ops formula
Solid new formula. The structure follows the repo conventions well — cache-hit-safe onTest (independent _llar_consumer tree resolving via the installed CMake config), a strict option allowlist in filter, license capture into licenses/, and a meaningful consumer test exercising both atomic_ops and atomic_ops_gpl. The lowercase libatomic_llar.gox stem is correct per write-formula guidance (the capitalized sibling recipes are the outliers).
A few points to consider, noted inline. None are blocking.
On the pkg-config $${pcfiledir} line: the path arithmetic (${pcfiledir}/../.. from installDir/lib/pkgconfig → installDir) is correct. The one thing worth confirming is the $$ escaping — see the inline note.
| // its verified flags while making the installed package relocatable. | ||
| pcPath := filepath.join(installDir, "lib", "pkgconfig", "atomic_ops.pc") | ||
| pc := string(os.readFile(pcPath)!) | ||
| pc = strings.replace(pc, "prefix="+installDir, "prefix=$${pcfiledir}/../..", 1) |
There was a problem hiding this comment.
Please verify the $$ in the installed atomic_ops.pc. XGo performs $NAME/${...} interpolation in strings (via Gop_Env), and $$ is the escape for a literal $ — so this is very likely intentional and correct, emitting prefix=${pcfiledir}/../... Since interpolation applies to string literals, dropping to a single $ here would break the file. Worth a quick cat of the installed .pc after build to confirm it contains a single ${pcfiledir} (not a literal $${pcfiledir} and not an eagerly-resolved absolute path). The path math itself — installDir/lib/pkgconfig + /../.. → installDir — is correct.
|
|
||
| onBuild ctx => { | ||
| installDir := ctx.outputDir | ||
| shared := target.options["shared"][0] == "ON" |
There was a problem hiding this comment.
onBuild/onTest index target.options["..."][0] unconditionally (lines 90-93, 150), but filter (74-86) only rejects unknown option names and non-ON/OFF values — it doesn't require each recognized option to carry a value. If a target supplies a recognized option with an empty value list, filter still returns true and this [0] indexes into an empty slice. defaults covers the common case, but filter is what guards non-default targets. Consider validating a single non-empty value per option in filter, or use membership like cglm's slices.contains(target.options["shared"], "ON") instead of positional [0].
| c.build | ||
|
|
||
| if target.options["shared"][0] == "ON" { | ||
| os.setenv("LD_LIBRARY_PATH", filepath.join(installDir, "lib"))! |
There was a problem hiding this comment.
These overwrite LD_LIBRARY_PATH/DYLD_LIBRARY_PATH wholesale rather than prepending to any existing value. Harmless for the current single-dependency test (and shrinking the path is safe, not a hijack risk), but prepending (installDir/lib + separator + the existing value) is the more robust idiom if this test ever needs additional runtime library paths.
1 participant
Summary
bdwgc/libatomic_opsFormula from CCI snapshotffe30dfv7.8.0, includingv7.10.0atomic_ops.pcmetadata and an independent C consumerValidation
/tmp/llar-b416 test --verbose ./bdwgc/libatomic_ops@v7.10.0/tmp/llar-b416 test --verbose ./bdwgc/libatomic_ops@v7.8.0