Skip to content

Change config location to /etc/dunst/dunstrc#798

Merged
tsipinakis merged 1 commit into
dunst-project:masterfrom
fwsmit:move-config
Jan 26, 2021
Merged

Change config location to /etc/dunst/dunstrc#798
tsipinakis merged 1 commit into
dunst-project:masterfrom
fwsmit:move-config

Conversation

@fwsmit

@fwsmit fwsmit commented Dec 24, 2020

Copy link
Copy Markdown
Member

Fixes #795
The makefile still isn't fully xdg compliant. If the user has set $XDG_CONFIG_DIRS it should be reflected our install, I believe.
I didn't know the best way to implement that, but this is some make documentation about all the standard directories, and this is the spec.

@fwsmit

fwsmit commented Dec 24, 2020

Copy link
Copy Markdown
Member Author

Weird, this commit doesn't seem to cause the ci failing. My bisect points to 58d215d.

Edit: that's only on wayland and is not a problem. I can't find what causes the ci failing. There doesn't seem to be an error message in the ci.

@tsipinakis

Copy link
Copy Markdown
Member

that's only on wayland and is not a problem. I can't find what causes the ci failing

The CI is failing in the test-install script. See here.

Re: This PR. I'm thinking in Debian terms where it's usual to have docs and example config files in /usr/share/doc/<pkg> and it's now muscle memory to search there when I want something. Looking in /etc/xdg is counter-intuitive in that sense. So, I'd opt for having both, one in /usr for documentation and the /etc/xdg one for actually loading it.

@fwsmit

fwsmit commented Dec 26, 2020

Copy link
Copy Markdown
Member Author

that's only on wayland and is not a problem. I can't find what causes the ci failing

The CI is failing in the test-install script. See here.

Ah thanks. To fix it I need to make sure the config is installed somewhere else when the prefix is changed. Do I need to make a new CONFIG_PREFIX or change PREFIX to / ?

Re: This PR. I'm thinking in Debian terms where it's usual to have docs and example config files in /usr/share/doc/<pkg> and it's now muscle memory to search there when I want something. Looking in /etc/xdg is counter-intuitive in that sense. So, I'd opt for having both, one in /usr for documentation and the /etc/xdg one for actually loading it.

I don't think it's a good idea to place two identical files in different places. Linking them doesn't seem good either. Maybe this is too distro specific. On arch I'm used to the config being something like /etc/dunst/dunstrc. There are only a few configs in /etc/xdg/.... I'm interested what other people think is the best place for the config.
We can also just make dunst search /usr/share/doc/dunst/dunstrc, but I think that's also a bad solution.

@tsipinakis

Copy link
Copy Markdown
Member

Linking or making dunst look in /usr/share/doc isn't going to work in Debian at least as everything in that directory is gzipped. Debian-wise I'd always opt to install the config file in both locations as the /usr/share/doc directory is muscle memory to all users by now, (and Debian policy suggests it).

The question here is what's best for upstream, default behaviour, does this extend to other distros?

Do I need to make a new CONFIG_PREFIX or change PREFIX to / ?

DESTDIR should be the target 'root' so changing PREFIX would break convention here. I'm not sure what the standard variable for /etc/ files is, if there is one. We should check what other projects do with such files.

@codecov-io

codecov-io commented Dec 27, 2020

Copy link
Copy Markdown

Codecov Report

Merging #798 (9615431) into master (7e9650a) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #798      +/-   ##
==========================================
- Coverage   59.55%   59.53%   -0.03%     
==========================================
  Files          36       36              
  Lines        5791     5793       +2     
==========================================
  Hits         3449     3449              
- Misses       2342     2344       +2     
Flag Coverage Δ
unittests 59.53% <0.00%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/settings.c 58.38% <0.00%> (-0.40%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e9650a...9615431. Read the comment docs.

@fwsmit

fwsmit commented Dec 27, 2020

Copy link
Copy Markdown
Member Author

Linking or making dunst look in /usr/share/doc isn't going to work in Debian at least as everything in that directory is gzipped. Debian-wise I'd always opt to install the config file in both locations as the /usr/share/doc directory is muscle memory to all users by now, (and Debian policy suggests it).

This page suggests that conffiles should be under /etc for debian. Every other distro I can find installs it's dotfiles in /etc as well.

The question here is what's best for upstream, default behaviour, does this extend to other distros?

I think it might be best to install them to /etc/dunst/dunstrc and change dunst to look there as well.

DESTDIR should be the target 'root' so changing PREFIX would break convention here. I'm not sure what the standard variable for /etc/ files is, if there is one. We should check what other projects do with such files.

Oh thanks, that's what I was looking for. I changed the test-install script to use that instead of PREFIX. Also made some other improvements to it.

@tsipinakis

tsipinakis commented Dec 28, 2020

Copy link
Copy Markdown
Member

This page suggests that conffiles should be under /etc for debian

I guess the divide here is that I view the dunstrc as both a configuration file (which is what this change is about) but also a big part of dunsts limited documentation/examples. Which is why I hesitate moving it entirely off that directory.

I think it might be best to install them to /etc/dunst/dunstrc and change dunst to look there as well.

That might be a good compromise, my holdout is that /etc/xdg is a rarely used path (no other program on my system has any config there other than autostart files).

Also, don't forget to update the path in dunsts manpage. (We now have 3 active PRs that touch the manpage, I'll probably merge #781 -> #798 -> #799 to have the easiest/least number of conflicts. So the last 2 PRs are blocked on the wayland one).

@tsipinakis tsipinakis mentioned this pull request Dec 29, 2020
@fwsmit fwsmit changed the title Change config location to /etc/xdg/dunst/dunstrc Change config location to /etc/dunst/dunstrc Jan 6, 2021
@fwsmit fwsmit force-pushed the move-config branch 2 times, most recently from 71039fb to 4388d0d Compare January 6, 2021 16:13
@fwsmit

fwsmit commented Jan 6, 2021

Copy link
Copy Markdown
Member Author

I finished this PR and rebased onto the wayland-2 branch. There were no conflicts.

@tsipinakis

Copy link
Copy Markdown
Member

I finished this PR and rebased onto the wayland-2 branch. There were no conflicts.

Apparently the merge commit confuses it. Both PRs have conflicts.

@fwsmit

fwsmit commented Jan 19, 2021

Copy link
Copy Markdown
Member Author

I finished this PR and rebased onto the wayland-2 branch. There were no conflicts.

Apparently the merge commit confuses it. Both PRs have conflicts.

No that's because I added some stuff to wayland-2 because of bebehei's comments. I rebased again and fixed the conflicts now.
I noticed that the changes from #793 also show up in the "files changed" tab. I don't know why, I just rebased on upstream/master.
I'll wait a bit before rebasing the other PR, since I don't know if I rebase this one correctly.

@fwsmit fwsmit requested a review from tsipinakis January 24, 2021 18:11
@fwsmit

fwsmit commented Jan 24, 2021

Copy link
Copy Markdown
Member Author

Could this be reviewed? Then it can be merged together with #799. Otherwise we will stack up conflicts.

@tsipinakis

Copy link
Copy Markdown
Member

Sorry, took a look at this yesterday and forgot to comment. This needs to be rebased. Merge commit is d3dd26a but in mater it's d3dd26a. I feel like this will cause some issues (or at least confuse history) if I merge as-is.

Also fixed the test-install script
@fwsmit

fwsmit commented Jan 25, 2021

Copy link
Copy Markdown
Member Author

Yeah I thought so, but didn't know how to fix. I rebased again and it has been cleaned up

@tsipinakis tsipinakis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM.

While reviewing this I couldn't help but think that we should simplify the test-install script to only provide the DESTDIR variable and let everything else be the default. But that's out of scope of this.

@tsipinakis tsipinakis merged commit 5d1b585 into dunst-project:master Jan 26, 2021
@fwsmit

fwsmit commented Jan 27, 2021

Copy link
Copy Markdown
Member Author

While reviewing this I couldn't help but think that we should simplify the test-install script to only provide the DESTDIR variable and let everything else be the default. But that's out of scope of this.

I don't think that's better. Setting these variables to some non-default value makes sure you aren't hardcoding the defaults in the makefile.

I noticed btw that I didn't update commit message. It should be "Change config location to /etc/dunst/dunstrc". Well I guess it's too late to change it now.

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 default config to /etc/xdg/dunst/dunstrc

3 participants