Don't use notifybysnore.h on MSYS2
ClosedPublic
Actions

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

Details

Reviewers

broulik
brute4s99
vonreth

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.

Harbormaster completed remote builds in B26035: Diff 81440.Apr 28 2020, 3:36 PM

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

I'm sorry, missing what exactly?

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.

Harbormaster completed remote builds in B26094: Diff 81529.Apr 29 2020, 4:52 PM

In D29258#659423, @brute4s99 wrote:

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.

Thanks for looking into that matter.

In 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.

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.

In D29258#674669, @vonreth wrote:

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

Harbormaster completed remote builds in B27140: Diff 83272.Jun 14 2020, 3:45 PM

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?

It probably won't hurt

This revision is now accepted and ready to land.Jun 14 2020, 4:33 PM

In D29258#675261, @vonreth wrote:

It probably won't hurt

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

Revision Contents
Changeset List

			Path	Packages
M			CMakeLists.txt (16 lines)
M			src/CMakeLists.txt (2 lines)
M			src/config-knotifications.h.cmake (2 lines)
M			src/knotificationmanager.cpp (6 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	81440	49d650a		Apr 28 2020, 3:36 PM	★	★
Diff 2	81529	49d650a	I see. I've updated the patch. Linking to downloaded library is now possible…	Apr 29 2020, 4:52 PM	★	★
Diff 3	83272	49d650a	Qt5Network added as required	Jun 14 2020, 3:45 PM	★	★