Skip to content

Conversation

sewnie
Copy link
Contributor

@sewnie sewnie commented Nov 2, 2023

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

Fixes #1224

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

sewnie commented Nov 2, 2023

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

@sewnie
Copy link
Contributor Author

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

@sewnie
Copy link
Contributor Author

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

@sewnie
Copy link
Contributor Author

sewnie commented Nov 9, 2023

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

@sewnie
Copy link
Contributor Author

sewnie commented Nov 9, 2023

@fwsmit tests program is failing here as well.

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

sewnie commented Nov 9, 2023

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.

@sewnie
Copy link
Contributor Author

sewnie commented Nov 10, 2023

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.

@sewnie
Copy link
Contributor Author

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

@sewnie sewnie requested a review from bebehei November 11, 2023 10:31
@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?

@sewnie
Copy link
Contributor Author

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

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

@sewnie
Copy link
Contributor Author

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

@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

@sewnie
Copy link
Contributor Author

sewnie commented Nov 11, 2023

  • Need to implement debug buildtype

@sewnie
Copy link
Contributor Author

sewnie commented Nov 13, 2023

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.

@sewnie
Copy link
Contributor Author

sewnie commented Nov 13, 2023

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

@bynect bynect merged commit f3d16aa into dunst-project:master Mar 20, 2025
23 checks passed
@bynect
Copy link
Member

bynect commented Mar 20, 2025

For now meson is the secondary build system, however I am positive we can move totally to meson in the future.
I would like to leave a couple of release before removing the makefile.
also to move to meson we have to make the ci work like the makefile.

@bynect
Copy link
Member

bynect commented Mar 20, 2025

anyway, thanks for your work ❤️

@bynect
Copy link
Member

bynect commented May 5, 2025

It now occurs to me that the prefix is not handled correctly, as pointed out by Narrat in #1462.

When the prefix is set to something the bindir is not actually used as prefix+bin.
I supposed that meson automatically managed the appending but I guess not.

Is there an idiomatic way to solve this? @apprehensions @eli-schwartz

@eli-schwartz
Copy link

When the prefix is set to something the bindir is not actually used as prefix+bin.
I supposed that meson automatically managed the appending but I guess not.

There are valid use cases for knowing both a relative and an absolute bindir.

Given:

--prefix=/usr --bindir=/usr/bin

--prefix=/usr --bindir=bin

the value of get_option('bindir') is "bin" and the absolute path can be gotten with

abs_bindir = get_option('prefix') / get_option('bindir')

For something like this:

install_data('dunstctl', install_dir: get_option('bindir'))

Meson knows to install dunstctl in the directory sigified as bindir, and to find that relative to the prefix. The ambiguity comes when you want to render the string representation of that directory inside a configuration file, such as a systemd service.

Comment on lines +70 to +73
add_project_arguments(
'-DSYSCONFDIR="@0@"'.format(get_option('prefix') / sysconfdir),
language: 'c',
)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can see for example this was already done here.

install_data('dunstrc', install_dir: sysconfdir / 'dunst')

conf_data = configuration_data()
conf_data.set('bindir', get_option('bindir'))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... but forgotten here.

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
10 participants