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

schema/ListedLicense: Drop isDeprecated #583

Closed
wants to merge 2 commits into from

Conversation

wking
Copy link
Contributor

@wking wking commented Dec 29, 2017

deprecatedVersion should be set if and only if isDeprecated was true. So instead of recording that information twice, tooling which consumes this format should just check deprecatedVersion. If deprecatedVersion is non-empty, the license is deprecated. If deprecatedVersion is empty, the license is not deprecated.

This protects us from desynchronization like that fixed by cc1e05e (added deprecated version, 2017-12-28), because it is now impossible to deprecate a license without recording the deprecated version.

The schema change is a two-liner, and the src/* change was generated with:

$ sed -i 's/isDeprecated="true" //' $(git grep -l isDeprecated)

which should make review straightforward.

Downstream, this will need spdx/tools changes to get the license/exception-loading code looking at deprecatedVersion to generate isDeprecatedLicenseId output here and similar. This change is easy enough here that, if folks think this change is promising, we should probably block this PR on landing the spdx/tools update. I propose the following order:

  1. Review this PR for the general thrust. If, for some reason, the license list maintainers feel it is not productive, we can close this without further changes.
  2. Update spdx/tools to only consider deprecatedVersion when generating isDeprecatedLicenseId and similar.
  3. Rebase this PR to pick up whatever changes have landed in this repository in the meantime.
  4. Merge this PR.

@wking
Copy link
Contributor Author

wking commented Jan 16, 2018

Rebased around #452 with 1a74f3e4e1a22b.

deprecatedVersion should be set if and only if isDeprecated was true.
So instead of recording that information twice, tooling which consumes
this format should just check deprecatedVersion.  If deprecatedVersion
is non-empty, the license is deprecated.  If deprecatedVersion is
empty, the license is not deprecated.

This protects us from desynchronization like that fixed by cc1e05e
(added deprecated version, 2017-12-28), because it is now impossible
to deprecate a license without recording the deprecated version.
@wking wking force-pushed the drop-is-deprecated branch from 4e1a22b to a1a3e68 Compare January 29, 2018 23:51
To catch up with the previous commit's schema change.  Generated with:

  $ sed -i 's/isDeprecated="true" //' $(git grep -l isDeprecated)
@wking
Copy link
Contributor Author

wking commented Jan 29, 2018

Rebased around #586 and #599 with 4e1a22ba1a3e68.

@goneall
Copy link
Member

goneall commented Jan 30, 2018

Update to the tools in progress: spdx/LicenseListPublisher#3

The tools currently depend on the isDeprecated flag. This pull request changes the tools to only look at the deprecated version.

Note that the isDeprecateVersion is still maintained in the output JSON, RDFa, and RDF data files for both compatibility and those applications wishing to just check the Boolean flag for the deprecated condition.

@wking wking mentioned this pull request Mar 26, 2018
@wking
Copy link
Contributor Author

wking commented Mar 26, 2018

Update to the tools in progress: spdx/LicenseListPublisher#3

With that pull request landed, can we move this forward again? I think we'll need to cut a LicenseListPublisher release, update the Makefile here to use that version, and then rebase this PR to resolve the conflicts.

@goneall
Copy link
Member

goneall commented Mar 26, 2018

@wking I'll cut a new release of LicenseListPublish once we have the FSF Free/Libre issues worked out (which should be relatively soon).

@jlovejoy jlovejoy modified the milestones: 3.2 release, Later Release Apr 5, 2018
@Grinnerbren
Copy link

Open

Copy link

@lenora2468B lenora2468B left a comment

Choose a reason for hiding this comment

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

I am still learning all the steps to this and hope it's ok to copy the path. Thank you.

@jlovejoy
Copy link
Member

@goneall - can we close this, not sure what status is at this point?

@goneall
Copy link
Member

goneall commented May 13, 2021

@jlovejoy The tooling has been updated, so we can now apply the change. It does, however, need to be rebased before merging.

I think the change is a good idea since it would make it easier to maintain the license list - you only need to update the deprecated version and it would prevent inconsistencies such as having a deprecated version and isDeprecated set to false.

@wking added good documentation on how to create the pull request, so if @wking isn't available to rebase, I can generate a new PR sometime over the next couple of weeks.

Let's leave it open for now.

@swinslow
Copy link
Member

@goneall For this old PR -- if I'm following, it looks like this is (1) deleting the definition of isDeprecated in the schema, and then (2) deleting isDeprecated= attributes from the handful of licenses that have them.

Does that sound correct? If so, I can create a new PR to handle this. I think there are some now-deprecated licenses that weren't picked up here at the time, so it would likely need a refresh anyway.

@goneall
Copy link
Member

goneall commented Nov 17, 2021

@goneall For this old PR -- if I'm following, it looks like this is (1) deleting the definition of isDeprecated in the schema, and then (2) deleting isDeprecated= attributes from the handful of licenses that have them.

Does that sound correct? If so, I can create a new PR to handle this. I think there are some now-deprecated licenses that weren't picked up here at the time, so it would likely need a refresh anyway.

@swinslow Yes - that is correct - a new PR would be appreciated.

@jlovejoy
Copy link
Member

@swinslow @goneall - can I close this?

@swinslow
Copy link
Member

Yup -- I just added #1545 to track the yet-to-be-done task that came out of the discussion here, so this PR can be closed (which I'm doing now).

@swinslow swinslow closed this Jul 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants