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

Restructure licensing profile #399

Merged
merged 6 commits into from
Jul 27, 2023
Merged

Restructure licensing profile #399

merged 6 commits into from
Jul 27, 2023

Conversation

armintaenzertng
Copy link
Contributor

This PR summarizes the discussion we had on the tech team call yesterday (June 27).
This moves all classes used for more in-depth modeling of licenses into the ExpandedLicensing profile, keeping only AnyLicenseInfo, LicenseExpression as well as a new SimpleCustomLicense class in the LicensingProfile.

The new structure is open to discussion. I believe some descriptions have to be rewritten.

@armintaenzertng
rework licensing profile according to June 27th meeting
920c1f0 
Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.com>
@zvr
Copy link
Member

zvr commented Jun 28, 2023

I propose SimpleCustomLicense be renamed to SimpleLicensingText or something.

Then this can be used for License texts as well as for Addition texts. Otherwise, we would need a SimpleCustomAddition in the Licensing profile, which will be identical to SimpleLicense (only a string property for the text).

zvr
zvr requested changes Jun 28, 2023
View reviewed changes
model/Licensing/Classes/SimpleCustomLicense.md Outdated Show resolved Hide resolved
model/Licensing/Classes/SimpleCustomLicense.md Outdated Show resolved Hide resolved
model/Licensing/Classes/SimpleCustomLicense.md Outdated Show resolved Hide resolved
model/Licensing/Classes/SimpleCustomLicense.md Outdated Show resolved Hide resolved
@armintaenzertng
change SimpleCustomLicense to SimpleLicensingText
e0d88fe 
Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.com>
goneall
goneall reviewed Jun 28, 2023
View reviewed changes
Copy link
Member

@goneall goneall left a comment

Choose a reason for hiding this comment

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

More of a request for a discussion on my comments.

model/Licensing/Classes/SimpleLicensingText.md Outdated Show resolved Hide resolved
model/Licensing/Licensing.md Show resolved Hide resolved
model/Licensing/Classes/SimpleLicensingText.md Outdated Show resolved Hide resolved
@goneall
Copy link
Member

goneall commented Jun 28, 2023

@swinslow Some rather significant restructuring as a result of the last SPDX tech call. You can see the discussion in the minutes in the etherpad (probably not moved over to minutes yet). If you'd like to discuss real-time, just ping me and I can give you more context.

@goneall goneall requested a review from swinslow June 28, 2023 12:44
@goneall
Copy link
Member

goneall commented Jun 28, 2023

Thanks @armintaenzertng for creating this - overall structure looks good based on our discussion. A couple of review comments to consider / discuss.

@armintaenzertng
change superclass of SimpleLicensingText to Element
a0dbc34 
Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.com>
@armintaenzertng
move licenseText property to Licensing profile to replace simpleLicen…
37d08e6 
…singText

Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.com>
@maxhbr
Copy link
Member

maxhbr commented Jul 11, 2023

A snapshot at a0dbc34:
Licensing Profile
ExpandedLicense Profile

@goneall goneall added this to the 3.0-rc2 milestone Jul 12, 2023
@goneall
Copy link
Member

goneall commented Jul 24, 2023

@armintaenzertng - Pinging you on this one - can you go back through the review comments and update what you agree with and (re)comment on any outstanding?

I just bumped into this in my java code generator and would like to merge.

@armintaenzertng
rename Licensing to SimpleLicensing and ExpandedLicense to ExpandedLi…
4717cf5 
…censing

also add the corresponding ProfileIdentifierTypes

Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.com>
@armintaenzertng
Copy link
Contributor Author

@goneall, @zvr, I implemented all outstanding change requests, please take another look.

goneall
goneall approved these changes Jul 25, 2023
View reviewed changes
Copy link
Member

@goneall goneall left a comment

Choose a reason for hiding this comment

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

Thanks @armintaenzertng

@armintaenzertng
fix reference to licenseText in SimpleLicensingText
7f91ff4 
Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.com>
@armintaenzertng
Copy link
Contributor Author

Found and fixed another little bug.

@armintaenzertng armintaenzertng mentioned this pull request Jul 27, 2023
Closed
@swinslow
Copy link
Member

I have a few concerns about the way this is currently addressed. I'm not going to stand in the way of merging it as a next draft, but from an initial read, these are the points that I think should be addressed prior to the actual 3.0 release:

  • As currently structured, it looks like for validation purposes the only requirement is that LicenseExpression should be an xsd:string. My assumption is that it additionally would only be valid if that string conforms to the License Expression Syntax. I'm not sure how we express that in the spdx-3-model repo syntax, but I think that's an important thing for us to retain if all the pieces of the model for license combinations are being moved to a separate profile.

  • Relatedly, I think part of the expression being valid is to be able to ensure that the license identifiers are valid, e.g. that any non-LicenseRef's are actual identifiers on the License List. I'm not sure whether that's feasible to do under the current structure without leveraging the ExpandedLicensing profile, if ListedLicense and ListedLicenseException are over in that profile.

  • I am worried that it's confusing to have a simpleLicensingText class in the SimpleLicensing profile which is comparable to, but distinct from, CustomLicense in the ExpandedLicensing profile.

@swinslow
Copy link
Member

On the first point above, I should note that @goneall has filed #440 to track clarifying this.

@goneall
Copy link
Member

goneall commented Jul 27, 2023

Per discussion with @swinslow - I'll go ahead and merge this PR and we can discuss the outstanding issues raised in the comment above after merging.

@goneall goneall merged commit 87e131f into spdx:main Jul 27, 2023
1 check passed
@armintaenzertng armintaenzertng deleted the reworkLicensing branch July 28, 2023 05:47
@goneall goneall mentioned this pull request Jul 29, 2023
Closed
@yoshi-i yoshi-i mentioned this pull request Aug 1, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zvr zvr zvr approved these changes

@maxhbr maxhbr maxhbr left review comments

@goneall goneall goneall approved these changes

@swinslow swinslow Awaiting requested review from swinslow

Assignees
No one assigned
Labels
Profile:Licensing
Projects
None yet
Milestone
3.0-rc2
Development

Successfully merging this pull request may close these issues.
5 participants
@armintaenzertng @zvr @goneall @maxhbr @swinslow