fix(reporter): Add authors to concluded license in disclosure document #9600

wkl3nk · 2024-12-12T17:42:35Z

Include authors for package dependencies when both concluded_license and authors are curated, and ORT is configured with the addAuthorsToCopyrights option enabled. This ensures that package authors appear under the respective concluded license in the Disclosure Document.

This behaviour applies when the Scanner option skipConcluded is enabled, having the effect that the scan stage is skipped for the particular package dependency in this case, and if either the concluded license is different from the declared license or if no license is declared at all.

Fixes #9599.

codecov · 2024-12-12T18:02:53Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.09%. Comparing base (81f58ea) to head (e0f75f4).
Report is 15 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #9600      +/-   ##
============================================
+ Coverage     68.05%   68.09%   +0.04%     
- Complexity     1285     1293       +8     
============================================
  Files           249      249              
  Lines          8835     8840       +5     
  Branches        921      922       +1     
============================================
+ Hits           6013     6020       +7     
+ Misses         2433     2432       -1     
+ Partials        389      388       -1

Flag	Coverage Δ
funTest-docker	`65.14% <ø> (ø)`
funTest-non-docker	`33.28% <0.00%> (-0.03%)`	⬇️
test-ubuntu-24.04	`35.90% <100.00%> (+0.05%)`	⬆️
test-windows-2022	`35.88% <100.00%> (+0.05%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

model/src/main/kotlin/licenses/LicenseInfoResolver.kt

mnonnenmacher

Except for the one comment this change looks good to me. @sschuberth Do you also want to have another look?

model/src/test/kotlin/licenses/LicenseInfoResolverTest.kt

model/src/test/kotlin/licenses/TestData.kt

model/src/test/kotlin/licenses/LicenseInfoResolverTest.kt

sschuberth · 2024-12-17T12:28:43Z

model/src/main/kotlin/licenses/LicenseInfoResolver.kt

@@ -83,6 +83,10 @@ class LicenseInfoResolver(
                licenseInfo.concludedLicenseInfo.concludedLicense?.also {
                    originalExpressions += ResolvedOriginalExpression(expression = it, source = LicenseSource.CONCLUDED)
                }
+
+                licenseInfo.declaredLicenseInfo.authors.takeIf { it.isNotEmpty() && addAuthorsToCopyrights }?.also {
+                    locations += resolveCopyrightFromAuthors(it)


TBH, I still don't understand the details why this is necessary. Can it be that the real reason is not related to having a concluded license, but to not having a declared license, and so the below logic does not kick in?

Or is it more like:

Copyrights / authors are associated with specific licenses.

A concluded license trumps all other licenses.

If both a concluded license and a declared license are present, but only the declared license has a Copyright / author associated, that will not be used as the concluded license it taken instead of the declared license.

As @mnonnenmacher just explained to me, it's probably a mixture of both of the above: The problem occurs if there is a concluded license that is not also present as a declared license (which is esp. the case if there are no declared licenses).

It would be great to have this nailed down in the commit message as well, and / or in the test titles.

Yes, I guess it happens if a.) declared and concluded license are not the same (overwritten in curation) or there is no declared license at all.

Hmm, you "guess" what you're fixing here? 😉 Us all having some more confidence in understanding what exact is being fixed would be great.

To document that, I propose a more concise commit message like the following (which also drops the explicit mention of the disclosure document, as the fix is not specific to that, and the fix is in the model, not in the reporter):

fix(model): Apply author to copyright mapping also for concluded licenses As concluded licenses are supposed to trump other licenses, they are usually taken instead of declared and detected licenses in reports. When doing do, and the concluded license is not among the other licenses, any author information that is configured to be mapped to copyright would be dropped. To prevent that, apply the same author to copyright mapping that is done for declared licenses also for concluded licenses.

Refactor the code to improve reusability and maintainability by extracting the functionality for resolving copyright statements from package authors into a separate function. The extracted function encapsulates the existing logic, making it easier to call from multiple places while reducing duplication. This change simplifies future enhancements or modifications to the copyright resolution process and ensures a clearer separation of concerns. The refactor does not introduce any changes to the existing behavior and serves purely as a structural improvement. Signed-off-by: Wolfgang Klenk <[email protected]>

Include authors for package dependencies when both `concluded_license` and `authors` are curated, and ORT is configured with the `addAuthorsToCopyrights` option enabled. This ensures that package authors appear under the respective concluded license in the Disclosure Document. This behavior applies when the Scanner option `skipConcluded` is enabled, having the effect that the scan stage is skipped for the particular package dependency in this case. Fixes oss-review-toolkit#9599. Signed-off-by: Wolfgang Klenk <[email protected]>

Verify that package authors defined in a curation file are stated under the respective concluded license in the Disclosure Document if ORT option `addAuthorsToCopyrights` is enabled. Signed-off-by: Wolfgang Klenk <[email protected]>

sschuberth · 2024-12-18T10:51:17Z

model/src/main/kotlin/licenses/LicenseInfoResolver.kt

-                            )
-                        }
-                    )
+                    locations += resolveCopyrightFromAuthors(it)


Just for the record: To me, a simple "Extract existing code without modifications to a function for later reuse." would have been enough as the commit message body. But having more explanation here cannot hurt, I guess 😅

sschuberth · 2024-12-18T11:01:56Z

model/src/main/kotlin/licenses/LicenseInfoResolver.kt

@@ -83,6 +83,10 @@ class LicenseInfoResolver(
                licenseInfo.concludedLicenseInfo.concludedLicense?.also {
                    originalExpressions += ResolvedOriginalExpression(expression = it, source = LicenseSource.CONCLUDED)
                }
+
+                licenseInfo.declaredLicenseInfo.authors.takeIf { it.isNotEmpty() && addAuthorsToCopyrights }?.also {
+                    locations += resolveCopyrightFromAuthors(it)


Hmm, you "guess" what you're fixing here? 😉 Us all having some more confidence in understanding what exact is being fixed would be great.

To document that, I propose a more concise commit message like the following (which also drops the explicit mention of the disclosure document, as the fix is not specific to that, and the fix is in the model, not in the reporter):

fix(model): Apply author to copyright mapping also for concluded licenses As concluded licenses are supposed to trump other licenses, they are usually taken instead of declared and detected licenses in reports. When doing do, and the concluded license is not among the other licenses, any author information that is configured to be mapped to copyright would be dropped. To prevent that, apply the same author to copyright mapping that is done for declared licenses also for concluded licenses.

sschuberth · 2024-12-18T11:04:38Z

model/src/test/kotlin/licenses/LicenseInfoResolverTest.kt

@@ -589,6 +589,45 @@ class LicenseInfoResolverTest : WordSpec({
            result should containCopyrightStatementsForLicenseExactly("LicenseRef-a")
            result should containCopyrightStatementsForLicenseExactly("LicenseRef-b")
        }
+
+        "resolve copyrights from authors in concluded license" {


Also here, you're not testing something that's specific to disclosure documents, so your commit message should not mention that. Any our test is in the model, not the reporter.

PS: I liked the previous approach with disabled test being added beforehand better, but this can stay as-is now from my perspective.

wkl3nk requested a review from a team as a code owner December 12, 2024 17:42

This comment was marked as outdated.

Sign in to view

mnonnenmacher reviewed Dec 12, 2024

View reviewed changes

model/src/main/kotlin/licenses/LicenseInfoResolver.kt Show resolved Hide resolved

mnonnenmacher reviewed Dec 12, 2024

View reviewed changes

model/src/main/kotlin/licenses/LicenseInfoResolver.kt Outdated Show resolved Hide resolved

wkl3nk force-pushed the wkl3nk/add-authors-to-concluded-licenses branch 2 times, most recently from 75abb71 to 9d51625 Compare December 13, 2024 11:01

This comment was marked as outdated.

Sign in to view

wkl3nk force-pushed the wkl3nk/add-authors-to-concluded-licenses branch from 9d51625 to 1a4580f Compare December 13, 2024 13:49

wkl3nk requested a review from sschuberth December 13, 2024 13:52

wkl3nk force-pushed the wkl3nk/add-authors-to-concluded-licenses branch 2 times, most recently from 053ada6 to 3e6f6f4 Compare December 16, 2024 16:30

mnonnenmacher requested changes Dec 17, 2024

View reviewed changes

model/src/test/kotlin/licenses/LicenseInfoResolverTest.kt Show resolved Hide resolved

sschuberth reviewed Dec 17, 2024

View reviewed changes

wkl3nk added 2 commits December 18, 2024 11:07

wkl3nk force-pushed the wkl3nk/add-authors-to-concluded-licenses branch from 3e6f6f4 to 875975c Compare December 18, 2024 10:38

wkl3nk force-pushed the wkl3nk/add-authors-to-concluded-licenses branch from 875975c to e0f75f4 Compare December 18, 2024 10:44

wkl3nk requested review from sschuberth and mnonnenmacher December 18, 2024 10:53

sschuberth requested changes Dec 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(reporter): Add authors to concluded license in disclosure document #9600

fix(reporter): Add authors to concluded license in disclosure document #9600

wkl3nk commented Dec 12, 2024 •

edited

Loading

This comment was marked as outdated.

codecov bot commented Dec 12, 2024 •

edited

Loading

This comment was marked as outdated.

mnonnenmacher left a comment

sschuberth Dec 17, 2024

sschuberth Dec 17, 2024

sschuberth Dec 17, 2024 •

edited

Loading

wkl3nk Dec 18, 2024

sschuberth Dec 18, 2024

sschuberth Dec 18, 2024

sschuberth Dec 18, 2024

sschuberth Dec 18, 2024

sschuberth Dec 18, 2024

fix(reporter): Add authors to concluded license in disclosure document #9600

Are you sure you want to change the base?

fix(reporter): Add authors to concluded license in disclosure document #9600

Conversation

wkl3nk commented Dec 12, 2024 • edited Loading

This comment was marked as outdated.

codecov bot commented Dec 12, 2024 • edited Loading

Codecov Report

This comment was marked as outdated.

mnonnenmacher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sschuberth Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wkl3nk commented Dec 12, 2024 •

edited

Loading

codecov bot commented Dec 12, 2024 •

edited

Loading

sschuberth Dec 17, 2024 •

edited

Loading