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.

Sign up for GitHub

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

Jump to bottom

LicenseChoices: Add repository license choices #3801

Merged
merged 6 commits into from
Mar 29, 2021
Merged

LicenseChoices: Add repository license choices #3801

merged 6 commits into from
Mar 29, 2021

Conversation

neubs-bsi
Copy link
Contributor

Resolves #3396.

Introduce license choices that span the whole project scope and can be overwritten by package specific license choices.

@neubs-bsi
Copy link
Contributor Author

cc:@MarcelBochtler

@neubs-bsi neubs-bsi force-pushed the license-choice-repo-config-local branch 2 times, most recently from d8ca3af to 8251705 Compare March 23, 2021 16:04
mnonnenmacher
mnonnenmacher requested changes Mar 25, 2021
View reviewed changes
model/src/main/kotlin/config/LicenseChoices.kt Outdated Show resolved Hide resolved
model/src/main/kotlin/config/LicenseChoices.kt

init {
val choicesWithoutGiven = repositoryLicenseChoices.filter { it.given == null }
require(choicesWithoutGiven.isEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do repositoryLicenseChoices require a given but packageLicenseChoices do not?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When using a package license choice we have a specific effective license of the package a choice (or several) can be applied to, which is most likely to be known before applying the choice. When we have a repository license choice, the license choice would be applied to each package that offers this license as a choice. Making sure that there is a given helps only applying the choice to a wanted given as opposed to all licenses with that choice, which could lead to unwanted choices.
For consistency it would be possible to make given mandatory for packageLicenseChoices, but this was previously discussed before and decided otherwise.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation, can you add that to the code docs, e.g. for the LicenseChoices class or the properties? And maybe also to the documentation from the last commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the explanation to the RepositoryLicenseChoices, but am unsure if this is how and where it belongs.
I also added it to the documentation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I think this is good enough for now. I just wanted to avoid that when you read this code without knowing the context you stumble over the fact that only one of the properties has this requirement.

reporter/src/funTest/assets/static-html-reporter-test-input.yml Outdated Show resolved Hide resolved
reporter/src/funTest/assets/static-html-reporter-test-input.yml Outdated Show resolved Hide resolved
reporter/src/funTest/assets/static-html-reporter-test-input.yml Outdated Show resolved Hide resolved
evaluator/src/main/kotlin/PackageRule.kt Show resolved Hide resolved
@neubs-bsi neubs-bsi force-pushed the license-choice-repo-config-local branch from 8251705 to 5f357fe Compare March 26, 2021 08:28
@neubs-bsi neubs-bsi requested a review from mnonnenmacher March 26, 2021 08:29
@neubs-bsi neubs-bsi force-pushed the license-choice-repo-config-local branch 2 times, most recently from 5388f7f to a8aaf17 Compare March 26, 2021 15:11
mnonnenmacher
mnonnenmacher requested changes Mar 26, 2021
View reviewed changes
model/src/main/kotlin/config/LicenseChoices.kt Outdated Show resolved Hide resolved
model/src/main/kotlin/config/LicenseChoices.kt

init {
val choicesWithoutGiven = repositoryLicenseChoices.filter { it.given == null }
require(choicesWithoutGiven.isEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I think this is good enough for now. I just wanted to avoid that when you read this code without knowing the context you stumble over the fact that only one of the properties has this requirement.

@neubs-bsi
ResolvedLicenseConfiguration: Add repository license choices
69ff88e 
These are meant for license choices that apply to all packages
in the repository.

Signed-off-by: Stephanie Neubauer <Stephanie.Neubauer@bosch.io>
@neubs-bsi neubs-bsi force-pushed the license-choice-repo-config-local branch from a8aaf17 to b604557 Compare March 29, 2021 06:25
@neubs-bsi neubs-bsi requested a review from mnonnenmacher March 29, 2021 06:26
mnonnenmacher
mnonnenmacher previously approved these changes Mar 29, 2021
View reviewed changes
sschuberth
sschuberth reviewed Mar 29, 2021
View reviewed changes
evaluator/src/test/kotlin/RuleSetTest.kt Outdated Show resolved Hide resolved
evaluator/src/test/kotlin/RuleSetTest.kt
}
}
}

private fun PackageRule.LicenseRule.containsLicense(expression: SpdxExpression) =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: The extraction of this function could have happened in a preparing commit, as it's unrelated to the actual change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to leave these changes together. As with the extraction came a fix for the actual test we weren't aware of before.

model/src/main/kotlin/licenses/ResolvedLicenseInfo.kt Show resolved Hide resolved
model/src/main/kotlin/licenses/ResolvedLicenseInfo.kt Outdated Show resolved Hide resolved
@neubs-bsi
RuleSetTest: Fix rule matcher of license choice test
eafe2ef 
Since for a license choice only the concluded is relevant, the license view is
changed to use `ONLY_CONCLUDED`.

Signed-off-by: Stephanie Neubauer <Stephanie.Neubauer@bosch.io>
@neubs-bsi
PackageRule: Apply repository license choices
779ceda 
The order of applying the license choices from the different sources
ensures that the more precise scope is applied first (=prioritized).

Signed-off-by: Stephanie Neubauer <Stephanie.Neubauer@bosch.io>
@neubs-bsi
ResolvedLicenseInfo: Allow multiple license choices in effective license
dd0b2ae 
Enable to use license choices from different sources (e.g. global and
package specific) in a chosen order.

Signed-off-by: Stephanie Neubauer <Stephanie.Neubauer@bosch.io>
@neubs-bsi
ReportTableModelMapper: Apply repoLicenseChoices to effectiveLicense
db9cfa5 
Signed-off-by: Stephanie Neubauer <Stephanie.Neubauer@bosch.io>
@neubs-bsi
Documentation: Add repository license choice to ort.yml documentation
2577355 
Resolves oss-review-toolkit#3396.

Signed-off-by: Stephanie Neubauer <Stephanie.Neubauer@bosch.io>
@neubs-bsi neubs-bsi requested a review from sschuberth March 29, 2021 13:04
@neubs-bsi neubs-bsi force-pushed the license-choice-repo-config-local branch from b604557 to 2577355 Compare March 29, 2021 13:05
@sschuberth sschuberth merged commit de3668b into oss-review-toolkit:master Mar 29, 2021
@sschuberth sschuberth deleted the license-choice-repo-config-local branch March 29, 2021 13:39
@MarcelBochtler MarcelBochtler mentioned this pull request Aug 23, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sschuberth sschuberth sschuberth approved these changes

@mnonnenmacher mnonnenmacher mnonnenmacher left review comments

@fviernau fviernau Awaiting requested review from fviernau

@tsteenbe tsteenbe Awaiting requested review from tsteenbe

Assignees

@mnonnenmacher mnonnenmacher

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add functionality to handle license choices
3 participants
@neubs-bsi @sschuberth @mnonnenmacher