Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

initial meson move #1226

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Conversation

apprehensions
Copy link
Contributor

@apprehensions apprehensions commented Nov 2, 2023

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

Fixes #1224

@apprehensions apprehensions marked this pull request as draft November 2, 2023 15:40
@apprehensions
Copy link
Contributor Author

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
Copy link
Member

fwsmit commented Nov 4, 2023

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
Copy link
Contributor Author

apprehensions commented Nov 6, 2023

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
Copy link
Member

fwsmit commented Nov 7, 2023

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

@bebehei
Copy link
Member

bebehei commented Nov 7, 2023

Nice!

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

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

@apprehensions
Copy link
Contributor Author

apprehensions commented Nov 7, 2023

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

@bebehei
Copy link
Member

bebehei commented Nov 7, 2023

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

@apprehensions
Copy link
Contributor Author

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

@apprehensions
Copy link
Contributor Author

@fwsmit tests program is failing here as well.

@apprehensions apprehensions marked this pull request as ready for review November 9, 2023 17:37
@apprehensions
Copy link
Contributor Author

Notable changes:

  • GitHub CI workflow is Ubuntu only, due to dependency constraints, and how it shouldn't realistically fail to run on musl or other distributions, the source is their problem typically.

.github/workflows/main.yml Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
src/meson.build Outdated Show resolved Hide resolved
@apprehensions
Copy link
Contributor Author

Can main.c be moved into src?

@bebehei
Copy link
Member

bebehei commented Nov 11, 2023

Can main.c be moved into src?

Probably yes.

@apprehensions
Copy link
Contributor Author

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
Copy link
Member

bynect commented Nov 11, 2023

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
Copy link
Contributor Author

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
Copy link
Member

bynect commented Nov 11, 2023

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
Copy link
Contributor Author

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
Copy link
Member

bynect commented Nov 11, 2023

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
Copy link
Contributor Author

apprehensions commented Nov 11, 2023

  • Need to implement debug buildtype

@apprehensions
Copy link
Contributor Author

anything missing from the makefile so far?

@bynect
Copy link
Member

bynect commented Nov 13, 2023

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
Copy link
Contributor Author

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

@bynect
Copy link
Member

bynect commented Nov 13, 2023

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

@apprehensions
Copy link
Contributor Author

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
Copy link
Member

bynect commented Feb 29, 2024

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
Copy link
Contributor Author

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.

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.

meson.build Show resolved Hide resolved
@bynect
Copy link
Member

bynect commented Feb 29, 2024

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

I am not quite sure I understand this part. Are you referring to compiler warnings? Using make test I don't get any (with gcc version 12.3.1 20240112)

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

Shouldn't it be possible with meson to create a test target to build separately the test program? So that you can either build dunst or the test and not necessarily together?

@apprehensions
Copy link
Contributor Author

apprehensions commented Feb 29, 2024

Are you referring to compiler warnings?

Yes: https://kitten.pastes.sh/ninja.log i believe it comes from the warning_level=1.

Shouldn't it be possible with meson to create a test target to build separately the test program?

No: mesonbuild/meson#2518, the only workaround behind it is a configure time option.

@bynect
Copy link
Member

bynect commented Feb 29, 2024

Are you referring to compiler warnings?

Yes: https://kitten.pastes.sh/ninja.log i believe it comes from the warning_level=1.

Does this option change the flags sent to the compiler? Also at the end of the warnings it says a cflag was not recognized.

Shouldn't it be possible with meson to create a test target to build separately the test program?

No: mesonbuild/meson#2518, the only workaround behind it is a configure time option.

I see. It's not too much of a problem but is there a way to silence the warnings?

@apprehensions
Copy link
Contributor Author

Does this option change the flags sent to the compiler?

I'm not sure, but changing warning_level to 0 seems to solve it, but i'm not sure if that is a good idea.

@bynect
Copy link
Member

bynect commented Feb 29, 2024

Does this option change the flags sent to the compiler?

I'm not sure, but changing warning_level to 0 seems to solve it, but i'm not sure if that is a good idea.

According to
https://mesonbuild.com/Builtin-options.html#details-for-warning_level

Edit:
It will add a -Wall option.

So I don't really know what is going on since the test should use that in the makefile without complaining. Maybe the test in make does not use pedantic? At the moment I can't check since I am outside.

@apprehensions
Copy link
Contributor Author

But since you already add that to the whole project

Where?

@bynect
Copy link
Member

bynect commented Feb 29, 2024

But since you already add that to the whole project

Where?

I updated the comment. I thought I saw it but checking it back there wasn't 🤦‍♂️

@bynect
Copy link
Member

bynect commented Feb 29, 2024

I tried building with meson build and ninja -C build and everything worked well. I also tried with the x11 and wayland options. (nitpick question, is it possible to move dunst directly in the build dir?)

But when I tried to run the tests with meson test -C build the runner segfaults. When I read the log a lot of test failed, supposedly because the files in test/data where not found by the runner.

testlog.txt

Also, I didn't get any warnings about the test (with gcc) so I think it is somehow compiler specific.

@apprehensions
Copy link
Contributor Author

Congrats you found #1228 on your own, which I can replicate without Meson.

@alebastr
Copy link
Contributor

But when I tried to run the tests with meson test -C build the runner segfaults. When I read the log a lot of test failed, supposedly because the files in test/data where not found by the runner.

WARNING: Cannot open '/home/nect/dev/dunst/build/test/data/dunstrc.default': 'No such file or directory'

I'm just going to point to #1226 (review), because it's most certainly a meson port issue.

@bynect
Copy link
Member

bynect commented Feb 29, 2024

Congrats you found #1228 on your own, which I can replicate without Meson.

It actually is not the same thing, because I tried commenting the notification test suite (which is what causes the problem there) and it still is being problematic. Also a lot (and I mean it) of tests fail, so there is something else going on. Obviously I tried make and it works for the tests.

But when I tried to run the tests with meson test -C build the runner segfaults. When I read the log a lot of test failed, supposedly because the files in test/data where not found by the runner.

WARNING: Cannot open '/home/nect/dev/dunst/build/test/data/dunstrc.default': 'No such file or directory'

I'm just going to point to #1226 (review), because it's most certainly a meson port issue.

Thanks for pointing that out. I thought that a way to fix this problem was added already.

Also I noticed that the test runner is built/run with the debug verbosity which is not ideal since it clutters all the output

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move to meson
7 participants