Repository navigation
fix: Include optional dependencies in Arch packages - #1131
Conversation
pull-request-size
Bot
added
the
size/S
caarlos0
left a comment
caarlos0
left a comment
There was a problem hiding this comment.
Approving. The mapping of recommends + suggests to optdepend is correct for .PKGINFO, it follows the existing loop pattern in createPkginfo, and it adds no configuration surface. The two comments below are non-blocking.
Acceptance coverage (no single site in this diff)
testdata/acceptance/core.complex.yaml already sets recommends: fish and suggests: zsh, and the complex stage of testdata/acceptance/archlinux.dockerfile already greps pacman -Qi for Depends On, Replaces and Provides. Nothing there asserts the new field, so the unit test is the only proof. One more line in that stage makes pacman itself prove that it parses the new entry:
RUN pacman -Qi foo | grep "Optional Deps\\s*: fish"I verified that no acceptance stage does strict output matching, so this change breaks no current stage.
Verification gaps
- I did not build or run tests locally, as requested.
exampleInfo()is shared by 12 tests inarch/arch_test.go, sogo test ./arch/...is the check that matters. - I did not run the independent
caarlos0andanvilsub-agent checks. The diff is 7 lines and fits one direct read.
This review is from an automated (bot) reviewer.
| } | ||
| } | ||
|
|
||
| for _, optdepend := range append(info.Recommends, info.Suggests...) { |
There was a problem hiding this comment.
Nit, non-blocking: append(info.Recommends, ...) writes into the caller's backing array when cap > len. This can occur, because expandEnvVarsStringSlice uses slices.Delete, which keeps the capacity after it removes an empty entry (recommends: [git, "${UNSET}"] gives len 1, cap 2).
I could not build a repro that a user can observe: all writes land past len, and no code in this repository reads past len of Recommends. So this is style, not a bug. slices.Concat removes the question:
for _, optdepend := range slices.Concat(info.Recommends, info.Suggests) {No new test is necessary.
| require.Contains(t, string(pkginfoData), "optdepend = git\n") | ||
| require.Contains(t, string(pkginfoData), "optdepend = fish\n") |
There was a problem hiding this comment.
These two assertions read the raw bytes, because extractPkginfoFields (line 248) puts each key in a map[string]string and thus keeps only the last value of a repeated key. Result: no test can find a dropped, duplicated, or wrongly repeated optdepend, and this is the first field in .PKGINFO that occurs more than one time in the example data.
A small helper keeps the assertion at the same layer as the others and makes the count part of the contract:
func extractPkginfoValues(data []byte, key string) []string {
var out []string
for _, line := range strings.Split(strings.TrimSpace(string(data)), "\n") {
if k, v, ok := strings.Cut(line, " = "); ok && k == key {
out = append(out, v)
}
}
return out
}Then:
require.ElementsMatch(t, []string{"git", "fish"}, extractPkginfoValues(pkginfoData, "optdepend"))ElementsMatch prints both slices on failure and does not make the line order a contract.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1131 +/- ##
==========================================
- Coverage 83.85% 83.82% -0.03%
==========================================
Files 30 30
Lines 2787 2789 +2
==========================================
+ Hits 2337 2338 +1
- Misses 450 451 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Serialize recommends and suggests as Arch optdepend metadata so optional dependencies are retained in generated packages. See goreleaser#1123
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_018mQoXuBtWLvs8N7nSYWNKz
474ad78 to
1c37fbc
Compare
|
Thanks for the review. Added the acceptance assertion you suggested to the RUN pacman -Qi foo | grep "Optional Deps\\s*: fish"
Also rebased on current
|
|
Applied both non-blocking suggestions in a040b70: |
caarlos0
left a comment
caarlos0
left a comment
There was a problem hiding this comment.
No findings. Approving the reviewed head.
The complete merge-base-to-head diff received independent read-only caarlos0 maintainer and anvil adversarial checks; both reported no findings, independently checked against the code and Arch PKGINFO specification.
Verification gaps: No local builds, tests, or Docker acceptance runs were performed. git diff --check passed. Exact-head product workflows report action_required; only WIP reports success. Unit and Arch acceptance results remain unverified and should pass before merge.
This supersedes automated review 5149516868.
This review is from an automated (bot) reviewer.
Superseded by automated full-diff review 5460189475 of a040b70; previous non-blocking suggestions are addressed.
Address the review nits: avoid writing into the Recommends backing array, and assert the full list of optdepend values instead of substrings, since extractPkginfoFields keeps only the last value of a repeated key. Signed-off-by: amirahrari <[email protected]>
a040b70 to
b1c3749
Compare
Co-authored-by: Copilot <[email protected]> Copilot-Session: cb68ab4e-96d3-4658-a1c0-c2c33e8c52a0 Signed-off-by: Carlos Alexandro Becker <[email protected]>
What changed
Arch packages now include
recommendsandsuggestsentries asoptdependmetadata in.PKGINFO.Verification
go test ./...go build ./cmd/nfpmgo vet ./...git diff --checkCloses #1123