D6672: add KAboutLicense::spdx and introduce orLater qualification

[Harald Sitter]

sitter created this revision.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.

REVISION SUMMARY
  External software (e.g. appstream) uses the standardized SPDX license
  identifiers. Seeing as they are specified and ours are not it is useful to
  have a quick way to convert our licenses to SPDX notation.
  
  To also preserve 'this version or later versions' notations properly so
  we can reflect them into SPDX there now is an orLater bool which tracks
  the qualification. The qualification is both for us and for SPDX denoted
  as a trailing plus sign. To that end we'll infer that input keywords with
  a trailing plus sign are orLater. For code-constructed licenses via
  kAboutData.addLicense() there is a new explicit parameter orLater to
  qualify the license. The param defaults to false to retain backwards
  compatibility.
  Constructing KAboutData with orLater set is not supported as the param list
  is long enough.
  
  ::spdx() returns expressions. Right now they are simple ones (because we
  actually do not support anything more involved from a KAboutLicense POV)
  such as 'GPL-2.0+'. In the future this can easily be
  'GPL-2.0+ WITH excpetion' though if we decide to support that.
  
  Private::spdxID() is internally used to translate our enum key to the
  well-known identifier used by SPDX. It does not construct an expression.
  
  CHANGELOG: New spdx API on KAboutLicense to get SPDX license expressions

TEST PLAN
  - new test passes

REPOSITORY
  R244 KCoreAddons

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D6672

AFFECTED FILES
  autotests/kaboutdatatest.cpp
  src/lib/kaboutdata.cpp
  src/lib/kaboutdata.h

To: sitter, sebas
Cc: #frameworks

D6672: add KAboutLicense::spdx and introduce orLater qualification

[Harald Sitter]

sitter updated this revision to Diff 16633.
sitter added a comment.


  forgot to sort out BIC
  
  There is one theoretical BIC with the License ctor which has a new param. I 
am not sure we care about this given the ctor is private. Objections welcome 
but I think in the past we decied to not care about BC for privates so long as 
the object size doesn't change

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6672?vs=16632&id=16633

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D6672

AFFECTED FILES
  autotests/kaboutdatatest.cpp
  src/lib/kaboutdata.cpp
  src/lib/kaboutdata.h

To: sitter, sebas
Cc: #frameworks

D6672: add KAboutLicense::spdx and introduce orLater qualification

[Harald Sitter]

sitter updated this revision to Diff 16634.
sitter added a comment.


  add missing since tag

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6672?vs=16633&id=16634

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D6672

AFFECTED FILES
  autotests/kaboutdatatest.cpp
  src/lib/kaboutdata.cpp
  src/lib/kaboutdata.h

To: sitter, sebas
Cc: #frameworks

D6672: add KAboutLicense::spdx and introduce orLater qualification

[Harald Sitter]

sitter added a reviewer: mpyne.

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D6672

To: sitter, sebas, mpyne
Cc: #frameworks

D6672: add KAboutLicense::spdx and introduce orLater qualification

[Harald Sitter]

sitter added a comment.


  For convenience reasons it may actually be prudent to expose both `spdxID` 
and `orLater` publicly. Case in point right now appstream license information 
seem to be semi-expressions... they have the spdx ID followed by plus sign 
notation but compound expressions use lower case syntax. Still investigating 
though.

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D6672

To: sitter, sebas, mpyne
Cc: #frameworks

D6672: add KAboutLicense::spdx and introduce orLater qualification

[Harald Sitter]

sitter added a comment.


  In https://phabricator.kde.org/D6672#124995, @sitter wrote:
  
  > For convenience reasons it may actually be prudent to expose both `spdxID` 
and `orLater` publicly. Case in point right now appstream license information 
seem to be semi-expressions... they have the spdx ID followed by plus sign 
notation but compound expressions use lower case syntax. Still investigating 
though.
  
  
  Turns out this is a non-requirement for now. The appstream spec was just 
being a bit confusing.

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D6672

To: sitter, sebas, mpyne
Cc: #frameworks

D6672: add KAboutLicense::spdx and introduce orLater qualification

[Michael Pyne]

mpyne requested changes to this revision.
mpyne added a comment.
This revision now requires changes to proceed.


  AFAICT we do need to maintain BIC here even for private ctors.  The inline 
comment has more detail.  Other than that and with using a flag enum instead of 
a `bool` I like the patch and the approach, and how you've maintained the same 
basic approach the existing code does.
  
  I would lean towards also providing the convenience accessor in 
`KAboutLicense` for the orLater flag, but it is already exposed back to the 
user so perhaps it wouldn't add enough extra detail to be worth it.

INLINE COMMENTS

> kaboutdata.h:291
> +explicit KAboutLicense(enum KAboutLicense::LicenseKey licenseType, const 
> KAboutData *aboutData,
> +   bool orLater = false);
>  /**

Regarding the BIC question, we do consider this BIC, even though it is SC 
thanks to the default param.  Instead a second ctor is needed with this 
signature (and w/out the default param to avoid C++ errors), with a comment to 
merge with the existing ctor in KF6.

Please see 
https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B, 
you can search for "extending a function with another parameter" to see what 
I'm referring to.  This guidance applies to methods of any type, even private 
methods.

However, I think the `orLater` param would be more understandable as an enum 
flag, in the same way we modified our APIs in KDE4 and KF5 to try and avoid 
`bool` params.  This would be especially important for readability if we do 
start to support things like license exceptions, you can imagine future calls 
would then look like `...->addLicense(KAboutLicense::LGPL_V3, true, 
KAboutLicense::ExceptionGeneratedCodeUse);` and then wonder what the `true` was 
for.

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D6672

To: sitter, sebas, mpyne
Cc: #frameworks

D6672: add KAboutLicense::spdx and introduce orLater qualification

[Harald Sitter]

sitter updated this revision to Diff 16694.
sitter added a comment.


  BC constructor addition
  + change to enum for orlater as suggested
  + switch param orders to take license, then restriction, then kaboutdata
  
kaboutdata looks and feels idiomatically like a qobject parent which also is
always at the end. additionally this allows more for yet nicer looking code
as the param list is `GPL, OrLaterVersions, kaboutdata` so license 
description
params are right next to each other

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6672?vs=16634&id=16694

BRANCH
  spdx

REVISION DETAIL
  https://phabricator.kde.org/D6672

AFFECTED FILES
  autotests/kaboutdatatest.cpp
  src/lib/kaboutdata.cpp
  src/lib/kaboutdata.h

To: sitter, sebas, mpyne
Cc: #frameworks

D6672: add KAboutLicense::spdx and introduce orLater qualification

[Michael Pyne]

mpyne requested changes to this revision.
mpyne added a comment.
This revision now requires changes to proceed.


  Still a couple of things:
  
  1. The included new autotest fails for me, because of using `OrLaterVersion` 
as a default version restriction.  `OnlyThisVersion` is used elsewhere as the 
default, so I recommend fixing the KAboutLicense::Private to match the 
autotest.  I have verified that all KAboutData autotests build and pass with 
this change made.
  2. KAboutLicense::Private ctors all need to be updated to set 
`_versionRestriction` as well (basically anywhere that `_licenseKey` is set, 
needs to also set `_versionRestriction`).
  
  Once those fixes are made you have my +1, don't need to wait for a separate 
review for it unless you want another look.

INLINE COMMENTS

> kaboutdata.cpp:166
>_licenseKey(Unknown),
>_aboutData(aboutData)
>  {

Need a setter here for `_versionRestriction`

> kaboutdata.cpp:175
>_pathToLicenseTextFile(other._pathToLicenseTextFile),
>_aboutData(other._aboutData)
>  {}

Need a setter here for `_versionRestriction`

> kaboutdata.cpp:206
>  KAboutLicense::KAboutLicense(LicenseKey licenseType, const KAboutData 
> *aboutData)
> -: d(new Private(licenseType, aboutData))
> +: KAboutLicense(licenseType, OrLaterVersions, aboutData)
> +{

We use `OnlyThisVersion` elsewhere as a default version restriction; using 
`OrLaterVersions` here causes the auto test you added to break as well. ;)

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D6672

To: sitter, sebas, mpyne
Cc: #frameworks

D6672: add KAboutLicense::spdx and introduce orLater qualification

[Harald Sitter]

sitter updated this revision to Diff 16813.
sitter marked an inline comment as done.
sitter added a comment.


  - change "partial" Private ctor to delegate to "full" ctor so we don't have 
repetitive init lists
  - fix Private cctor to copy version restriction properly
- adjust copy test to assert proper restriction copy by comparing the spdx
  - fix default value I accidently changed to AndLater when it should be 
OnlyThis

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6672?vs=16694&id=16813

BRANCH
  spdx

REVISION DETAIL
  https://phabricator.kde.org/D6672

AFFECTED FILES
  autotests/kaboutdatatest.cpp
  src/lib/kaboutdata.cpp
  src/lib/kaboutdata.h

To: sitter, sebas, mpyne
Cc: #frameworks

D6672: add KAboutLicense::spdx and introduce orLater qualification

[Harald Sitter]

sitter updated this revision to Diff 16814.
sitter added a comment.


  rebase

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6672?vs=16813&id=16814

BRANCH
  spdx

REVISION DETAIL
  https://phabricator.kde.org/D6672

AFFECTED FILES
  autotests/kaboutdatatest.cpp
  src/lib/kaboutdata.cpp
  src/lib/kaboutdata.h

To: sitter, sebas, mpyne
Cc: #frameworks

D6672: add KAboutLicense::spdx and introduce orLater qualification

[Harald Sitter]

sitter marked 3 inline comments as done.
sitter added inline comments.

INLINE COMMENTS

> mpyne wrote in kaboutdata.cpp:166
> Need a setter here for `_versionRestriction`

I am changing this one to delegate to the other ctor actually. Not much point

> mpyne wrote in kaboutdata.cpp:206
> We use `OnlyThisVersion` elsewhere as a default version restriction; using 
> `OrLaterVersions` here causes the auto test you added to break as well. ;)

Ah yes. Good test that ;)

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D6672

To: sitter, sebas, mpyne
Cc: #frameworks

D6672: add KAboutLicense::spdx and introduce orLater qualification

[Harald Sitter]

sitter marked 2 inline comments as done.
sitter added a comment.


  Oh, actually. Maybe we could/should make `KAboutLicense::KAboutLicense(const 
KAboutData *aboutData)` delegate to the "full" public ctor. Then we can drop 
`Private(const KAboutData *aboutData);` entirely.
  
  Like so
  
KAboutLicense::KAboutLicense(LicenseKey licenseType,
 VersionRestriction versionRestriction,
 const KAboutData *aboutData)
: d(new Private(licenseType, versionRestriction, aboutData))
{
}

KAboutLicense::KAboutLicense(const KAboutData *aboutData)
: KAboutLicense(Unknown, OnlyThisVersion, aboutData)
{
}

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D6672

To: sitter, sebas, mpyne
Cc: #frameworks

D6672: add KAboutLicense::spdx and introduce orLater qualification

[Michael Pyne]

mpyne added a comment.


  I like delegating constructors, but we can't use them in Frameworks yet. :(
  
  The current compiler requirement policy is that we require a C++ compiler 
supported in the last 3 Qt minor releases (see 
https://community.kde.org/Frameworks/Policies#Frameworks_Qt_requirements).  
This means Qt 5.7's compilers are the oldest compilers we support, and Qt 5.7 
supports GCC 4.6 due to Ubuntu 
.  
But GCC didn't support delegating constructors until GCC 4.7 
.

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D6672

To: sitter, sebas, mpyne
Cc: #frameworks

D6672: add KAboutLicense::spdx and introduce orLater qualification

[Harald Sitter]

sitter added a comment.


  Ah drat. We could still get rid of the second private ctor though, by moving 
the defaults from the private to the public and calling the full private ctor:
  
KAboutLicense::KAboutLicense(const KAboutData *aboutData)
: d(new Private(Unknown, OnlyThisVersion, aboutData))
{
}

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D6672

To: sitter, sebas, mpyne
Cc: #frameworks

D6672: add KAboutLicense::spdx and introduce orLater qualification

[Michael Pyne]

mpyne added a comment.


  In https://phabricator.kde.org/D6672#126360, @sitter wrote:
  
  > Ah drat. We could still get rid of the second private ctor though, by 
moving the defaults from the private to the public and calling the full private 
ctor:
  
  
  Yes, that would be fine if you want to implement it that way.

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D6672

To: sitter, sebas, mpyne
Cc: #frameworks

D6672: add KAboutLicense::spdx and introduce orLater qualification

[Harald Sitter]

sitter updated this revision to Diff 16858.
sitter added a comment.


  - do not use ctor delegation, can't use that in kf5 yet
  - eliminate the "partial" private ctor, instead call the full private ctor 
from the partial public ctors. this results in defaults being implemented in 
the public ctors' code rather than the private one, and makes it slightly 
harder to forget initalizing members as there's now only two ctors that have to 
deal with them directly.

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6672?vs=16814&id=16858

BRANCH
  spdx

REVISION DETAIL
  https://phabricator.kde.org/D6672

AFFECTED FILES
  autotests/kaboutdatatest.cpp
  src/lib/kaboutdata.cpp
  src/lib/kaboutdata.h

To: sitter, sebas, mpyne
Cc: #frameworks

D6672: add KAboutLicense::spdx and introduce orLater qualification

[Michael Pyne]

mpyne accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R244 KCoreAddons

BRANCH
  spdx

REVISION DETAIL
  https://phabricator.kde.org/D6672

To: sitter, sebas, mpyne
Cc: #frameworks

D6672: add KAboutLicense::spdx and introduce orLater qualification

[Harald Sitter]

This revision was automatically updated to reflect the committed changes.
Closed by commit R244:107a7fd1a3c0: add KAboutLicense::spdx and introduce 
orLater qualification (authored by sitter).

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6672?vs=16858&id=16932

REVISION DETAIL
  https://phabricator.kde.org/D6672

AFFECTED FILES
  autotests/kaboutdatatest.cpp
  src/lib/kaboutdata.cpp
  src/lib/kaboutdata.h

To: sitter, sebas, mpyne
Cc: #frameworks