D21305: Add the FreeBSD default-path for os-release.
adridg abandoned this revision. adridg added a comment. Thanks for looking at this, Harald. For various downstream packaging reasons, we're going to be stuck with patching, so I'm going to give up on this particular patch. REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D21305 To: adridg, sitter Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D21305: Add the FreeBSD default-path for os-release.
sitter added a comment.
I do wonder if we couldn't just move the freebsd path to the end of the list
and drop the ifdef. As far as linux is concerned we still obey the lookup order
but simply have an additional path where we may look (and where the file should
not ever exist on linux). Conversely on the freebsd side the file should never
exist in the other two paths so it would simply fall through to usr/local.
That said, the code is fine, ifdefs just trip me up when reading ^^
INLINE COMMENTS
> kosrelease.cpp:79
> {
> -if (QFile::exists(QStringLiteral("/etc/os-release"))) {
> -return QStringLiteral("/etc/os-release");
> -} else if (QFile::exists(QStringLiteral("/usr/lib/os-release"))) {
> -return QStringLiteral("/usr/lib/os-release");
> -} else {
> -return QString();
> +for (const auto& path : {
> +#ifdef Q_OS_FREEBSD
& goes on the left of the space please
> kosrelease.cpp:81
> +#ifdef Q_OS_FREEBSD
> +QStringLiteral("/usr/local/etc/os-release"),
> +#endif
The indentation is missing a space on every line. Currently it aligns with the
bracket when it fact it should align with the arguments after the opening
bracket.
REPOSITORY
R244 KCoreAddons
REVISION DETAIL
https://phabricator.kde.org/D21305
To: adridg, sitter
Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D21305: Add the FreeBSD default-path for os-release.
adridg edited the test plan for this revision. REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D21305 To: adridg, sitter Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D21305: Add the FreeBSD default-path for os-release.
adridg edited the test plan for this revision. REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D21305 To: adridg, sitter Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D21305: Add the FreeBSD default-path for os-release.
adridg created this revision.
adridg added a reviewer: sitter.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
adridg requested review of this revision.
REVISION SUMMARY
- After much hemming and hawing we ended up with /usr/local/etc/os-release,
which isn't one of the standard paths (according to freedesktop.org) so in
spite of us **having** the file, not all software that looks for it will find
it. Patch in the correct path.
- Patching in the correct path in the original code inserted #ifdefs that
interact really badly with the existing code-layout.
- Switch to iterating over a constant list; with Clang and -O3 this yields
slightly smaller code compared to the original. This code is also much easier
to #ifdef for specific needs (e.g. for NetBSD and OpenBSD).
TEST PLAN
- Included in downstream packaging.
- Test for functionality: ```
#include
#include
static void derp(const QString& s)
{
qDebug() << s;
KOSRelease r(s);
qDebug() << " name" << r.name()
<< "\n version" << r.version();
}
int main(int argc, char **argv)
{
derp(QString());
derp("/usr/local/etc/os-release");
return 0;
}
- Test for code size:
#include
#include
QString defaultFilePath()
{
#ifdef ONE
if (QFile::exists(QStringLiteral("/etc/os-release"))) {
return QStringLiteral("/etc/os-release");
} else if (QFile::exists(QStringLiteral("/usr/lib/os-release"))) {
return QStringLiteral("/usr/lib/os-release");
} else {
return QString();
}
#endif
#ifdef TWO
for (const auto& path : {
QStringLiteral("/etc/os-release"),
QStringLiteral("/usr/lib/os-release")
}) {
if (QFile::exists(path)) {
return path;
}
}
return QString();
#endif
}
REPOSITORY
R244 KCoreAddons
BRANCH
master
REVISION DETAIL
https://phabricator.kde.org/D21305
AFFECTED FILES
src/lib/util/kosrelease.cpp
To: adridg, sitter
Cc: kde-frameworks-devel, michaelh, ngraham, bruns
