-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
easyeffects: Make service compatible with v8.0.x #8192
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
base: master
Are you sure you want to change the base?
Conversation
AlephNought0
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tested this commit with easyeffects version 8.0.3 and so far everything works well. There's only one thing that needs change and another one that is optional imo.
6cc8ae8 to
9c1fe22
Compare
|
Works great for me, solves the bug as described. |
|
It seems like the newest version of 8.0.3 easyeffects has dropped in nixos-unstable now. I believe we should hurry so people don't experience a broken service for too long. |
c77367a to
29b44f4
Compare
|
I feel this is ready, we should be backwards compatible now. I tested with both |
modules/services/easyeffects.nix
Outdated
| if olderThan8 then | ||
| { | ||
| Before = [ "pipewire.socket" ]; | ||
| } | ||
| else | ||
| { | ||
| Type = "dbus"; | ||
| BusName = "com.github.wwmm.easyeffects"; | ||
| } | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this flipped?
| if olderThan8 then | |
| { | |
| Before = [ "pipewire.socket" ]; | |
| } | |
| else | |
| { | |
| Type = "dbus"; | |
| BusName = "com.github.wwmm.easyeffects"; | |
| } | |
| ); | |
| if olderThan8 then | |
| { | |
| Type = "dbus"; | |
| BusName = "com.github.wwmm.easyeffects"; | |
| } | |
| else | |
| { | |
| Before = [ "pipewire.socket" ]; | |
| } | |
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, not only that but the code is also incorrect. BusName and Type are a part of Service section, not Unit. There is no need to try to remove Before=["pipewire.socket"]; for the older version of easyeffects, because it is not incorrect and it will work with it in the older version as well. Instead please do this.
Service =
{
ExecStart =
if olderThan8
then "${cfg.package}/bin/easyeffects --gapplication-service ${presetOpts}"
else "${cfg.package}/bin/easyeffects --hide-window --service-mode ${presetOpts}";
ExecStop = "${cfg.package}/bin/easyeffects --quit";
KillMode = "mixed";
Restart = "on-failure";
RestartSec = 5;
TimeoutStopSec = 10;
}
// (
if olderThan8
then {
BusName = "com.github.wwmm.easyeffects";
Type = "dbus";
}
else {}
);
So basically, there is no need to change Unit=, only Service=.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for that mess, i only checked the service args when i tested.
Updated it now. This is the output from the tests i did:
nixpkgs stable 25.05
$ easyeffects --version
easyeffects version: 7.2.5
$ systemctl --user show easyeffects.service --property=BusName,Type
Type=dbus
BusName=com.github.wwmm.easyeffectsStartup args on service: easyeffects --gapplication-service
nixpkgs unstable
$ easyeffects --version
easyeffects 8.0.3
$ systemctl --user show easyeffects.service --property=BusName,Type
Type=simple
BusName=Startup args on service: easyeffects --hide-window --service-mode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything seems to be fine now. The only one minor thing is that you removed the Before= directive in the Unit section. I did take a look around before commenting and it seems like that After= is the correct way to do this for sockets. It just seems that Before= worked because systemd seems to deal with this already. So you can either add After=["pipewire.socket"] or leave it be. It doesn't seem to fully matter from my research. The code is correct and works either way. I believe the PR can be merged now,
29b44f4 to
c53fb42
Compare
AlephNought0
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Can be reverted once nix-community/home-manager#8192 is merged
Description
Fixes #8185
The upstream easyeffects project has migrated from GTK to Qt, deprecating --gapplication-service and removing DBus integration. See #8185 for more details.
Tested with version
8.0.3and7.2.5, useslib.versionOlder cfg.package.version "8.0.0"to be backwards compatible.Other notes
Since they have moved config directory, it will warn show warnings because of empty dirs. These warnings will also show in the service until you remove them. I dont think this is a problem and new users will not experience this.
--
Checklist
Change is backwards compatible.
Code formatted with
nix fmtornix-shell -p treefmt nixfmt deadnix keep-sorted --run treefmt.Code tested through
nix run .#tests -- test-allornix-shell --pure tests -A run.all.Test cases updated/added. See example.
Commit messages are formatted like
See CONTRIBUTING for more information and recent commit messages for examples.
If this PR adds a new module
If this PR adds an exciting new feature or contains a breaking change.