New class KOSRelease - a parser for os-release files
ClosedPublic

Authored by sitter on Mar 28 2019, 1:59 PM.

Details

Summary

Imported form KInfoCenter along with test case. This also includes a
license change from GPL to LGPL.

When working on Linux-centric software it may be necessary to get specific
information about the system beyond what may be obtained from QSysInfo.
This new class allows complete access to all information in the os-release
file as per the os-release "specification".

https://www.freedesktop.org/software/systemd/man/os-release.html

Test Plan
  • new test passes
  • header installed
  • camelheader installed
  • kinfocenter can successfully find the header and link the new class in place of the old builtin one

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.Mar 28 2019, 1:59 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMar 28 2019, 1:59 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
sitter requested review of this revision.Mar 28 2019, 1:59 PM
sitter added a reviewer: apol.Mar 28 2019, 1:59 PM
apol requested changes to this revision.Mar 28 2019, 3:42 PM
apol added inline comments.
src/lib/util/kosrelease.h
35

it's a "free desktop standard".

40

Maybe it would make sense to disable the copy operator? Or implement it otherwise.

This revision now requires changes to proceed.Mar 28 2019, 3:42 PM
sitter updated this revision to Diff 54993.Mar 28 2019, 4:29 PM
  • doc wording
  • disable cctor. no real use case for copy at this point
apol accepted this revision.Mar 29 2019, 12:21 AM
This revision is now accepted and ready to land.Mar 29 2019, 12:21 AM
apol added a subscriber: mpyne.Mar 29 2019, 12:22 AM

@mpyne is KCoreAddons maintainer, maybe he has some comments on the new class.

aacid added a subscriber: aacid.Mar 29 2019, 10:07 PM

Just to make sure, have you checked the log to make sure everyone involved in the class agrees to GPL -> LGPL?

mpyne accepted this revision.Mar 30 2019, 4:19 PM

+1 to @aacid's comment about relicensing.

The code is good and easy to review. Only comment I had was to think about using a warning or error when unable to parse the file.

I think that the ID_LIKE handling could be done by using setVar(QString) and then splitting the result on space instead of stripping quotes first and letting KShell figure it out, but I concur that anyone trying to use special characters in ID_LIKE is just asking for trouble anyways.

However the warning/error decision goes, I support landing the change as long as any copyright holders to the KInfoCenter source agree to the change.

src/lib/util/kosrelease.cpp
127

Would this situation not warrant a warning or debug entry to be printed to console? Or are you concerned this might be too noisy?

pino added a subscriber: pino.Mar 30 2019, 5:16 PM
pino added inline comments.
src/lib/util/kosrelease.cpp
121–127

This fails if any field as '=' in it; considering that few fields contain display text, they may contain it as part of the "pretty name" of the distribution. Sure, not highly probably, but still possible.
I recommend to manually find the first '=', and split key/value accordingly.

src/lib/util/kosrelease.h
105

If it is not needed as public, just make it static in the .cpp file.

Dave, are you ok with changing the license from GPL to LGPL?

Sure.

sitter updated this revision to Diff 55338.Apr 3 2019, 12:02 PM
  • defaultFilePath is now static in the cpp only, the public ctor defaults to QString() for the filePath, which gets checked in the private ctor and diverted to defaultFilePath if necessary
  • Private ctor now copies filePath as we may need to change it to defaultFilePath
  • = and \# parsing revisitede
    • new helper splitEntry mids the line at first = character only preventing = characters in pretty strings from tripping up the parsing
    • \# characters that are not the first character in the line are now not explicitly causing a line skip as far as entry parsing is concerned. I think this is actually a more truthful implementation of the format specification. since the spec doesn't even mention trailing \# we can do whatever with them and not skipping entire lines outright seems like the nicer solution here
  • api docs updated accordingly. now clearly states that trailing comments result in undefined behavior
  • test and fixture updated to make sure = and \# work in pretty string and that completely invalid lines are skipped
  • parsing skips now produce categorized warnings

I am wondering if from an API POV it'd be nicer if we maybe had two ctors (one default, one with QString) so we can let initalizer lists handle whether the defaults apply or not.

apol accepted this revision.Apr 16 2019, 12:52 AM
This revision was automatically updated to reflect the committed changes.