dunst-project / dunst

Lightweight and customizable notification daemon
https://dunst-project.org
Other
4.44k stars 338 forks source link

initial meson move #1226

Open apprehensions opened 8 months ago

apprehensions commented 8 months ago

Currently, this has no support for the following features that already existed within Makefile:

Fixes #1224

apprehensions commented 8 months ago

I had underestimated how big this project is, which is why i initially thought this would be quite easy, as i had figured dunst is this simple notification daemon program.

fwsmit commented 8 months ago

What is difficult about the test binary? It just needs to be compiled and run with valgrind. Meson probably has some way to run binaries

apprehensions commented 8 months ago

Thankfully, Meson comes with coverage reports, so i hope we can drop the coverage reporting all together and only have valgrind tests and run the test program.

https://mesonbuild.com/howtox.html#producing-a-coverage-report

sound good to you?

(this is still somewhat very difficult due to the complexity of Dunst, its website, documentation, testing suite and such... i still really expected dunst to be a simple project)

fwsmit commented 8 months ago

Yeah, seems good to me. Maybe @bebehei has a stronger opinion about this, since he set up most of our testing infrastructure.

bebehei commented 8 months ago

Nice!

Meson was on my "fancy things I might implement" list.

Need to try it with some spare time at the evening.

apprehensions commented 8 months ago

If you are a bit more experienced with Meson, I'd suggest you give it a shot rather than me :D

bebehei commented 8 months ago

Sorry for giving false hints, but I do not have any experience with meson.

apprehensions commented 8 months ago

I'll be dropping coverage tests for the time being.

apprehensions commented 8 months ago

@fwsmit tests program is failing here as well.

apprehensions commented 8 months ago

Notable changes:

apprehensions commented 8 months ago

Can main.c be moved into src?

bebehei commented 8 months ago

Can main.c be moved into src?

Probably yes.

apprehensions commented 8 months ago

I am rather concerned over the fact that 'main.c' isnt simply just the dunst_main function from dunst.c, seems complicated for no good reason that i know of currently..

bynect commented 8 months ago

I am rather concerned over the fact that 'main.c' isnt simply just the dunst_main function from dunst.c, seems complicated for no good reason that i know of currently..

I was also wondering why that is.(some leftover?)

Probably main.c could be removed altogether by putting main in dunst.c?

apprehensions commented 8 months ago

Probably main.c could be removed altogether by putting main in dunst.c?

Other code in the test suites depend on code from dunst.c. I've tried to move dunst_main to main.c myself but unsure what to do about the setup_done variable.

bynect commented 8 months ago

Probably main.c could be removed altogether by putting main in dunst.c?

Other code in the test suites depend on code from dunst.c. I've tried to move dunst_main to main.c myself but unsure what to do about the setup_done variable.

Well, since the main function is heavily tied to everything in dunst.c probably moving main there would be better.

apprehensions commented 8 months ago

Moving main.c to dunst.c, while dunst.c is required by the test suite causes problems, as the test suite depends on dunst's code too.

bynect commented 8 months ago

Moving main.c to dunst.c, while dunst.c is required by the test suite causes problems, as the test suite depends on dunst's code too.

Some simple meta programming can fix that honestly.

#ifndef TESTING

int main(int argc, char **argv) {
    return dunst_main(argc, argv);
}
#endif
apprehensions commented 8 months ago
apprehensions commented 8 months ago

anything missing from the makefile so far?

bynect commented 8 months ago

I was wondering, why remove outright the makefile when they can coexist afaik? Shouldn't the option to use meson be added alongside of makefiles?

Then, after some time (to get feedback from the users), removing the latter can be thought of in another pr.

apprehensions commented 8 months ago

It doesn't make sense to keep both build systems, it will be messy.

bynect commented 8 months ago

Yes, but it is still an additional dependency that is really not that required. I mean, it doesn't seem to add any value to what the makefiles can already do. So people would have to install and learn meson instead of just using make (installed by default everywhere) just for the sake of using meson.

Well, that is my idea. @fwsmit or @bebehei should decide a reasonable compromise.

Note: I am not saying that meson is useless, just that removing the makefiles point blank is probably not the best way

Narrat commented 8 months ago

There were some projects (like mpv, util-linux) which moved to meson and often thery had some gracious time for switching. First deprecating the old system with one release and with another removing it. But this transition period allowed to fix some eventual bugs/quirks with the new buildsystem and allowed package maintainers a smooth transition with a greater time window. It can be frustrating if you want to just make a minor version bump and suddenly the whole thing fails. And often in those cases you're on a time budget and couldn't follow upstream changes :D And then you need to invest some extra time. True, this is a rather uncomplicated project and switching to meson is often fairly easy.. But still. Just for package maintainers sake I would also favour a transition time.

apprehensions commented 8 months ago

Yeah thats.. understandable.

apprehensions commented 8 months ago

uh oh

apprehensions commented 8 months ago

unfortunately, i'm not really that great at writing this new change in the CHANGELOG or README, sorry

fwsmit commented 6 months ago

The changes look good overall. I've left one comment. Does @bebehei have any last comments?

unfortunately, i'm not really that great at writing this new change in the CHANGELOG or README, sorry

Okay, I can take a look at that.

fwsmit commented 6 months ago

It seems the tests don't run. At least in the github tests it says the following

0s
Run ninja -C build test
ninja: Entering directory `build'
[0/1] Running all tests.
No tests defined.
apprehensions commented 6 months ago

It seems the tests don't run. At least in the github tests it says the following

I had the test program get built only on a build option, but i've dropped that and now only ninja -C build test can run the program - but the test program will always get built so, hope that's alright.

alebastr commented 6 months ago

It appears that the protocol headers generated by meson were unused. The app was always including the existing prebuilt files from src/wayland/protocols, because that's the only possible match for #include "protocols/*-client-header.h"

I've tried to fix that with https://github.com/alebastr/dunst/commit/b9cd5499952a16cce7bf826ace4031fc59751305, which relies on -I passed either by meson or by Makefile.

apprehensions commented 6 months ago

I've tried to fix that with https://github.com/alebastr/dunst/commit/b9cd5499952a16cce7bf826ace4031fc59751305, which relies on -I passed either by meson or by Makefile.

https://github.com/alebastr/dunst/blob/b9cd5499952a16cce7bf826ace4031fc59751305/Makefile#L139-L140

If the Makefile works, i dont believe any modification should be required to the code.

alebastr commented 6 months ago

https://github.com/alebastr/dunst/blob/b9cd5499952a16cce7bf826ace4031fc59751305/Makefile#L139-L140

If the Makefile works, i dont believe any modification should be required to the code.

Why did you add src/wayland/protocols/meson.build then? If you believe the result of this file should not be used, then it's not necessary.

apprehensions commented 6 months ago

May i know how you know its unused?

alebastr commented 6 months ago

May i know how you know its unused?

Open src/wayland/protocols/wlr-layer-shell-unstable-v1-client-header.h and add #error "This file should not be included.

meson puts the generated code for protocols at build-dir/src/wayland/protocols/libclient_protos.a.p/, which cannot be resolved by include with protocols/ prefix.

apprehensions commented 6 months ago

Why did you add src/wayland/protocols/meson.build then?

This is rather interesting. Removing the subdir requirement for this somehow makes it still run properly? why is this?

Edit: based of what you had said, my best guess is because this is including headers directly, and the compiler is probably matching for it, as it doesnt complain about missing links or whatever.

fwsmit commented 6 months ago

Hi @apprehensions, do you have some time to work on this? It'd be nice to finish this soon, since otherwise other PR's might interfere with this work

apprehensions commented 6 months ago

I am mostly within constraint of how the source code of dunst is structured, and keeping it backwards compatible with Makefile. It would be also great if you can fix #1228 as it occurs in this PR.

To be honest, i don't believe it is my right to fix these inconsistencies regarding wayland protocol generation, so i suggest that get fixed in a seperate PR by @alebastr.

fwsmit commented 6 months ago

I am mostly within constraint of how the source code of dunst is structured, and keeping it backwards compatible with Makefile.

What do you mean exactly? Is it constraining to you to keep backwards compatibility? If so we can discuss some things you might want to change. But I still want the makefile to exist alongside meson for at least one release. It's not neccesary to do things exactly the same way as in the makefile, but that makes it easier for me to review the PR.

It would be also great if you can fix #1228 as it occurs in this PR.

As I noted in that issue, I couldn't reproduce it, so that makes it a little bit hard to fix. An option for you to still run the test suite locally is to use the docker CI images (https://github.com/dunst-project/docker-images/). These can also be used locally to run the tests in docker. See the instructions in the readme for how to do that.

To be honest, i don't believe it is my right to fix these inconsistencies regarding wayland protocol generation, so i suggest that get fixed in a seperate PR by @alebastr.

That's fine by me. The makefile doesn't address this either.

I would like it if you addressed all open discussions in this PR (and leaving a comment where you're not going to implement the suggestion). When that's done I can write some documentation and create issues for the things that still need to be done.

apprehensions commented 6 months ago

dealing with CI is horrible

fwsmit commented 6 months ago

dealing with CI is horrible

Yeah, github CI is a mess. But if you're not able to do it in this PR, you could also revert the changes to the CI for now. It's okay if the CI keeps running on make for until it's removed.

apprehensions commented 6 months ago

It would be better to have Meson support in CI, as the original idea was to deprecate Makefile.

fwsmit commented 5 months ago

It would be better to have Meson support in CI, as the original idea was to deprecate Makefile.

Or course. But if that's too hard for this PR, it can also be done in a separate PR (since make is not yet removed).

apprehensions commented 5 months ago

It is not a good idea to keep Meson's CI checking in a seperate PR. The main goal atleast that was proposed by the maintainers, is to switch primarily to Meson and keep Make deprecated. To have the PR that switches the project to Meson but still use Make in it's CI is not a good idea.

fwsmit commented 5 months ago

It is not a good idea to keep Meson's CI checking in a seperate PR. The main goal atleast that was proposed by the maintainers, is to switch primarily to Meson and keep Make deprecated. To have the PR that switches the project to Meson but still use Make in it's CI is not a good idea.

Well, then the CI needs to be fixed before this is merged. Currently the CI doesn't work. The issue seems to be related to https://github.com/actions/checkout/issues/1590

apprehensions commented 5 months ago

Simplicity is key, move to sourcehut and experience simpler and faster build frameworks.

I promise i was not paid to say this.

bynect commented 4 months ago

It would be better to have Meson support in CI, as the original idea was to deprecate Makefile.

Like I said in the past I think that this switch is kind of breaking (at least for package maintainers) so before removing make completely we should wait for a major release (v2?) or at least a version bump.

Similarly the CI should have both of the build systems just in case. If it is too difficult to do everything in a single pr, the ci part could be moved to a different pr like @fwsmit suggested. (especially if the meson code is done and just the ci is failing)

apprehensions commented 4 months ago

Like I said in the past I think that this switch is kind of breaking

I meant 'deprecated' in the intention that it would be un-used but not explicitly removed.

If it is too difficult to do everything in a single pr, the ci part could be moved to a different pr like @\fwsmit suggested. (especially if the meson code is done and just the ci is failing)

The meson code is technically complete. I believe that the CI should be functioning before this gets merged, to gurantee that the Meson move is functioning. The CI is failing due to the rather complicated test framework and the need for many distributions it seems.

In the meantime, i will just remove the CI modifications and let someone else handle it.

bynect commented 4 months ago

Like I said in the past I think that this switch is kind of breaking

I meant 'deprecated' in the intention that it would be un-used but not explicitly removed.

If it is too difficult to do everything in a single pr, the ci part could be moved to a different pr like @\fwsmit suggested. (especially if the meson code is done and just the ci is failing)

The meson code is technically complete. I believe that the CI should be functioning before this gets merged, to gurantee that the Meson move is functioning. The CI is failing due to the rather complicated test framework and the need for many distributions it seems.

In the meantime, i will just remove the CI modifications and let someone else handle it.

If you did a lot of the work on the ci you could simply create a new pr and send them there.

Also, sorry to bother you but could you take into account #1290 and make a way to compile only wayland without x11? It should be fairly easy, you just need to removed the x deps and define a ENABLE_X11 as I did in the makefile.

Anyway the changes look good to me.

apprehensions commented 4 months ago

I had the test program get built only on a build option, but i've dropped that and now only ninja -C build testcan run the program - but the test program will always get built so, hope that's alright.

Please keep in mind that the test program has many warnings and will always build by default.

1290:

If it is needed, i can hide the test program behind a flag, which adds an extra step to running the test program.