-
Notifications
You must be signed in to change notification settings - Fork 337
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
base: master
Are you sure you want to change the base?
initial meson move #1226
Conversation
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. |
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 |
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) |
Yeah, seems good to me. Maybe @bebehei has a stronger opinion about this, since he set up most of our testing infrastructure. |
Nice! Meson was on my "fancy things I might implement" list. Need to try it with some spare time at the evening. |
If you are a bit more experienced with Meson, I'd suggest you give it a shot rather than me :D |
Sorry for giving false hints, but I do not have any experience with meson. |
I'll be dropping coverage tests for the time being. |
@fwsmit tests program is failing here as well. |
Notable changes:
|
Can |
Probably yes. |
I am rather concerned over the fact that 'main.c' isnt simply just the |
I was also wondering why that is.(some leftover?) 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 |
Well, since the main function is heavily tied to everything in dunst.c probably moving main there would be better. |
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 |
|
anything missing from the makefile so far? |
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. |
It doesn't make sense to keep both build systems, it will be messy. |
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 |
I meant 'deprecated' in the intention that it would be un-used but not explicitly removed.
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. |
cc374d8
to
f8ae210
Compare
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? Anyway the changes look good to me. |
Please keep in mind that the test program has many warnings and will always build by default.
|
becomes consistent with Makefile.
d1db2a8
to
4ce5154
Compare
I am not quite sure I understand this part. Are you referring to compiler warnings? Using
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? |
Yes: https://kitten.pastes.sh/ninja.log i believe it comes from the
No: mesonbuild/meson#2518, the only workaround behind it is a configure time option. |
Does this option change the flags sent to the compiler? Also at the end of the warnings it says a cflag was not recognized.
I see. It's not too much of a problem but is there a way to silence the warnings? |
I'm not sure, but changing |
According to Edit: 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. |
Where? |
I updated the comment. I thought I saw it but checking it back there wasn't 🤦♂️ |
I tried building with But when I tried to run the tests with Also, I didn't get any warnings about the test (with gcc) so I think it is somehow compiler specific. |
Congrats you found #1228 on your own, which I can replicate without Meson. |
I'm just going to point to #1226 (review), because it's most certainly a meson port issue. |
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.
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 |
Currently, this has no support for the following features that already existed within Makefile:
Valgrindhttps://mesonbuild.com/Unit-tests.html#other-test-optionsTest programmeson setup -Dtest=true
Documentation (Move to scdoc #1225)Fixes #1224