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

feat(custom-license-texts): Add "SAP Developer License Agreement" content #201

Merged
merged 1 commit into from
Jun 20, 2024

Conversation

xLexip
Copy link
Contributor

@xLexip xLexip commented Jun 18, 2024

@xLexip xLexip marked this pull request as ready for review June 18, 2024 09:23
@xLexip xLexip requested a review from a team as a code owner June 18, 2024 09:23
@sschuberth
Copy link
Member

sschuberth commented Jun 18, 2024

For reference, I just verified that ScanCode 32.1.0 does not detect this license directly, but as

LicenseRef-scancode-nilsson-historical AND LicenseRef-scancode-unknown-license-reference AND LicenseRef-scancode-other-permissive

so having a custom LicenseRef for it seems sensible.

However, I wonder whether by convention we should use "ort" (not "SAP") as the "namespace", i.e. the part after the LicenseRef- prefix. Any opinion @oss-review-toolkit/core-devs?

@tsteenbe
Copy link
Member

Since the ORT project will be assigning the LicenseRef to this license it needs to be in our namespace so imo it should start with LicenseRef-ort - LicenseRef-SAP-* may be used by SAP itself which may result in a license namespace conflict.

@sschuberth
Copy link
Member

LicenseRef-SAP-* may be used by SAP itself which may result in a license namespace conflict.

I Agree.

@xLexip please rename the file to LicenseRef-ort-SAP-Developer-License-Agreement, amending your existing commit.

@xLexip
Copy link
Contributor Author

xLexip commented Jun 18, 2024

Alright, done @sschuberth.

@fviernau
Copy link
Member

What's the rationale @oss-review-toolkit/core-devs to start adding custom license texts into this repository?
Alternatively, the text could be contributed to ScanCode.

@sschuberth
Copy link
Member

What's the rationale @oss-review-toolkit/core-devs to start adding custom license texts into this repository?

Users who make use of this repo can then curate license findings to this custom license and get a proper license text in reports.

Alternatively, the text could be contributed to ScanCode.

That should be done additionally, not alternatively, as it does not help users until a new ScanCode version with the additional rule is released.

@fviernau
Copy link
Member

fviernau commented Jun 18, 2024

Users who make use of this repo can then curate license findings to this custom license and get a proper license text in reports.

That should be done additionally, not alternatively, as it does not help users until a new ScanCode version with the additional rule is released.

To me this is a clear indication that the custom license text is a temporary solution. This was one of the main goals the custom license text feature was invented for. So, my underlying question has not been discussed: Why should we invest effort into temporary solutions instead of leaving this up to the users entirely?

(License IDs are immutable, so under what contract do we put them here. To maintain these forever, including a deprecation mechanism? Maybe it could be an obligation, to at the minimum also have a ticket for ScanCode to add the license, and to have the agreement that the license ID can be dropped again, once available in ScanCode)

@sschuberth
Copy link
Member

To me this is a clear indication that the custom license text is a temporary solution.

Ideally yes, absolutely.

Why should we invest effort into temporary solutions instead of leaving this up to the users entirely?

To not require our users to each do the same work (internally) again.

to have the agreement that the license ID can be dropped again, once available in ScanCode

... and the ORT Docker image comes with the version of ScanCode that adds the license. That approach would make sense to me.

fviernau
fviernau previously approved these changes Jun 20, 2024

Version 3.1

Source: https://tools.eu1.hana.ondemand.com/developer-license-3_1.txt
Copy link
Member

Choose a reason for hiding this comment

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

This source line is not part of the original linked license text. I believe we should aim for only committing the exact 1:1 license text here. The origin should be documented as part of the commit message. Or what do @oss-review-toolkit/core-devs think?

Copy link
Member

Choose a reason for hiding this comment

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

good catch imo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed them.

Copy link
Member

Choose a reason for hiding this comment

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

@xLexip, I'm still seeing some whitespace / encoding related differences compared to the original files. Please really just take them as-is, without making any modifications compared to the upstream file. I'm expecting the 3.1. file to have a SHA1 sum of 65b2e7cba1e678ae1f0ca059ab42dff8d1a00777, and for 3.2 cbff7422c9b707e418665efeff42ec77e2f6fab7 respectively.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sschuberth done....

Copy link
Member

Choose a reason for hiding this comment

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

One more nit on the commit message: Why does it link to its own PR by adding "#201" to the commit message? I'd prefer to have that dropped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To have a reference to this entire discussion in the history for the next person who wants to add license text and looks at the previous changes. Removed it.

@sschuberth sschuberth merged commit 6995e27 into oss-review-toolkit:main Jun 20, 2024
2 checks passed
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.

4 participants