Details

Reviewers

sebas
mpyne

Commits

R244:107a7fd1a3c0: add KAboutLicense::spdx and introduce orLater qualification

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

Diff Detail

Repository

R244 KCoreAddons

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

sitter created this revision.Jul 13 2017, 9:51 AM

Restricted Application added a project: Frameworks. · View Herald TranscriptJul 13 2017, 9:51 AM

Restricted Application added a subscriber: Frameworks. · View Herald Transcript

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

add missing since tag

sitter added a reviewer: mpyne.Jul 13 2017, 10:09 AM

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.

In 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.

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.

src/lib/kaboutdata.h
305	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.

This revision now requires changes to proceed.Jul 13 2017, 4:24 PM

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

Still a couple of things:

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.
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.

src/lib/kaboutdata.cpp
158	Need a setter here for `_versionRestriction`
168	Need a setter here for `_versionRestriction`
207	We use `OnlyThisVersion` elsewhere as a default version restriction; using `OrLaterVersions` here causes the auto test you added to break as well. ;)

This revision now requires changes to proceed.Jul 17 2017, 2:06 AM

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

rebase

sitter marked 3 inline comments as done.Jul 17 2017, 12:52 PM

sitter added inline comments.

src/lib/kaboutdata.cpp
158	I am changing this one to delegate to the other ctor actually. Not much point
207	Ah yes. Good test that ;)

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)
{
}

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.

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))
{
}

In 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.

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.

mpyne accepted this revision.Jul 19 2017, 9:38 PM

This revision is now accepted and ready to land.Jul 19 2017, 9:38 PM

Closed by commit R244:107a7fd1a3c0: add KAboutLicense::spdx and introduce orLater qualification (authored by sitter). · Explain WhyJul 19 2017, 9:41 PM

This revision was automatically updated to reflect the committed changes.

kossebau mentioned this in D27640: templates: Convert license statements to SPDX headers.Feb 29 2020, 11:20 AM

			Path	Packages
M			autotests/kaboutdatatest.cpp (38 lines)
M			src/lib/kaboutdata.h (61 lines)
M			src/lib/kaboutdata.cpp (110 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	16632	b8cb28b		Jul 13 2017, 9:51 AM	★	★
Diff 2	16633	b8cb28b	forgot to sort out BIC	Jul 13 2017, 9:57 AM	★	★
Diff 3	16634	b8cb28b	add missing since tag	Jul 13 2017, 9:59 AM	★	★
Diff 4	16694	b8cb28b	BC constructor addition	Jul 14 2017, 9:54 AM	★	★
Diff 5	16813	b8cb28b	- change "partial" Private ctor to delegate to "full" ctor so we don't	Jul 17 2017, 12:49 PM	★	★
Diff 6	16814	0c33e31	rebase	Jul 17 2017, 12:51 PM	★	★
Diff 7	16858	0c33e31	- do not use ctor delegation, can't use that in kf5 yet	Jul 18 2017, 8:54 AM	★	★
Diff 8	16932	7520288	R244:107a7fd1a3c06f0bbf0d4baef8615c99cea8c634	Jul 19 2017, 9:41 PM	★	★

add KAboutLicense::spdx and introduce orLater qualification
ClosedPublic
Actions

Details

Diff Detail

Revision Contents
Changeset List

Diff 16932

autotests/kaboutdatatest.cpp

src/lib/kaboutdata.h

src/lib/kaboutdata.cpp

add KAboutLicense::spdx and introduce orLater qualificationClosedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 16932

autotests/kaboutdatatest.cpp

src/lib/kaboutdata.h

src/lib/kaboutdata.cpp

add KAboutLicense::spdx and introduce orLater qualification
ClosedPublic
Actions

Revision Contents
Changeset List