[Differential] [Updated] D1216: Disallow ptrace on greeter and kcheckpass process on FreeBSD
rakuco added a comment.
Looks great to me now; I'll leave it to Martin to accept the diff.
INLINE COMMENTS
CMakeLists.txt:34 While here, you could adjust the wording a little, like I
suggested in my comment ("disallow" -> "disallowing", "the greeter and
kcheckpass processes").
REPOSITORY
rKSCREENLOCKER KScreenLocker
REVISION DETAIL
https://phabricator.kde.org/D1216
EMAIL PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To: tcberner, graesslin, rakuco
Cc: plasma-devel, sebas
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
[Differential] [Commented On] D1216: Disallow ptrace on greeter and kcheckpass process on FreeBSD
rakuco added inline comments.
INLINE COMMENTS
CMakeLists.txt:25-30 I think you need to merge these two `add_feature_info()`
calls: as-is, either one or the other will always be shown in the "disabled
features" list, and I guess you only want this to happen if neither call/header
is found. Perhaps something like this:
```
check_include_file(...)
check_symbol_exists(...)
# ...
if (HAVE_PR_SET_DUMPABLE OR HAVE_PROC_TRACE_CTL)
set(CAN_DISABLE_PTRACE TRUE)
endif ()
add_feature_info("prctl/procctl tracing control"
CAN_DISABLE_PTRACE
"Required for disallowing ptrace on greeter and kcheckpass
processes.")
```
greeter/main.cpp:67 There's an extra space before the `if` here and in a few
other places.
greeter/main.cpp:140-141 The indentation here looks wrong.
kcheckpass/kcheckpass.c:339-340 The indentation looks wrong here too.
REPOSITORY
rKSCREENLOCKER KScreenLocker
REVISION DETAIL
https://phabricator.kde.org/D1216
EMAIL PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To: tcberner, graesslin, rakuco
Cc: plasma-devel
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 126203: Disable ptrace for kcheckpass and the greeter
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126203/#review89002 --- Ship it! Thanks for spending time making this portable. I'm unable to test this at the moment; if Tobias is around and able to submit a FreeBSD version it'd be good to integrate it into the same patch, otherwise we can just do that later. - Raphael Kubo da Costa On Dec. 1, 2015, 3:13 p.m., Martin Gräßlin wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126203/ > --- > > (Updated Dec. 1, 2015, 3:13 p.m.) > > > Review request for Plasma and Tobias Berner. > > > Repository: kscreenlocker > > > Description > --- > > Setting the PR_SET_DUMPABLE flag to 0 for the security relevant > command kcheckpass and kscreenlocker_greet. If one wants to gdb into > the running command it will result in: > > ptrace: Operation not permitted. > > For kscreenlocker_greet ptrace is permitted in testing mode. > > As root it's still possible to attach to the process. > > --- > > @Tobias: I assume this is a strong linux-ism. Is there a FreeBSD compareable > functionality? > > I'm considering to push this explicitly without an ifdef. It's a new security > feature and I want to make non-Linux systems aware of the fact that it adds a > new feature and that a replacement should be added. > > > Diffs > - > > CMakeLists.txt f48bd53cafc188f79e041518dae0769d57597c69 > config-kscreenlocker.h.cmake 2a034dee8ec21e426bc1db1d56b0ed152d3de2ca > greeter/main.cpp e4e679e7ef40b319665428281fdba5f4e0b4eb25 > kcheckpass/kcheckpass.c fd2d2215bf2199f159a121bb0ce08e7b2b254aaa > > Diff: https://git.reviewboard.kde.org/r/126203/diff/ > > > Testing > --- > > Tried to gdb into the processes: failed > Tried to gdb into kscreenlocker_greet --testing: succeeded > Tried to gdb into kscreenlocker_greet as root: succeeded > > > Thanks, > > Martin Gräßlin > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 126203: Disable ptrace for kcheckpass and the greeter
> On Dec. 1, 2015, 12:04 p.m., Raphael Kubo da Costa wrote: > > CMakeLists.txt, line 25 > > <https://git.reviewboard.kde.org/r/126203/diff/2/?file=420269#file420269line25> > > > > This is being checked for here but not set in > > `config-kscreenlocker.h.cmake`, so the header is still being included > > unconditionally. > > Martin Gräßlin wrote: > sorry, I don't get what you mean. I added a cmakedefine01 to > config-kscreenlocker.h.cmake You only added a cmakedefine01 for the result of the `check_symbol_exists()` call, but nothing is being done about the `check_include_file()` one, which was presumably added for you to only include `sys/prctl.h` in the code if it was found. In other words: ``` #include #if defined(HAVE_SYS_PRCTL_H) # include #endif ``` - Raphael --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126203/#review88989 --- On Dec. 1, 2015, 9:34 a.m., Martin Gräßlin wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126203/ > --- > > (Updated Dec. 1, 2015, 9:34 a.m.) > > > Review request for Plasma and Tobias Berner. > > > Repository: kscreenlocker > > > Description > --- > > Setting the PR_SET_DUMPABLE flag to 0 for the security relevant > command kcheckpass and kscreenlocker_greet. If one wants to gdb into > the running command it will result in: > > ptrace: Operation not permitted. > > For kscreenlocker_greet ptrace is permitted in testing mode. > > As root it's still possible to attach to the process. > > --- > > @Tobias: I assume this is a strong linux-ism. Is there a FreeBSD compareable > functionality? > > I'm considering to push this explicitly without an ifdef. It's a new security > feature and I want to make non-Linux systems aware of the fact that it adds a > new feature and that a replacement should be added. > > > Diffs > - > > CMakeLists.txt f48bd53cafc188f79e041518dae0769d57597c69 > config-kscreenlocker.h.cmake 2a034dee8ec21e426bc1db1d56b0ed152d3de2ca > greeter/main.cpp e4e679e7ef40b319665428281fdba5f4e0b4eb25 > kcheckpass/kcheckpass.c fd2d2215bf2199f159a121bb0ce08e7b2b254aaa > > Diff: https://git.reviewboard.kde.org/r/126203/diff/ > > > Testing > --- > > Tried to gdb into the processes: failed > Tried to gdb into kscreenlocker_greet --testing: succeeded > Tried to gdb into kscreenlocker_greet as root: succeeded > > > Thanks, > > Martin Gräßlin > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 126203: Disable ptrace for kcheckpass and the greeter
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126203/#review88989 --- CMakeLists.txt (line 25) <https://git.reviewboard.kde.org/r/126203/#comment60916> This is being checked for here but not set in `config-kscreenlocker.h.cmake`, so the header is still being included unconditionally. greeter/main.cpp (line 112) <https://git.reviewboard.kde.org/r/126203/#comment60917> The `#if` check is missing here. - Raphael Kubo da Costa On Dec. 1, 2015, 9:34 a.m., Martin Gräßlin wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126203/ > --- > > (Updated Dec. 1, 2015, 9:34 a.m.) > > > Review request for Plasma and Tobias Berner. > > > Repository: kscreenlocker > > > Description > --- > > Setting the PR_SET_DUMPABLE flag to 0 for the security relevant > command kcheckpass and kscreenlocker_greet. If one wants to gdb into > the running command it will result in: > > ptrace: Operation not permitted. > > For kscreenlocker_greet ptrace is permitted in testing mode. > > As root it's still possible to attach to the process. > > --- > > @Tobias: I assume this is a strong linux-ism. Is there a FreeBSD compareable > functionality? > > I'm considering to push this explicitly without an ifdef. It's a new security > feature and I want to make non-Linux systems aware of the fact that it adds a > new feature and that a replacement should be added. > > > Diffs > - > > CMakeLists.txt f48bd53cafc188f79e041518dae0769d57597c69 > config-kscreenlocker.h.cmake 2a034dee8ec21e426bc1db1d56b0ed152d3de2ca > greeter/main.cpp e4e679e7ef40b319665428281fdba5f4e0b4eb25 > kcheckpass/kcheckpass.c fd2d2215bf2199f159a121bb0ce08e7b2b254aaa > > Diff: https://git.reviewboard.kde.org/r/126203/diff/ > > > Testing > --- > > Tried to gdb into the processes: failed > Tried to gdb into kscreenlocker_greet --testing: succeeded > Tried to gdb into kscreenlocker_greet as root: succeeded > > > Thanks, > > Martin Gräßlin > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 126203: Disable ptrace for kcheckpass and the greeter
> On Nov. 30, 2015, 10:34 a.m., Tobias Berner wrote:
> > I think FreeBSD 10 introduced a similar interface in r277322
> > https://svnweb.freebsd.org/base?view=revision&revision=277322
> > Though it is explicitely stated to not be a "security feature".
> >
> > According to the man page of progctl
> > https://www.freebsd.org/cgi/man.cgi?query=procctl&apropos=0&sektion=2&manpath=FreeBSD+10.2-RELEASE&arch=default&format=html
> > this would be PROC_TRACE_CTL with data PROC_TRACE_CTL_DISABLE.
> >
> >
> > I will have to ask someone more familiar with that.
>
> Martin Gräßlin wrote:
> > how do I get kwin to output the debug output for the framebuffer
> backend, with qcdebug lines,
>
> Given the notes on it, that seems fine. It's not my intention to protect
> against kernel or root.
>
> Raphael Kubo da Costa wrote:
> From a non-Linux perspective, it would be good if there was a CMake check
> for `prctl()` and `PR_SET_DUMPABLE/PROC_TRACE_CTL` with a big warning in case
> none was found. The oldest supported FreeBSD release (9.x) will be supported
> until the end of 2016 and it does not have this functionality, and neither do
> {Net,DragonFly,Open}BSD, so adding the calls without additional checks will
> just create additional problems for those packaging kscreenlocker for those
> platforms, and I doubt they will be able to add a replacement for this
> without help from their kernel (and existing releases would still be broken).
>
> Martin Gräßlin wrote:
> The CMake check seems like a good idea to me - this also allows to make a
> nice HAVE_PRCTL_DUMPABABLE flag which I would prefer over QOSLINUX or
> something like that. Anybody know how to write such a check? I assume trying
> to compile and check whether it compiles?
I think so. Something like this (untested):
```
include(CheckIncludeFile)
include(CheckSymbolExists)
check_include_file("sys/prctl.h" HAVE_SYS_PRCTL_H)
check_symbol_exists(PR_SET_DUMPABLE "sys/prctl.h" HAVE_PR_SET_DUMPABLE)
check_include_file("sys/procctl.h" HAVE_SYS_PROCCTL_H)
check_symbol_exists(PROC_TRACE_CTL "sys/procctl.h" HAVE_PROC_TRACE_CTL)
if (NOT HAVE_PR_SET_DUMPABLE OR NOT HAVE_PROC_TRACE_CTL)
message(WARNING "Support for disabling ptrace(2) and core dumps via
prctl/procctl is not available. .")
endif ()
configure_file(...)
```
It's unfortunate that the naming could not be standardized, as now one needs to
check for those macros and call different functions with different arguments
depending on their values.
- Raphael
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126203/#review88939
---
On Nov. 30, 2015, 10:01 a.m., Martin Gräßlin wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126203/
> ---
>
> (Updated Nov. 30, 2015, 10:01 a.m.)
>
>
> Review request for Plasma and Tobias Berner.
>
>
> Repository: kscreenlocker
>
>
> Description
> ---
>
> Setting the PR_SET_DUMPABLE flag to 0 for the security relevant
> command kcheckpass and kscreenlocker_greet. If one wants to gdb into
> the running command it will result in:
>
> ptrace: Operation not permitted.
>
> For kscreenlocker_greet ptrace is permitted in testing mode.
>
> As root it's still possible to attach to the process.
>
> ---
>
> @Tobias: I assume this is a strong linux-ism. Is there a FreeBSD compareable
> functionality?
>
> I'm considering to push this explicitly without an ifdef. It's a new security
> feature and I want to make non-Linux systems aware of the fact that it adds a
> new feature and that a replacement should be added.
>
>
> Diffs
> -
>
> greeter/main.cpp e4e679e7ef40b319665428281fdba5f4e0b4eb25
> kcheckpass/kcheckpass.c fd2d2215bf2199f159a121bb0ce08e7b2b254aaa
>
> Diff: https://git.reviewboard.kde.org/r/126203/diff/
>
>
> Testing
> ---
>
> Tried to gdb into the processes: failed
> Tried to gdb into kscreenlocker_greet --testing: succeeded
> Tried to gdb into kscreenlocker_greet as root: succeeded
>
>
> Thanks,
>
> Martin Gräßlin
>
>
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 126203: Disable ptrace for kcheckpass and the greeter
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126203/#review88953 --- greeter/main.cpp (line 32) <https://git.reviewboard.kde.org/r/126203/#comment60901> Both `prctl()` and `PR_SET_DUMPABLE` seem to be defined in `sys/prctl.h` on Linux, so this include should not be necessary. kcheckpass/kcheckpass.c (line 61) <https://git.reviewboard.kde.org/r/126203/#comment60902> Ditto here. - Raphael Kubo da Costa On Nov. 30, 2015, 10:01 a.m., Martin Gräßlin wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126203/ > --- > > (Updated Nov. 30, 2015, 10:01 a.m.) > > > Review request for Plasma and Tobias Berner. > > > Repository: kscreenlocker > > > Description > --- > > Setting the PR_SET_DUMPABLE flag to 0 for the security relevant > command kcheckpass and kscreenlocker_greet. If one wants to gdb into > the running command it will result in: > > ptrace: Operation not permitted. > > For kscreenlocker_greet ptrace is permitted in testing mode. > > As root it's still possible to attach to the process. > > --- > > @Tobias: I assume this is a strong linux-ism. Is there a FreeBSD compareable > functionality? > > I'm considering to push this explicitly without an ifdef. It's a new security > feature and I want to make non-Linux systems aware of the fact that it adds a > new feature and that a replacement should be added. > > > Diffs > - > > greeter/main.cpp e4e679e7ef40b319665428281fdba5f4e0b4eb25 > kcheckpass/kcheckpass.c fd2d2215bf2199f159a121bb0ce08e7b2b254aaa > > Diff: https://git.reviewboard.kde.org/r/126203/diff/ > > > Testing > --- > > Tried to gdb into the processes: failed > Tried to gdb into kscreenlocker_greet --testing: succeeded > Tried to gdb into kscreenlocker_greet as root: succeeded > > > Thanks, > > Martin Gräßlin > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 126203: Disable ptrace for kcheckpass and the greeter
> On Nov. 30, 2015, 10:34 a.m., Tobias Berner wrote:
> > I think FreeBSD 10 introduced a similar interface in r277322
> > https://svnweb.freebsd.org/base?view=revision&revision=277322
> > Though it is explicitely stated to not be a "security feature".
> >
> > According to the man page of progctl
> > https://www.freebsd.org/cgi/man.cgi?query=procctl&apropos=0&sektion=2&manpath=FreeBSD+10.2-RELEASE&arch=default&format=html
> > this would be PROC_TRACE_CTL with data PROC_TRACE_CTL_DISABLE.
> >
> >
> > I will have to ask someone more familiar with that.
>
> Martin Gräßlin wrote:
> > how do I get kwin to output the debug output for the framebuffer
> backend, with qcdebug lines,
>
> Given the notes on it, that seems fine. It's not my intention to protect
> against kernel or root.
>From a non-Linux perspective, it would be good if there was a CMake check for
>`prctl()` and `PR_SET_DUMPABLE/PROC_TRACE_CTL` with a big warning in case none
>was found. The oldest supported FreeBSD release (9.x) will be supported until
>the end of 2016 and it does not have this functionality, and neither do
>{Net,DragonFly,Open}BSD, so adding the calls without additional checks will
>just create additional problems for those packaging kscreenlocker for those
>platforms, and I doubt they will be able to add a replacement for this without
>help from their kernel (and existing releases would still be broken).
- Raphael
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126203/#review88939
---
On Nov. 30, 2015, 10:01 a.m., Martin Gräßlin wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126203/
> ---
>
> (Updated Nov. 30, 2015, 10:01 a.m.)
>
>
> Review request for Plasma and Tobias Berner.
>
>
> Repository: kscreenlocker
>
>
> Description
> ---
>
> Setting the PR_SET_DUMPABLE flag to 0 for the security relevant
> command kcheckpass and kscreenlocker_greet. If one wants to gdb into
> the running command it will result in:
>
> ptrace: Operation not permitted.
>
> For kscreenlocker_greet ptrace is permitted in testing mode.
>
> As root it's still possible to attach to the process.
>
> ---
>
> @Tobias: I assume this is a strong linux-ism. Is there a FreeBSD compareable
> functionality?
>
> I'm considering to push this explicitly without an ifdef. It's a new security
> feature and I want to make non-Linux systems aware of the fact that it adds a
> new feature and that a replacement should be added.
>
>
> Diffs
> -
>
> greeter/main.cpp e4e679e7ef40b319665428281fdba5f4e0b4eb25
> kcheckpass/kcheckpass.c fd2d2215bf2199f159a121bb0ce08e7b2b254aaa
>
> Diff: https://git.reviewboard.kde.org/r/126203/diff/
>
>
> Testing
> ---
>
> Tried to gdb into the processes: failed
> Tried to gdb into kscreenlocker_greet --testing: succeeded
> Tried to gdb into kscreenlocker_greet as root: succeeded
>
>
> Thanks,
>
> Martin Gräßlin
>
>
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 121762: Fix build on FreeBSD
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121762/#review74261 --- Ship it! - Raphael Kubo da Costa On Jan. 18, 2015, 10 p.m., Alex Richardson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/121762/ > --- > > (Updated Jan. 18, 2015, 10 p.m.) > > > Review request for Plasma and Raphael Kubo da Costa. > > > Repository: plasma-desktop > > > Description > --- > > 5 Commits: > > --- > > Fix compilation of geometry_parser.cpp with clang 3.4 > > Apparently the default instantiation depth of 256 (with clang 3.4) is not > enough to compile geometry_parser.cpp. It produces the following error: > > /usr/local/include/boost/type_traits/is_reference.hpp:38:62: fatal error: > recursive template instantiation exceeded maximum depth of 256 > BOOST_TT_AUX_BOOL_TRAIT_DEF1(is_reference,T,::boost::detail::is_reference_impl::value) > ^ > /usr/local/include/boost/type_traits/detail/bool_trait_def.hpp:69:30: note: > expanded from macro 'BOOST_TT_AUX_BOOL_TRAIT_DEF1' > BOOST_TT_AUX_BOOL_C_BASE(C) \ > ^ > /usr/local/include/boost/type_traits/detail/bool_trait_def.hpp:63:81: note: > expanded from macro 'BOOST_TT_AUX_BOOL_C_BASE' > > And after that a few hundred lines of template instantiation details. > Rasing the limit to 512 allows successfully compiling these files. > > --- > > > Fix build of kbpreviewframe.cpp with clang > > Clang doesn't allow variable length arrays of non-POD type > > --- > > Add missing includes of cmath and cstdlib > > --- > > Fix build of kapplymousetheme if X11 isn't installed to /usr > > --- > > Use a proper CMake find module for libcanberra instead of pkg_check_modules > > This fixes the build on e.g. FreeBSD where libcanberra is not in /usr/lib > but /usr/local/lib so that the linker complains that it cannot find > -lcanberra since the CMake variable from pkg_check_modules does not contain > an absolute path. > > The CMake module was copied from the KMix repository. If there are any more > users of libcanberra it might make sense to add this to extra-cmake-modules > > > Diffs > - > > kcms/keyboard/CMakeLists.txt 3db5dc3f938bbe25259067783bda4fdd882f60f6 > kcms/keyboard/kcmmisc.cpp 9aed7de2a8a63aa546dc9484eea0edadcc64ec66 > kcms/keyboard/preview/kbpreviewframe.cpp > 9735eb02ac6875bb3905fb0bf8183a9d97e0e3e4 > kcms/keyboard/tests/CMakeLists.txt bd62c7d281766bc5c04b0b1e933861d258241cf7 > kcms/phonon/CMakeLists.txt 8f964e2041cc812bf70f6a48e2915427f088b990 > cmake/modules/FindCanberra.cmake PRE-CREATION > containments/folder/plugin/positioner.cpp > c24827197bf20b4cb55a6b885f023074f179159c > kcms/input/CMakeLists.txt a2b72a45a3b694b7fa367abf33d0752b0dc9734b > > Diff: https://git.reviewboard.kde.org/r/121762/diff/ > > > Testing > --- > > compiles > > > Thanks, > > Alex Richardson > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 121763: Fix build on FreeBSD
> On Jan. 2, 2015, 10:24 a.m., Martin Gräßlin wrote: > > Modules/info/CMakeLists.txt, lines 28-30 > > <https://git.reviewboard.kde.org/r/121763/diff/1/?file=337463#file337463line28> > > > > just out of interest: is devinfo a FreeBSD only library or is it of > > interest for more non-Linux variants? It's a library that originated on FreeBSD (http://mdoc.su/f/devinfo.3) and as far as I can tell only DragonFlyBSD also uses/ships it, so I wouldn't worry about making the check more generic for now (I don't use DragonFly myself, but I remember some checks used to work for both OSes). If someone from DragonFlyBSD complains at some point, we can turn this into a proper Find module. - Raphael --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121763/#review72900 --- On Dec. 30, 2014, 8:48 p.m., Alex Richardson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/121763/ > --- > > (Updated Dec. 30, 2014, 8:48 p.m.) > > > Review request for Plasma and Raphael Kubo da Costa. > > > Repository: kinfocenter > > > Description > --- > > We can't call the KCM devinfo there since we need to link against -ldevinfo > and CMake would try to link to the KCM instead of the library in that case. > > > Diffs > - > > Modules/devinfo/CMakeLists.txt 2395ce3dc83080e959cbfa9f97724218cdff6bd9 > Modules/devinfo/devinfo.desktop 1bc98a06b9a567ee45c51c7f25ee5ad6b43750d7 > Modules/info/CMakeLists.txt 7b0e0affd13d6b556749f4a350012f27fb43ae0b > Modules/pci/CMakeLists.txt 5b2b30a0c3791a8add00a380e61469a96cd66ae1 > > Diff: https://git.reviewboard.kde.org/r/121763/diff/ > > > Testing > --- > > > Thanks, > > Alex Richardson > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 121762: Fix build on FreeBSD
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121762/#review72802 --- kcms/phonon/CMakeLists.txt <https://git.reviewboard.kde.org/r/121762/#comment50700> You shouldn't need this anymore. - Raphael Kubo da Costa On Dec. 30, 2014, 11:37 p.m., Alex Richardson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/121762/ > --- > > (Updated Dec. 30, 2014, 11:37 p.m.) > > > Review request for Plasma and Raphael Kubo da Costa. > > > Repository: plasma-desktop > > > Description > --- > > 5 Commits: > > --- > > Fix compilation of geometry_parser.cpp with clang 3.4 > > Apparently the default instantiation depth of 256 (with clang 3.4) is not > enough to compile geometry_parser.cpp. It produces the following error: > > /usr/local/include/boost/type_traits/is_reference.hpp:38:62: fatal error: > recursive template instantiation exceeded maximum depth of 256 > BOOST_TT_AUX_BOOL_TRAIT_DEF1(is_reference,T,::boost::detail::is_reference_impl::value) > ^ > /usr/local/include/boost/type_traits/detail/bool_trait_def.hpp:69:30: note: > expanded from macro 'BOOST_TT_AUX_BOOL_TRAIT_DEF1' > BOOST_TT_AUX_BOOL_C_BASE(C) \ > ^ > /usr/local/include/boost/type_traits/detail/bool_trait_def.hpp:63:81: note: > expanded from macro 'BOOST_TT_AUX_BOOL_C_BASE' > > And after that a few hundred lines of template instantiation details. > Rasing the limit to 512 allows successfully compiling these files. > > --- > > > Fix build of kbpreviewframe.cpp with clang > > Clang doesn't allow variable length arrays of non-POD type > > --- > > Add missing includes of cmath and cstdlib > > --- > > Fix build of kapplymousetheme if X11 isn't installed to /usr > > --- > > Use a proper CMake find module for libcanberra instead of pkg_check_modules > > This fixes the build on e.g. FreeBSD where libcanberra is not in /usr/lib > but /usr/local/lib so that the linker complains that it cannot find > -lcanberra since the CMake variable from pkg_check_modules does not contain > an absolute path. > > The CMake module was copied from the KMix repository. If there are any more > users of libcanberra it might make sense to add this to extra-cmake-modules > > > Diffs > - > > cmake/modules/FindCanberra.cmake PRE-CREATION > containments/folder/plugin/positioner.cpp > c24827197bf20b4cb55a6b885f023074f179159c > kcms/input/CMakeLists.txt a2b72a45a3b694b7fa367abf33d0752b0dc9734b > kcms/keyboard/CMakeLists.txt 3db5dc3f938bbe25259067783bda4fdd882f60f6 > kcms/keyboard/kcmmisc.cpp 9aed7de2a8a63aa546dc9484eea0edadcc64ec66 > kcms/keyboard/preview/kbpreviewframe.cpp > 9735eb02ac6875bb3905fb0bf8183a9d97e0e3e4 > kcms/keyboard/tests/CMakeLists.txt bd62c7d281766bc5c04b0b1e933861d258241cf7 > kcms/phonon/CMakeLists.txt 8f964e2041cc812bf70f6a48e2915427f088b990 > > Diff: https://git.reviewboard.kde.org/r/121762/diff/ > > > Testing > --- > > compiles > > > Thanks, > > Alex Richardson > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 121762: Fix build on FreeBSD
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121762/#review72792 --- kcms/keyboard/CMakeLists.txt <https://git.reviewboard.kde.org/r/121762/#comment50691> `-fexceptions` is already set above. kcms/keyboard/flags.h <https://git.reviewboard.kde.org/r/121762/#comment50690> I'd move the `struct` forward declarations after the `class` ones here and in the other places that have been changed. kcms/keyboard/tests/CMakeLists.txt <https://git.reviewboard.kde.org/r/121762/#comment50692> `-fexceptions` is already set by the `keyboard_preview_unit_tests` macro, which also allows you to remove the commented out line below. kcms/phonon/CMakeLists.txt <https://git.reviewboard.kde.org/r/121762/#comment50694> In this case, I think it pays off to write a proper find module instead of the `pkg_check_module` call we currently have -- in fact, it appears I wrote one a while ago for KMix that's still in the repository. In addition to the points I mentioned inline, it would be good if in your final commit messages you pasted the actual errors clang is throwing at you for posterity. - Raphael Kubo da Costa On Dec. 30, 2014, 8:35 p.m., Alex Richardson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/121762/ > --- > > (Updated Dec. 30, 2014, 8:35 p.m.) > > > Review request for Plasma and Raphael Kubo da Costa. > > > Repository: plasma-desktop > > > Description > --- > > Fix build with clang 3.4 > > Clang doesn't allow variable length arrays of non-POD data types > Also it seems that the default template instantion depth is not > enough to compile geometry_parser.cpp > > -- > > Add missing includes of cmath and cstdlib > > - > > Fix clang warning -Wmismatched-tags > > - > > > Fix build of geometry_parser test with clang > > We need to increase the template instantiation depth > > - > > Fix more -Wmismatched-tag warnings > > > > Fix build of kapplymousetheme if X11 isn't installed to /usr > > > > > Fix build of phonon KCM if libcanberra is not in /usr > > > Diffs > - > > kcms/keyboard/bindings.h d59790752ced2ec8fd77269f75012e74ccda4cc4 > kcms/keyboard/flags.h 3957a983f06044e8f5ed130afc4774f8c644ed8c > kcms/keyboard/kcm_add_layout_dialog.h > a2c0ac5ebc264d69555063668cb68be5255b3030 > kcms/keyboard/kcm_keyboard.h c88fe3dd8540df07eb886b3119dfbe30fab2844b > kcms/keyboard/kcm_keyboard_widget.h > 5994ea4815d6398afaf6765b77a2bc8cd349f780 > kcms/keyboard/kcm_view_models.h 872e26185248c6dfb8d8808115865ae18596cbea > kcms/keyboard/kcmmisc.cpp 9aed7de2a8a63aa546dc9484eea0edadcc64ec66 > containments/folder/plugin/positioner.cpp > c24827197bf20b4cb55a6b885f023074f179159c > kcms/input/CMakeLists.txt a2b72a45a3b694b7fa367abf33d0752b0dc9734b > kcms/keyboard/CMakeLists.txt 3db5dc3f938bbe25259067783bda4fdd882f60f6 > kcms/keyboard/keyboard_daemon.h 4d7587f9113edea31dd07f23d658818941731df2 > kcms/keyboard/layout_tray_icon.h 8338bf26307627747fb434ae8e0903050142aef4 > kcms/keyboard/layouts_menu.h db2f3ff5844e16340ad3ca6102e6b1c4866ad8db > kcms/keyboard/preview/kbpreviewframe.cpp > 9735eb02ac6875bb3905fb0bf8183a9d97e0e3e4 > kcms/keyboard/tests/CMakeLists.txt bd62c7d281766bc5c04b0b1e933861d258241cf7 > kcms/keyboard/xkb_helper.h d3dab1d9b05784ae463db44ab941119e5b214986 > kcms/phonon/CMakeLists.txt 8f964e2041cc812bf70f6a48e2915427f088b990 > > Diff: https://git.reviewboard.kde.org/r/121762/diff/ > > > Testing > --- > > compiles > > > Thanks, > > Alex Richardson > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 121763: Fix build on FreeBSD
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121763/#review72791 --- Looks fine to me, not sure if someone else needs to approve. - Raphael Kubo da Costa On Dec. 30, 2014, 8:48 p.m., Alex Richardson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/121763/ > --- > > (Updated Dec. 30, 2014, 8:48 p.m.) > > > Review request for Plasma and Raphael Kubo da Costa. > > > Repository: kinfocenter > > > Description > --- > > We can't call the KCM devinfo there since we need to link against -ldevinfo > and CMake would try to link to the KCM instead of the library in that case. > > > Diffs > - > > Modules/devinfo/CMakeLists.txt 2395ce3dc83080e959cbfa9f97724218cdff6bd9 > Modules/devinfo/devinfo.desktop 1bc98a06b9a567ee45c51c7f25ee5ad6b43750d7 > Modules/info/CMakeLists.txt 7b0e0affd13d6b556749f4a350012f27fb43ae0b > Modules/pci/CMakeLists.txt 5b2b30a0c3791a8add00a380e61469a96cd66ae1 > > Diff: https://git.reviewboard.kde.org/r/121763/diff/ > > > Testing > --- > > > Thanks, > > Alex Richardson > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 121761: Fix build on FreeBSD
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121761/#review72789 --- I'd keep the `errno.h` includes alphabetically sorted, and commit each of those changes (errno, then X11 dependencies etc) separately. Other than that, looks good to me. - Raphael Kubo da Costa On Dec. 30, 2014, 8:33 p.m., Alex Richardson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/121761/ > --- > > (Updated Dec. 30, 2014, 8:33 p.m.) > > > Review request for Plasma and Raphael Kubo da Costa. > > > Repository: plasma-workspace > > > Description > --- > > Add missing errno.h include > > Wrap Linux-only signals in #ifdef Q_OS_LINUX > > Fix build with X11 not installed to /usr > > > Diffs > - > > drkonqi/detachedprocessmonitor.cpp 85a87874c6e9ce47856a01195b42aef9f3a4991a > drkonqi/systeminformation.cpp 8f5fc7fe789a4bbe23f178e7e940ba1d1a1b59db > ksmserver/CMakeLists.txt 84a8aa393dfb6ed4671094d1fccbb3c79c53f9af > ksmserver/screenlocker/greeter/authenticator.cpp > ad60f0bd0076cd9c8c3875f13dc92b5da253bb1a > ksmserver/screenlocker/greeter/autotests/killtest.cpp > 6f2ef114459b5d641e357f3a817b82e7af2e72a3 > libkworkspace/CMakeLists.txt 53ce6108bd91f87108206fca02ac303dadf069e1 > > Diff: https://git.reviewboard.kde.org/r/121761/diff/ > > > Testing > --- > > compiles, still compiles on linux > > > Thanks, > > Alex Richardson > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119651: kactivities: Do not load plugins marked as EnabledByDefault=false.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119651/ --- (Updated Aug. 7, 2014, 6:13 p.m.) Status -- This change has been marked as submitted. Review request for Plasma and Ivan Čukić. Bugs: 305529 http://bugs.kde.org/show_bug.cgi?id=305529 Repository: kactivities Description --- Follow up to dc0fc3f: just setting X-KDE-PluginInfo-EnabledByDefault to false in the activity ranking plugin is not enough, as Application will still try to load it. Diffs - src/service/Application.cpp ac4978b4ee59d45a18687ccdca7c21e6bcd08854 Diff: https://git.reviewboard.kde.org/r/119651/diff/ Testing --- kactivitymanagerd stops crashing on startup when loading the activity ranking plugin. Thanks, Raphael Kubo da Costa ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request 119651: kactivities: Do not load plugins marked as EnabledByDefault=false.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119651/ --- Review request for Plasma and Ivan Čukić. Bugs: 305529 http://bugs.kde.org/show_bug.cgi?id=305529 Repository: kactivities Description --- Follow up to dc0fc3f: just setting X-KDE-PluginInfo-EnabledByDefault to false in the activity ranking plugin is not enough, as Application will still try to load it. Diffs - src/service/Application.cpp ac4978b4ee59d45a18687ccdca7c21e6bcd08854 Diff: https://git.reviewboard.kde.org/r/119651/diff/ Testing --- kactivitymanagerd stops crashing on startup when loading the activity ranking plugin. Thanks, Raphael Kubo da Costa ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119025: Actually pass IBUS_DEFINITIONS when compiling ibus-panel
On July 19, 2014, 12:17 a.m., Vadim Zhukov wrote: > > (As a general note, for build system related stuff like this you can also > > try including the "buildsystem" group, which can be more responsive at > > times) > > > > > The ibus-panel can't build on OpenBSD because some required definitions > > > obtained from pkgconfig file are not used. > > > 1. IBUS_DEFINITIONS is not used in ibus-panel/CMakeLists.txt > > > > Can you post the error you get here? I've tried building kimtoy here on > > FreeBSD expecting to hit the same issue(s), but it all went along just fine. > > > > > 2. IBUS_DEFINITIONS is not saved in cache, and thus isn't available at > > > compile time > > > > This doesn't make much sense; all values found at configuration time in > > CMake are then used to generate your make/ninja/whatever files, regardless > > of whether they are cached or not. > > Vadim Zhukov wrote: > Raphael, thank you for your input! > > The issue was caused by the fact that X includes are placed in the > /usr/X11R6/include directory on OpenBSD. This catalog is mentioned in > PC_IBUS_DEFINITIONS variable but isn't propagated to the caller of > find_package(IBus). Yes, I was wrong: the CACHE part may and should be > omitted. This patch was added by me a long time ago (7 Feb 2012 according to > git log; guess the KDE version used then), when I was much less expirienced > in CMake... I've started a massive push of OpenBSD local patches upstream > recently during OpenBSD hackathon, when I got time for such cleanup. > > At the present time, the /usr/X11R6/include gets to the > include_directories() from another place, so the patch isn't required at all. > But, still, the PC_IBUS_DEFINITIONS should be respected, IMHO. What do you > think? > > Please note that FreeBSD and OpenBSD and quiet different. So you can't > test on one OS instead of another. > The issue was caused by the fact that X includes are placed in the > /usr/X11R6/include directory on OpenBSD. This catalog is mentioned in > PC_IBUS_DEFINITIONS variable but isn't propagated to the caller of > find_package(IBus). > [...] > At the present time, the /usr/X11R6/include gets to the include_directories() > from another place, so the patch isn't required at all. But, still, the > PC_IBUS_DEFINITIONS should be respected, IMHO. What do you think? While that is not wrong, the approach we normally take with CMake is that pkg-config's presence is optional, and we don't depend on its output to be able to build a module. In practice, this means one should look for IBus and X11 separately, as well as add their compiler flags/link against them independently. If kimtoy already does that, I just wouldn't make any change. > Please note that FreeBSD and OpenBSD and quiet different. So you can't test > on one OS instead of another. You don't need to preach to the choir :-) I'm well aware of the differences between them, my point is that in many cases packaging problems in one also impact the other (missing includes because people assume all software is in `/usr`, reliance on non-POSIX features without checking for their availability etc). - Raphael --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119025/#review62669 --- On July 19, 2014, 9:43 p.m., Vadim Zhukov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/119025/ > --- > > (Updated July 19, 2014, 9:43 p.m.) > > > Review request for kde-workspace, Plasma and Hui Ni. > > > Repository: kimtoy > > > Description > --- > > The ibus-panel can't build on OpenBSD because some required definitions > obtained from pkgconfig file are not used. This happens due to the following > reasons: > > 1. IBUS_DEFINITIONS is not used in ibus-panel/CMakeLists.txt > 2. IBUS_DEFINITIONS is not saved in cache, and thus isn't available at > compile time > > This patch resolves both issues and makes ibus-panel compile on OpenBSD. > > (I found no suitable review group and therefore used "plasma" instead, as it > was in "plasma-addons" previously; please, feel free to correct me if I'm > wrong and sorry for any possible inconvenience) > > ((as there was no feedback for more than a week, I've added "kde-workspace" > group to list of reviewers, too)) > > > Diffs > - > > ibus-panel/CMakeLists.txt 3a1ee49 > > Diff: https://git.reviewboard.kde.org/r/119025/diff/ > > > Testing > --- > > OpenBSD/i386-CURRENT, KDE 4.13 (it doesn't have kimtoy package, of course, > but the code is same) > > > Thanks, > > Vadim Zhukov > > ___ Plasma-devel mailing list Plasma-devel@kde.org
Re: Review Request 119025: Actually pass IBUS_DEFINITIONS when compiling ibus-panel
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119025/#review62669 --- cmake/FindIBus.cmake <https://git.reviewboard.kde.org/r/119025/#comment43439> Typo: paramaters (As a general note, for build system related stuff like this you can also try including the "buildsystem" group, which can be more responsive at times) > The ibus-panel can't build on OpenBSD because some required definitions > obtained from pkgconfig file are not used. > 1. IBUS_DEFINITIONS is not used in ibus-panel/CMakeLists.txt Can you post the error you get here? I've tried building kimtoy here on FreeBSD expecting to hit the same issue(s), but it all went along just fine. > 2. IBUS_DEFINITIONS is not saved in cache, and thus isn't available at > compile time This doesn't make much sense; all values found at configuration time in CMake are then used to generate your make/ninja/whatever files, regardless of whether they are cached or not. - Raphael Kubo da Costa On July 12, 2014, 4:12 p.m., Vadim Zhukov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/119025/ > --- > > (Updated July 12, 2014, 4:12 p.m.) > > > Review request for kde-workspace, Plasma and Hui Ni. > > > Repository: kimtoy > > > Description > --- > > The ibus-panel can't build on OpenBSD because some required definitions > obtained from pkgconfig file are not used. This happens due to the following > reasons: > > 1. IBUS_DEFINITIONS is not used in ibus-panel/CMakeLists.txt > 2. IBUS_DEFINITIONS is not saved in cache, and thus isn't available at > compile time > > This patch resolves both issues and makes ibus-panel compile on OpenBSD. > > (I found no suitable review group and therefore used "plasma" instead, as it > was in "plasma-addons" previously; please, feel free to correct me if I'm > wrong and sorry for any possible inconvenience) > > ((as there was no feedback for more than a week, I've added "kde-workspace" > group to list of reviewers, too)) > > > Diffs > - > > cmake/FindIBus.cmake 8250c49 > ibus-panel/CMakeLists.txt 3a1ee49 > > Diff: https://git.reviewboard.kde.org/r/119025/diff/ > > > Testing > --- > > OpenBSD/i386-CURRENT, KDE 4.13 (it doesn't have kimtoy package, of course, > but the code is same) > > > Thanks, > > Vadim Zhukov > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 119355: Change use of MacroPushRequiredVars to CMakePushCheckState
> On July 18, 2014, 2:57 p.m., Aleix Pol Gonzalez wrote: > > Is it available in 2.8.13? Otherwise we'd have to bump the dependency > > requirement to 3.0. It's actually been available since 2.8.6: http://cmake.org/gitweb?p=cmake.git;a=commit;h=1325260a664577a1356f3034ed5f110d45f6a6d8 - Raphael --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119355/#review62649 --- On July 18, 2014, 2:43 p.m., Tobias Berner wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/119355/ > --- > > (Updated July 18, 2014, 2:43 p.m.) > > > Review request for Plasma and Raphael Kubo da Costa. > > > Repository: plasma-workspace > > > Description > --- > > MacroPushRequiredVars has been merged into CMake, and has been renamed to > CMakePushCheckState > > > Diffs > - > > cmake/UnixAuth.cmake 904f0cb > > Diff: https://git.reviewboard.kde.org/r/119355/diff/ > > > Testing > --- > > Tested locally. > > > Thanks, > > Tobias Berner > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: ksplashx: Adjust erroneous CMake calls.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105468/#review16173 --- Ping? - Raphael Kubo da Costa On July 7, 2012, 3:07 a.m., Raphael Kubo da Costa wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/105468/ > --- > > (Updated July 7, 2012, 3:07 a.m.) > > > Review request for Build System and Plasma. > > > Description > --- > > ksplashx: Adjust erroneous CMake calls. > > Since its inception, ksplashx's CMakeLists.txt was using `include(FindFOO)' > instead of `find_package(FOO)' to find libpng and libjpeg. While it has indeed > worked so far, it is not the right way to find dependencies. > > Adjust this by correctly looking for libpng and libjpeg via find_package(). > The > calls are in the top-level CMakeLists.txt since putting all find_package() and > macro_optional_find_package() calls in the same file seems to appease some > packagers. > > While here, also add libjpeg's include directory to the compiler's include > path > and use a FindPNG variable that's not internal to do the same for libpng. > > > Diffs > - > > CMakeLists.txt ded5a2995b3d25c1c7c891afe93795fb0dfcfb87 > ksplash/ksplashx/CMakeLists.txt 65447939dc9ac17c69f1e94afe935b37c7dceba4 > > Diff: http://git.reviewboard.kde.org/r/105468/diff/ > > > Testing > --- > > kde-workspace built fine both before and after this change > > > Thanks, > > Raphael Kubo da Costa > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request: ksplashx: Adjust erroneous CMake calls.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105468/ --- Review request for Build System and Plasma. Description --- ksplashx: Adjust erroneous CMake calls. Since its inception, ksplashx's CMakeLists.txt was using `include(FindFOO)' instead of `find_package(FOO)' to find libpng and libjpeg. While it has indeed worked so far, it is not the right way to find dependencies. Adjust this by correctly looking for libpng and libjpeg via find_package(). The calls are in the top-level CMakeLists.txt since putting all find_package() and macro_optional_find_package() calls in the same file seems to appease some packagers. While here, also add libjpeg's include directory to the compiler's include path and use a FindPNG variable that's not internal to do the same for libpng. Diffs - CMakeLists.txt ded5a2995b3d25c1c7c891afe93795fb0dfcfb87 ksplash/ksplashx/CMakeLists.txt 65447939dc9ac17c69f1e94afe935b37c7dceba4 Diff: http://git.reviewboard.kde.org/r/105468/diff/ Testing --- kde-workspace built fine both before and after this change Thanks, Raphael Kubo da Costa ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Do not check if a wallpaper is a valid Plasma::Package
> On May 6, 2011, 7:28 a.m., Aaron J. Seigo wrote:
> > this is the wrong fix. well, it's not really a fix at all, it just allows
> > any broken thing to go through. removing error checking is rarely the
> > answer ;) clearly, it is failing for single image file wallpapers, which
> > means Package::isValid() is thinking the package is missing things.
> >
> > if you apply this simple patch in kdelibs/plasma, does it fix things for
> > you:
> >
> > diff --git a/plasma/private/packages.cpp b/plasma/private/packages.cpp
> > index 5e064d9..f24a283 100644
> > --- a/plasma/private/packages.cpp
> > +++ b/plasma/private/packages.cpp
> > @@ -266,6 +266,7 @@ void WallpaperPackage::pathChanged()
> > findBestPaper();
> > } else {
> > // dirty trick to support having a file passed in instead of a
> > directory
> > +removeDefinition("images");
> > addFileDefinition("preferred", info.fileName(), i18n("Recommended
> > wallpaper file"));
> > setContentsPrefixPaths(QStringList());
> > //kDebug() << "changing" << path() << "to" << info.path();
> >
>
> Aaron J. Seigo wrote:
> ok, i just committed a slightly better fix than the above path to master
> to kdelibs/plasma/private/packages.cp. can you check if that works for you
> and if so i'll backport it.
Yep, it works fine now, thanks.
I'm closing the review as discarded now.
- Raphael
-----------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/101302/#review3150
---
On May 6, 2011, 5:17 a.m., Raphael Kubo da Costa wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101302/
> ---
>
> (Updated May 6, 2011, 5:17 a.m.)
>
>
> Review request for Plasma.
>
>
> Summary
> ---
>
> Do not check if the created Plasma::Package is valid.
>
> Wallpapers consisting of a single image (without a .desktop metadata or
> anything else), usually obtained via GHNS, are not valid packages, but are
> valid wallpapers.
>
> Commit b60136e, besides moving the loading code to a separate thread, also
> started checking if a given found path was also a valid package, which kept
> any wallpaper downloaded via the "get new wallpapers" button from being
> listed at all.
>
>
> This addresses bug 269587.
> http://bugs.kde.org/show_bug.cgi?id=269587
>
>
> Diffs
> -
>
> plasma/generic/wallpapers/image/backgroundlistmodel.cpp 3c92024
>
> Diff: http://git.reviewboard.kde.org/r/101302/diff
>
>
> Testing
> ---
>
> All downloaded wallpapers are listed and selectable again.
>
>
> Thanks,
>
> Raphael
>
>
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request: Do not check if a wallpaper is a valid Plasma::Package
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101302/ --- Review request for Plasma. Summary --- Do not check if the created Plasma::Package is valid. Wallpapers consisting of a single image (without a .desktop metadata or anything else), usually obtained via GHNS, are not valid packages, but are valid wallpapers. Commit b60136e, besides moving the loading code to a separate thread, also started checking if a given found path was also a valid package, which kept any wallpaper downloaded via the "get new wallpapers" button from being listed at all. This addresses bug 269587. http://bugs.kde.org/show_bug.cgi?id=269587 Diffs - plasma/generic/wallpapers/image/backgroundlistmodel.cpp 3c92024 Diff: http://git.reviewboard.kde.org/r/101302/diff Testing --- All downloaded wallpapers are listed and selectable again. Thanks, Raphael ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
KDE/kdebase/workspace/plasma/netbook/shell
SVN commit 1157732 by rkcosta:
Fix build with gcc < 4.3.0.
The issue was brought up on the kde-packager/release-team mailing lists[1], and
it turns out to be a bug in gcc itself related to an iffy part of the c++
spec[2][3].
Thanks a lot to SadEagle (Maksim Orlovich) for helping chase the cause of this
bug on IRC.
Will backport to the 4.5 branch.
CCMAIL: plasma-devel@kde.org
[1] http://lists.kde.org/?l=kde-release-team&m=128051849414809&w=2
[2] http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36490
[3] http://gcc.gnu.org/bugzilla/show_bug.cgi?id=25950
M +6 -1 netcorona.cpp
--- trunk/KDE/kdebase/workspace/plasma/netbook/shell/netcorona.cpp
#1157731:1157732
@@ -87,8 +87,13 @@
QString defaultConfig = KStandardDirs::locate("appdata",
"plasma-default-layoutrc");
if (!defaultConfig.isEmpty()) {
kDebug() << "attempting to load the default layout from:" <<
defaultConfig;
-importLayout(KConfig(defaultConfig));
+// gcc bug 36490: KConfig's copy constructor is private, so passing it
as a
+// temporary to importLayout, ie importLayout(KConfig(defaultConfig))
fails
+// on gcc < 4.3.0
+KConfig c(defaultConfig);
+importLayout(c);
+
return;
}
}
___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
