nixos/qbittorrent: copy config instead of symlinking to nix store#482534
nixos/qbittorrent: copy config instead of symlinking to nix store#482534Titaniumtown wants to merge 1 commit intoNixOS:masterfrom
Conversation
sith-lord-vader
left a comment
There was a problem hiding this comment.
Looks good. Much-needed change
There was a problem hiding this comment.
Thanks for the PR, came across it by coincidence. Pinging @fsnkty as well, as we're listed as maintainers.
First off, I think moving to copy/install instead of symlinking is the right way to go.
Some sidenotes:
Before the initial merge I did some digging and found out that qbit deletes the config before writing, as can be seen in the source code. So when trying to write to the config file it will remove the symlink and than write a new config. That explained why the symlink disappeared and qbit can write to the config. I'm not certain if this behavior is still the same, but I expect it is.
Currently changes made in the webUI also do not persist across reboots, as tested.
So this is what happens at the moment
- symlink gets made
- optionally: user makes changes
- qbit deletes symlink and writes config
A restart or changes to the declarative config repeat this cycle. So changes do not persist when using a declarative config.
a582d37 to
fda09fe
Compare
|
@undefined-landmark sorry about the late reply. my latest change addressed your comment. Thanks! |
|
Thanks. In another PR of mine I received feedback that tmpfiles is preferred over install/cp: #483956 (comment). Same applies here I assume. Probably tmpfiles can simply be changed from symlinking to copying? |
|
@undefined-landmark I don't know how to use tmpfiles that way, could you provide an example? |
|
I understand what you are requesting now. I am pushing a fix shortly. |
fda09fe to
8018e46
Compare
|
Take a look now! |
|
Yes, that's what I had in my mind. With this change we do duplicate part of Any opinions @fsnkty @kira-bruneau? |
8018e46 to
e061b00
Compare
|
@undefined-landmark Pushed! |
undefined-landmark
left a comment
There was a problem hiding this comment.
Sorry for the frequent back and forth. One last comment. Tests are building fine, almost there!
| @@ -170,7 +169,7 @@ in | |||
| "nss-lookup.target" | |||
| ]; | |||
| wantedBy = [ "multi-user.target" ]; | |||
| restartTriggers = optionals (cfg.serverConfig != { }) [ configFile ]; | |||
| restartTriggers = optionals (cfg.serverConfig != { }) [ (gendeepINI cfg.serverConfig) ]; | |||
There was a problem hiding this comment.
We repeat line 159 here. We could create a variable, e.g. configContent, in the let block. That variable can than be used in both this line and in line 159.
The
serverConfigoption generates a declarative config file, but the current implementation uses a tmpfilesL+rule to create a symlink to the config in the nix store. This doesn't work because:This PR changes the approach to use
ExecStartPreto copy the declarative config to the runtime location before qBittorrent starts. With this change, WebUI changes won't persist across service restarts.I came across this issue when trying to get qBittorrent working again once Vuetorrent was updated. Because of the update, the nix store path changed, breaking the WebUI.
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.