Separate authors and copyright holders #5504

porsche-rbieniek · 2022-06-29T01:53:09Z

In German law, the author and the copyright holder can be two seperate
legal entities and therefore also need to be treated separately.

Introduce a new copyright holder field that is now the primary source
for copyright holder information. Authors are still only used as
copyright holders if the addAuthorsToCopyrights option is enabled.

For now, all package manager implementations set empty copyright
holders. Filling the copyright holder field is left as an exercise for
future actions. Right now, the only way to add copyright holders is via
curations.

This change resolves #4519.

Signed-off-by: Rainer Bieniek [email protected]

codecov · 2022-06-29T07:04:36Z

Codecov Report

Merging #5504 (455579c) into main (285f018) will decrease coverage by 3.00%.
The diff coverage is 66.66%.

❗ Current head 455579c differs from pull request most recent head 86672b1. Consider uploading reports for the commit 86672b1 to get more accurate results

@@             Coverage Diff              @@
##               main    #5504      +/-   ##
============================================
- Coverage     72.70%   69.70%   -3.01%     
- Complexity     2045     2192     +147     
============================================
  Files           267      268       +1     
  Lines         14124    14897     +773     
  Branches       2109     2412     +303     
============================================
+ Hits          10269    10384     +115     
- Misses         2773     3427     +654     
- Partials       1082     1086       +4

Flag	Coverage Δ
funTest-analyzer-docker	`73.43% <94.59%> (+1.96%)`	⬆️
funTest-non-analyzer	`47.09% <0.00%> (-0.15%)`	⬇️
test	`32.60% <12.28%> (-0.07%)`	⬇️

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

Impacted Files	Coverage Δ
...el/src/main/kotlin/licenses/LicenseInfoResolver.kt	`84.93% <0.00%> (-6.62%)`	⬇️
...rc/main/kotlin/experimental/ExperimentalScanner.kt	`53.16% <0.00%> (-0.31%)`	⬇️
analyzer/src/main/kotlin/managers/Pip.kt	`55.27% <50.00%> (-16.16%)`	⬇️
...main/kotlin/licenses/DefaultLicenseInfoProvider.kt	`76.66% <66.66%> (-0.53%)`	⬇️
analyzer/src/main/kotlin/managers/Carthage.kt	`56.52% <75.00%> (-17.43%)`	⬇️
analyzer/src/main/kotlin/managers/Bower.kt	`67.76% <100.00%> (-20.07%)`	⬇️
analyzer/src/main/kotlin/managers/Bundler.kt	`56.64% <100.00%> (-15.59%)`	⬇️
analyzer/src/main/kotlin/managers/Cargo.kt	`73.80% <100.00%> (-13.34%)`	⬇️
analyzer/src/main/kotlin/managers/CocoaPods.kt	`76.38% <100.00%> (-16.72%)`	⬇️
analyzer/src/main/kotlin/managers/Composer.kt	`66.48% <100.00%> (-10.59%)`	⬇️
... and 37 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 285f018...86672b1. Read the comment docs.

sschuberth · 2022-06-29T07:15:03Z

@porsche-rbieniek please have a look at the detekt issues.

sschuberth · 2022-06-29T08:30:31Z

@porsche-rbieniek now the DCO checks fails because of a mismatch between the author and sign-off names:

porsche-rbieniek · 2022-06-29T08:42:46Z

So many small details to get it right. I was really wondering why my sign-off's were failing

sschuberth · 2022-06-30T08:15:27Z

@porsche-rbieniek Something's got stuck with the GitHub actions apparently... please rebase once more onto origin/main and force-push to retrigger the checks.

sschuberth · 2022-06-30T08:16:38Z

... and please add "Resolves #4519." to the commit message, too.

analyzer/src/test/kotlin/managers/CarthageTest.kt

analyzer/src/main/kotlin/managers/GoMod.kt

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

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

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

porsche-rbieniek · 2022-06-30T11:54:12Z

Changes implemented except CarthageTest. Here I would like to keep the test cverage and augment it by testing the authors as well.

porsche-rbieniek · 2022-07-04T08:54:46Z

Hi guys,

could you please gimme a hint why this seems to be stuck now?

IMHO I implemented the changes requested to move forward

scanner/src/main/kotlin/Scanner.kt

analyzer/src/main/kotlin/managers/Bower.kt

model/src/main/kotlin/PackageCurationData.kt

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

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

scanner/src/main/kotlin/Scanner.kt

fviernau · 2022-07-12T12:19:14Z

@porsche-rbieniek will you make the changes according to the comments so that we can move it forward?

porsche-rbieniek · 2022-07-12T17:08:32Z

Done all requested changes, we can proceed

model/src/main/kotlin/PackageCurationData.kt

scanner/src/main/kotlin/Scanner.kt

In German law, the author and the copyright holder can be two seperate legal entities and therefore also need to be treated separately. Introduce a new copyright holder field that is now the primary source for copyright holder information. Authors are still only used as copyright holders if the `addAuthorsToCopyrights` option is enabled. For now, all package manager implementations set empty copyright holders. Filling the copyright holder field is left as an exercise for future actions. Right now, the only way to add copyright holders is via curations. This change resolves oss-review-toolkit#4519. Signed-off-by: Rainer Bieniek <[email protected]>

Dismissing approval after July 14th ORT developer meeting discussion, additional changes to this PR are needed.

sschuberth · 2022-07-14T12:53:46Z

Dismissing approval after July 14th ORT developer meeting discussion, additional changes to this PR are needed.

While I couldn't join today's developer meeting, I believe we shouldn't try to do too many things at once. IMO the intermediate step, as done in this PR, should be to introduce copyrightHolders analog to authors, except that copyrightHolders for now can exclusively set by curations. All other changes could be done incrementally on top of that; it has already taken us too much time to get this PR into its current shape.

sschuberth · 2022-07-14T12:58:42Z

model/src/main/kotlin/PackageCurationData.kt

@@ -167,13 +175,16 @@ data class PackageCurationData(
 * Merge with [other] curation data. The `comment` properties are joined but probably need to be adjusted before
 * further processing as their meaning might have been distorted by the merge. For properties that cannot be merged,
 * data in this instance has precedence over data in the other instance.
+ *
+ * TODO: Clarify semantics if setting an author os copyright holders replaces or augments any scan result value


"TODO: Clarify semantics about whether setting an author or copyright holders should replace or amend results from a scan."

fviernau · 2022-07-14T13:10:24Z

While I couldn't join today's developer meeting, I believe we shouldn't try to do too many things at once. IMO the intermediate step, as done in this PR, should be to introduce copyrightHolders analog to authors, except that copyrightHolders for now can exclusively set by curations. All other changes could be done incrementally on top of that; it has already taken us too much time to get this PR into its current shape.

As far as I understand this statement tries to argue against the changes agreed on in the developer meeting.
Maybe it's worth mentioning these here, so that we are sure to be on the same page. Do you know which these are @sschuberth ?

sschuberth · 2022-07-14T13:16:34Z

Do you know which these are @sschuberth ?

I didn't yet have time to read through the meeting minutes, if that's what you mean. It seems like use-cases I've never heard of before popped up, and we're back to square one 😵

fviernau · 2022-07-14T13:28:58Z

Do you know which these are @sschuberth ?

I didn't yet have time to read through the meeting minutes, if that's what you mean. It seems like use-cases I've never heard of before popped up, and we're back to square one dizzy_face

That's a misunderstanding on your side then. The agreement today was that only one trivial change needs to be added to the PR, which is renaming the introduced property IIRC from copyrightHolders to something like declaredCopyrights. Not sure about the exact word, but it basically should align with the name of declaredLicenses.

porsche-rishisaxena · 2022-07-15T17:39:11Z

Hi @fviernau

I wanted to clarify on the following:

There was no agreement in the developers meeting. There was an expectation set from the reviewers in this case i.e., you and Thomas who proposed by enhancing the implementation with following cases:
1.1 Implement an ability to add and remove the copyright (c)
1.2 Implement separation of copyright (c) with Authors in the same way as license is being handled
1.3 If the reporter provides copyright (c) for example: "a","b","c" and manually "d" has been identified by the analyst, the curation should only consist of "d" and not "a","b","c", and "d"
Thomas requested to change the property name from copyright_holders to declared_copyright_holders and he proposed to change it by himself before merging.
@mnonnenmacher even proposed to take this pull request to be merged and later we bring the enhancements after creating an issue in the backlog.
Next Steps: we all aligned that we need a separate workshop to brain-storm to have Sebastian, Thomas, Rainer, Frank, Martin and myself to discuss and conclude. Thomas to set up this meeting.

I hope this clarifies.

Regards,
Rishi Saxena

CC: @porsche-rbieniek @sschuberth @tsteenbe @nikpete

fviernau · 2022-07-18T06:57:04Z

Hi @fviernau

I wanted to clarify on the following:

1. There was no agreement in the developers meeting. There was an expectation set from the reviewers in this case i.e., you and Thomas who proposed by enhancing the implementation with following cases:
   1.1 Implement an ability to add and remove the copyright (c)
   1.2 Implement separation of copyright (c) with Authors in the same way as license is being handled
   1.3 If the reporter provides copyright (c) for example: "a","b","c" and manually "d" has been identified by the analyst, the curation should only consist of "d" and not "a","b","c", and "d"

2. Thomas requested to change the property name from copyright_holders to declared_copyright_holders and he proposed to change it by himself before merging.

3. @mnonnenmacher even proposed to take this pull request to be merged and later we bring the enhancements after creating an issue in the backlog.

4. Next Steps: we all aligned that we need a separate workshop to brain-storm to have Sebastian, Thomas, Rainer, Frank, Martin and myself to discuss and conclude. Thomas to set up this meeting.

I hope this clarifies.

Regards, Rishi Saxena

CC: @porsche-rbieniek @sschuberth @tsteenbe @nikpete

@porsche-rishisaxena

Hm, so it seems things have been perceived differently, e.g. in your view "There was no agreement in the developers meeting" . In my view, e.g. @mnonnenmacher requested to do #2 as part of the PR, which would contradict your point #2 and #3.
To my understanding #1 and #4 was discussed as future improvements (and out of scope of the PR), e.g. we've discussed on a conceptual level how we envision the configuration to work in the future.

Anyhow, what problem are you trying to solve @porsche-rishisaxena? Find an agreement how to move on with this PR?

porsche-rbieniek · 2022-07-18T08:08:10Z

Could somebody please explain if there are now further changes required by me or not?

I couildn't join the developer meeting due to a vacation on short notice and I am now totally lost in this discussion

sschuberth · 2022-07-18T08:42:26Z

Could somebody please explain if there are now further changes required by me or not?

@porsche-rbieniek, let me take over this PR of yours, I'll get it merged. So please don't push to the branch anymore.

tsteenbe · 2022-07-18T09:02:54Z

@sschuberth Please don't merge it without my review, I was planning to update this PR today to be aligned with what was discussed in the ORT meeting.

mnonnenmacher · 2022-07-19T10:49:26Z

@sschuberth and @tsteenbe, since your are taking over the PR, to me the most important point is consistency in the handling of copyrights and licenses. Both can be declared in the metadata and detected in the source code, so the properties should use similar naming and the curation options should work in a similar way.

Similar to declaredLicense I would prefer the name declaredCopyrights over copyrightHolders in Package and Project.
I prefer copyrights over copyrightHolders because the detected values are statements like "(C) 2020 xyz", but "holder" sounds to me like it should only be "xyz" for this example. And likely the values in the metadata are also similar in structure as the values we get from ScanCode. See for example "© Microsoft Corporation. All rights reserved." here: https://api.nuget.org/v3/catalog0/data/2018.12.13.22.08.00/system.globalization.4.3.0.json (Btw, who will actually do the work and go through all package managers and check which have dedicated fields for copyright information?)
I don't know if it's currently possible to add declared licenses with a curation, but if this is implemented for copyrights it should work for licenses and copyrights in a similar way.

tsteenbe · 2022-07-21T07:58:31Z

@mnonnenmacher I looked up if the most used package manager have a declared copyright holder filed and they don't.

Cargo https://doc.rust-lang.org/cargo/reference/manifest.html
- authors — The authors of the package.
- license — The package license.
- license-file — Path to the text of the license.
Composer: https://getcomposer.org/doc/04-schema.md
- license
- authors
Maven: https://maven.apache.org/pom.html#More_Project_Information
- license
- organization
- developer
- contributor
NPM:
- license: SPDX license id https://docs.npmjs.com/cli/v8/configuring-npm/package-json#license
- author: one person https://docs.npmjs.com/cli/v8/configuring-npm/package-json#people-fields-author-contributors
- contributor: array of people
Python: https://docs.python.org/3/distutils/setupscript.html#additional-meta-data
- author
- author_email
- maintainer
- maintainer_email
- license

sschuberth · 2022-08-24T11:20:11Z

Superseded by #5680.

In German law, the author and the copyright holder can be two separate legal entities and therefore also need to be tracked separately. Introduce a new field for declared copyrights that is now the primary source for copyright holder information. Authors are still only used as copyright holders if the `addAuthorsToCopyrights` option is enabled. For now, all package manager implementations set empty declared copyrights. Filling the declared copyrights is left as a future exercise (also see [1]). Currently, the only way to add declared copyrights is via curations. This change resolves #4519. [1]: #5504 (comment) Signed-off-by: Rainer Bieniek <[email protected]> Signed-off-by: Sebastian Schuberth <[email protected]>

Similar to the `concludedLicense` field, introduce a `concludedCopyrights` field that is to be set exclusively via a package curation [1] to override any detected copyright statements. Note that there is no such thing as *declared* copyright statements because package managers do not support them explicitly [1]. The concluded copyrights will get associated to all effective licenses; there currently is no way to curate a copyright statement for a specific license only. Behavior-wise this is no change compared to previous feature of curating authors and enabling the `addAuthorsToCopyrights` option. As a bonus, this fixes a subtle bug where previously packages might have been skipped in the scan if `authors` were set but `addAuthorsToCopyrights` was disabled. Resolves #4519. [1]: #5504 (comment) [1]: https://github.com/oss-review-toolkit/ort/blob/main/docs/config-file-curations-yml.md Signed-off-by: Rainer Bieniek <[email protected]> Signed-off-by: Sebastian Schuberth <[email protected]>

porsche-rbieniek requested a review from a team as a code owner June 29, 2022 01:53

rbieniek force-pushed the feature/enhance-model-by-copyright-holder branch from 49b52ec to c473180 Compare June 29, 2022 02:24

porsche-rbieniek mentioned this pull request Jun 29, 2022

Seperate authorship and copyright ownership in the curation model #5315

Closed

porsche-rbieniek force-pushed the feature/enhance-model-by-copyright-holder branch 4 times, most recently from 94e418c to 2429ef2 Compare June 29, 2022 09:45

sschuberth changed the title ~~Separates authorship and the copyright holder~~ Separate authors and copyright holders Jun 29, 2022

porsche-rbieniek force-pushed the feature/enhance-model-by-copyright-holder branch 4 times, most recently from 6af3b1c to 9ec9f53 Compare June 30, 2022 08:02

porsche-rbieniek force-pushed the feature/enhance-model-by-copyright-holder branch 2 times, most recently from d2718ff to a896ef5 Compare June 30, 2022 08:44

sschuberth requested changes Jun 30, 2022

View reviewed changes

porsche-rbieniek force-pushed the feature/enhance-model-by-copyright-holder branch from a896ef5 to cbb331e Compare June 30, 2022 11:52

This was referenced Jul 4, 2022

model: Add resolved copyright source #5517

Open

reporter: Add volkswagen group custom reports #5518

Open

tsteenbe requested review from sschuberth, fviernau and tsteenbe July 7, 2022 08:26

MarcelBochtler added the release notes Changes that should be mentioned in release notes label Jul 7, 2022

fviernau requested changes Jul 7, 2022

View reviewed changes

porsche-rbieniek force-pushed the feature/enhance-model-by-copyright-holder branch from cbb331e to 7632134 Compare July 12, 2022 17:06

fviernau requested changes Jul 13, 2022

View reviewed changes

model/src/main/kotlin/PackageCurationData.kt Outdated Show resolved Hide resolved

model/src/main/kotlin/PackageCurationData.kt Outdated Show resolved Hide resolved

scanner/src/main/kotlin/Scanner.kt Outdated Show resolved Hide resolved

porsche-rbieniek force-pushed the feature/enhance-model-by-copyright-holder branch from 7632134 to 86672b1 Compare July 13, 2022 10:26

fviernau previously approved these changes Jul 13, 2022

View reviewed changes

sschuberth reviewed Jul 14, 2022

View reviewed changes

sschuberth closed this Aug 24, 2022

fviernau mentioned this pull request Aug 24, 2022

model: Introduce a concludedCopyrights field to package curations #5680

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate authors and copyright holders #5504

Separate authors and copyright holders #5504

porsche-rbieniek commented Jun 29, 2022 •

edited by sschuberth

Loading

codecov bot commented Jun 29, 2022 •

edited

Loading

sschuberth commented Jun 29, 2022

sschuberth commented Jun 29, 2022

porsche-rbieniek commented Jun 29, 2022

sschuberth commented Jun 30, 2022

sschuberth commented Jun 30, 2022

porsche-rbieniek commented Jun 30, 2022

porsche-rbieniek commented Jul 4, 2022

fviernau commented Jul 12, 2022

porsche-rbieniek commented Jul 12, 2022

sschuberth commented Jul 14, 2022

sschuberth Jul 14, 2022

fviernau commented Jul 14, 2022

sschuberth commented Jul 14, 2022

fviernau commented Jul 14, 2022

porsche-rishisaxena commented Jul 15, 2022

fviernau commented Jul 18, 2022

porsche-rbieniek commented Jul 18, 2022

sschuberth commented Jul 18, 2022

tsteenbe commented Jul 18, 2022 •

edited

Loading

mnonnenmacher commented Jul 19, 2022

tsteenbe commented Jul 21, 2022 •

edited

Loading

sschuberth commented Aug 24, 2022

Separate authors and copyright holders #5504

Separate authors and copyright holders #5504

Conversation

porsche-rbieniek commented Jun 29, 2022 • edited by sschuberth Loading

codecov bot commented Jun 29, 2022 • edited Loading

Codecov Report

sschuberth commented Jun 29, 2022

sschuberth commented Jun 29, 2022

porsche-rbieniek commented Jun 29, 2022

sschuberth commented Jun 30, 2022

sschuberth commented Jun 30, 2022

porsche-rbieniek commented Jun 30, 2022

porsche-rbieniek commented Jul 4, 2022

fviernau commented Jul 12, 2022

porsche-rbieniek commented Jul 12, 2022

sschuberth commented Jul 14, 2022

sschuberth Jul 14, 2022

Choose a reason for hiding this comment

fviernau commented Jul 14, 2022

sschuberth commented Jul 14, 2022

fviernau commented Jul 14, 2022

porsche-rishisaxena commented Jul 15, 2022

fviernau commented Jul 18, 2022

porsche-rbieniek commented Jul 18, 2022

sschuberth commented Jul 18, 2022

tsteenbe commented Jul 18, 2022 • edited Loading

mnonnenmacher commented Jul 19, 2022

tsteenbe commented Jul 21, 2022 • edited Loading

sschuberth commented Aug 24, 2022

porsche-rbieniek commented Jun 29, 2022 •

edited by sschuberth

Loading

codecov bot commented Jun 29, 2022 •

edited

Loading

tsteenbe commented Jul 18, 2022 •

edited

Loading

tsteenbe commented Jul 21, 2022 •

edited

Loading