Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Yet more CopyrightStatementsProcessor improvements #6022

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sschuberth
Copy link
Member

Please have a look at the individual commit messages for the details.

@codecov
Copy link

codecov bot commented Nov 1, 2022

Codecov Report

Base: 65.48% // Head: 58.04% // Decreases project coverage by -7.44% ⚠️

Coverage data is based on head (f9ff3df) compared to base (aa942fa).
Patch coverage: 0.00% of modified lines in pull request are covered.

❗ Current head f9ff3df differs from pull request most recent head 71a2bcd. Consider uploading reports for the commit 71a2bcd to get more accurate results

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6022      +/-   ##
============================================
- Coverage     65.48%   58.04%   -7.45%     
- Complexity     2003     2013      +10     
============================================
  Files           323      324       +1     
  Lines         16576    18699    +2123     
  Branches       1976     3847    +1871     
============================================
- Hits          10855    10854       -1     
- Misses         4661     6784    +2123     
- Partials       1060     1061       +1     
Flag Coverage Δ
funTest-analyzer-docker 74.66% <ø> (ø)
funTest-non-analyzer 50.95% <ø> (-0.02%) ⬇️
test 27.53% <0.00%> (ø)

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

Impacted Files Coverage Δ
...li/src/main/kotlin/commands/ListLicensesCommand.kt 0.00% <0.00%> (ø)
analyzer/src/main/kotlin/managers/Pip.kt 7.94% <0.00%> (-56.50%) ⬇️
...rc/main/kotlin/managers/utils/SpdxDocumentCache.kt 60.00% <0.00%> (-40.00%) ⬇️
analyzer/src/main/kotlin/managers/Poetry.kt 58.33% <0.00%> (-35.01%) ⬇️
analyzer/src/main/kotlin/managers/NuGet.kt 60.71% <0.00%> (-33.74%) ⬇️
analyzer/src/main/kotlin/managers/Npm.kt 52.99% <0.00%> (-32.01%) ⬇️
analyzer/src/main/kotlin/managers/Yarn.kt 42.30% <0.00%> (-31.03%) ⬇️
...in/kotlin/managers/utils/MavenDependencyHandler.kt 70.00% <0.00%> (-30.00%) ⬇️
analyzer/src/main/kotlin/managers/DotNet.kt 61.53% <0.00%> (-29.03%) ⬇️
...otlin/curation/OrtConfigPackageCurationProvider.kt 67.64% <0.00%> (-28.19%) ⬇️
... and 29 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@sschuberth sschuberth force-pushed the csp-imps-4 branch 3 times, most recently from f9ff3df to 71a2bcd Compare November 4, 2022 07:36
@sschuberth sschuberth marked this pull request as ready for review November 4, 2022 07:37
@sschuberth sschuberth requested a review from a team as a code owner November 4, 2022 07:37
@fviernau
Copy link
Member

Should this be moved back to "draft" state, as it introduces copyright statements without holder into e.g. the notices?

@sschuberth sschuberth marked this pull request as draft November 10, 2022 10:30
@sschuberth

This comment was marked as outdated.

…processed

Previously, these were neither stored as processed nor as unprocessed.
For transparency, store these now as unprocessed, and add a test to
ensure that each statement is either processed or unprocessed.

Maintaining "stubs" without an owner, like "Copyright (c) 2006", keeps
ORT on the safe side with respect to some license obligation like the
BSD-3-Clause's "Redistributions in binary form must reproduce the above
copyright notice", which strictly speaking also applies to such "stubs".

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
Any code that compares `Parts` as thus makes use of the data class's
auto-generated `equals()` function should not take the list of
`originalStatements` into account. Avoid that pitfall by rewriting the
code to not require `originalStatements` to be stored within `Parts`.

This implicitly also aligns the semantics of `compareTo() == 0` with
`equals() == true`, which is important to avoid unexpected behavior when
storing `Parts` in a `SortedSet`.

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
@sschuberth
Copy link
Member Author

Should this be moved back to "draft" state, as it introduces copyright statements without holder into e.g. the notices?

@fviernau, I finally found the time to continue working on this. In the meantime, I had verified with @LeChasseur that, to be on the safe side, even Copyright "stubs" without a holder, like "Copyright (c) 2006", should be maintained to fully satisfy e.g. BSD-3-Clause's "Redistributions in binary form must reproduce the above copyright notice" (whatever it reads) obligation.

And the second commit also proves to be the right thing in the context of the recent compareTo() vs. equals() alignment discussions, IMO.

@sschuberth sschuberth marked this pull request as ready for review December 20, 2022 09:24
@fviernau
Copy link
Member

@fviernau, I finally found the time to continue working on this. In the meantime, I had verified with @LeChasseur that, to be on the safe side, even Copyright "stubs" without a holder, like "Copyright (c) 2006", should be maintained to fully satisfy e.g. BSD-3-Clause's "Redistributions in binary form must reproduce the above copyright notice" (whatever it reads) obligation.

And the second commit also proves to be the right thing in the context of the recent compareTo() vs. equals() alignment discussions, IMO.

I fear I won't find the time to do a review prior to my X-mas holidays. Is this urgent?

@sschuberth
Copy link
Member Author

I fear I won't find the time to do a review prior to my X-mas holidays. Is this urgent?

It's not super urgent. But it's also not a lot of code. If you agree with the basic idea, and trust the test coverage, then also anyone else could approve, if you're fine with that. I just wanted to give you a heads up.

@sschuberth sschuberth enabled auto-merge (rebase) December 20, 2022 22:39
@mnonnenmacher
Copy link
Member

Code looks good to me, but I wonder if the outcome of the change should be discussed in a community meeting first. I'd also like to discuss this internally at Bosch before the merge but that won't be possible before next year.

@fviernau
Copy link
Member

Needs to be discussed in EPAM as well after the holidays.

@sschuberth sschuberth added the on hold Pull requests that cannot currently be merged label Dec 22, 2022
@pombredanne
Copy link
Contributor

Is this something that could be fixed upstream in ScanCode instead?

@sschuberth
Copy link
Member Author

Is this something that could be fixed upstream in ScanCode instead?

I don't think so. ScanCode already does report Copyright statements without holders (if that's how they're declared in the source), but our post-processing so far removed these. So it's something we need to tackle in ORT.

@sschuberth sschuberth marked this pull request as draft January 6, 2023 10:46
auto-merge was automatically disabled January 6, 2023 10:46

Pull request was converted to draft

@sschuberth sschuberth removed the on hold Pull requests that cannot currently be merged label Jan 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants