feat(java): read Jenkins plugin manifest licenses#10939
Conversation
DmitriyLewen
left a comment
There was a problem hiding this comment.
Hello
Thanks for your work!
I left a few comments.
Also, you need to update the docs and sign the CLA.
| // Classify and attach the LICENSE file now that the jar's own artifact is resolved | ||
| // (it may have been added above from MANIFEST.MF / SHA-1 / file name). |
There was a problem hiding this comment.
This comment isn't related to the new attachManifestLicenses().
|
|
||
| // Classify and attach the LICENSE file now that the jar's own artifact is resolved | ||
| // (it may have been added above from MANIFEST.MF / SHA-1 / file name). | ||
| attachManifestLicenses(pkgs, fileProps.FilePath, m.licenses) |
There was a problem hiding this comment.
It makes sense to add a comment about the priority.
Classifying a license from a file is an expensive operation, so we first try to detect licenses from the manifest file.
|
|
||
| // attachManifestLicenses attaches Jenkins plugin licenses declared in MANIFEST.MF | ||
| // to the jar's own package when a single unambiguous package belongs to this jar. | ||
| func attachManifestLicenses(pkgs []ftypes.Package, filePath string, licenses []string) { |
There was a problem hiding this comment.
A lot of duplicate code.
You can create a common function for attachFileLicenses() and attachManifestLicenses().
something like that:
// ownJarPackage returns the single package that belongs to filePath and still
// needs a license, or nil when there is none, the owner already has a license,
// or the owner is ambiguous (multiple packages share the jar's file path, e.g.
// an uber jar).
func ownJarPackage(pkgs []ftypes.Package, filePath string) *ftypes.Package {
var pkg *ftypes.Package
for i := range pkgs {
if pkgs[i].FilePath != filePath {
continue
}
if pkg != nil {
return nil // more than one package belongs to this jar
}
pkg = &pkgs[i]
}
if pkg == nil || len(pkg.Licenses) > 0 {
return nil
}
return pkg
}| m.bundleName = strings.TrimPrefix(line, "Bundle-Name:") | ||
| case strings.HasPrefix(line, "Bundle-SymbolicName:"): | ||
| m.bundleSymbolicName = strings.TrimPrefix(line, "Bundle-SymbolicName:") | ||
| default: |
There was a problem hiding this comment.
Add a new case for Plugin-License-Name (don't use the default here).
| for _, r := range suffix { | ||
| if r < '0' || r > '9' { | ||
| return false | ||
| } |
There was a problem hiding this comment.
I don't think there's any chance that there won't be a number here. Let's simplify the check. And if users do report such a case, we'll add it in a separate PR.
| key, value, ok := strings.Cut(line, ":") | ||
| if !ok || !isPluginLicenseNameKey(key) { | ||
| continue | ||
| } | ||
| if name := strings.TrimSpace(value); name != "" { | ||
| m.licenses = append(m.licenses, name) | ||
| } |
There was a problem hiding this comment.
I'd create a separate function for getting the license, and inside it do the Plugin-License-Name- check, trim the Plugin-License-Name* prefix, and return the licenses. If no licenses are found, return nil.
e.g. m.licenses = parseManifestLicenses(line)
| } | ||
| } | ||
|
|
||
| func newZipReader(t *testing.T, entries map[string]string) *bytes.Reader { |
There was a problem hiding this comment.
I don't think we need a separate test that builds a jar file. Let's add one test case to TestParse. And also create a new test function for parseManifestLicenses() (use the export_test.go file).
|
Thanks for the review. I’ll clean up the manifest parsing, reuse the package lookup logic, move the coverage into the existing parser tests, and update the docs. |
|
Updated this now. I cleaned up the manifest license parsing, reused the jar package lookup, moved the parser coverage into the existing test table, added focused tests for parseManifestLicenses, and updated the Java license docs.\n\nLocal check: go test ./pkg/dependency/parser/java/... -count=1\n\nPlease take another look when you have time. |
|
@DmitriyLewen thanks again for the clear review. I pushed the updates, and the CLA check is green now too. Happy to adjust anything else if needed. I’d be glad to keep contributing to Trivy, and would appreciate another look when you have time. |
DmitriyLewen
left a comment
There was a problem hiding this comment.
Thanks for your work!
I refactored a bit.
Can you recheck?
|
I rechecked the refactor and it looks good to me. I also ran go test ./pkg/dependency/parser/java/... -count=1 locally. |
Summary
Plugin-License-Namemanifest attributes as a JAR license fallbackPlugin-License-Name-2pom.xmllicenses win before manifest licenses, and packedLICENSEfiles remain the final fallbackCloses #10936
Validation
go test ./pkg/dependency/parser/java/jar -run TestParseJenkinsPluginManifestLicenses -count=1go test ./pkg/dependency/parser/java/jar -count=1go test ./pkg/dependency/parser/java/... -count=1git diff --check HEAD~1..HEAD