wojnilowicz marked an inline comment as done. wojnilowicz added a comment.
Thanks for looking into that matter. In D29258#674489 <https://phabricator.kde.org/D29258#674489>, @brute4s99 wrote: > I still do not understand the utility of this patch. What do you hope to fix by this patch exactly? I hope to fix KNotifications build on MSYS2 where there is no possibility to compile SnoreToast. It was possible to compile KNotifications without neither compiling SnoreToast, nor downloading its binaries before. This patch brings that possibility while allowing to build with SnoreToast enabled as well. >> SnoreToast fails to build on MSYS2 due to missing >> which apparently is not available for this compiler. > > which compiler are you referring to here? MinGW from MSYS2. >> This patch changes dependency on SnoreToast from required to optional. >> As a result, it allows to actually build KNotifications on MSYS2. > > why do you want to build KNotifications on MSYS2? I have my own build of KF5 libraries which I use to package app that I forked. > More inline comments follow, please take a look. INLINE COMMENTS > brute4s99 wrote in CMakeLists.txt:76 > why do we need Qt5Network for all kinds of Windows builds? We don't need it, but if it would be found together with SnoreToast then SnoreToast would be enabled by default in MSVC and MSYS2 builds. > brute4s99 wrote in CMakeLists.txt:79 > the option should be MSYS specific rather than being SnoreToast specific. I think it should be SnoreToast specific because if you download the binary of SnoreToast then you could possibly compile KNotifications with it on MSYS2. REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D29258 To: wojnilowicz, broulik, brute4s99, vonreth Cc: vonreth, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
