Don't use notifybysnore.h on MSYS2
ClosedPublic

Authored by wojnilowicz on Apr 28 2020, 3:36 PM.

Details

Summary

SnoreToast fails to build on MSYS2 due to missing
which apparently is not available for this compiler.

This patch changes dependency on SnoreToast from required to optional.
As a result, it allows to actually build KNotifications on MSYS2.

Test Plan

Built application on Windows, which links to KNotifications.

Diff Detail

Repository
R289 KNotifications
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 26094
Build 26112: arc lint + arc unit
wojnilowicz requested review of this revision.Apr 28 2020, 3:36 PM
wojnilowicz created this revision.

SnoreToast fails to build on MSYS2 due to missing
which apparently is not available for this compiler.

I'm sorry, missing what exactly?

vonreth requested changes to this revision.Apr 28 2020, 10:39 PM
vonreth added a subscriber: vonreth.

Well the code is header only, so gcc can use it too.
Everything needed is to pull in a binary, that's not ideal but I guess its better than no notifications on Windows.

This revision now requires changes to proceed.Apr 28 2020, 10:39 PM

I see. I've updated the patch. Linking to downloaded library is now possible, and there is a fallback if no library is present.

SnoreToast fails to build on MSYS2 due to missing
which apparently is not available for this compiler.

I'm sorry, missing what exactly?

Missing "wrl\wrappers\corewrappers.h".

Could anyone review my patch again?

I still do not understand the utility of this patch. What do you hope to fix by this patch exactly?

SnoreToast fails to build on MSYS2 due to missing
which apparently is not available for this compiler.

which compiler are you referring to here?

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?

More inline comments follow, please take a look.

CMakeLists.txt
76

why do we need Qt5Network for all kinds of Windows builds?

79

the option should be MSYS specific rather than being SnoreToast specific.

You could also just deploy the binary with msys.

wojnilowicz marked an inline comment as done.Jun 6 2020, 12:48 PM

Thanks for looking into that matter.

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.

CMakeLists.txt
76

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.

79

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.

wojnilowicz marked an inline comment as done.Jun 6 2020, 12:51 PM

You could also just deploy the binary with msys.

Thanks for the hint. I already knew that possibility.

brute4s99 added inline comments.Jun 7 2020, 10:03 PM
CMakeLists.txt
75 ↗(On Diff #81529)

please maintain the REQUIRED call for normal Windows builds.
You can use the OS specific build variables..

Qt5Network added as required

wojnilowicz marked an inline comment as done.Jun 14 2020, 3:46 PM
wojnilowicz added inline comments.
CMakeLists.txt
75 ↗(On Diff #81529)

Is it ready to land now?

vonreth accepted this revision.Jun 14 2020, 4:33 PM

It probably won't hurt

This revision is now accepted and ready to land.Jun 14 2020, 4:33 PM
wojnilowicz closed this revision.EditedJun 14 2020, 5:25 PM
wojnilowicz marked an inline comment as done.

It probably won't hurt

Thank you for the fast review. I pushed it to gitlab.