One thing I wasn't sure about was whether to add a wrapProgram that adds pulseaudio and libnotify to the PATH (but only on Linux?). I currently do that in my nixos config, but it also makes sense to do this in the package, I think.

Seems reasonable. Could you open an upstream issue about not hardcoding the sound theme directory (and potentially paplay, which we should otherwise patch to a full path) and link it in a comment?

Seems reasonable. Could you open an upstream issue about not hardcoding the sound theme directory (and potentially paplay, which we should otherwise patch to a full path) and link it in a comment?

Done (tulir/gomuks#260)

@charvp I didn't know it was supposed to make sounds, but then... I haven't used it in quite a while. If you're up for it, replace me in the maintainer list?

@charvp I didn't know it was supposed to make sounds, but then... I haven't used it in quite a while. If you're up for it, replace me in the maintainer list?

Done.

This is a semi-automatic executed nixpkgs-review which does not build all packages (e.g. lumo, tensorflow or pytorch)
If you find some bugs or got suggestions for further things to search or run please reach out to SuperSandro2000 on IRC.

Result of nixpkgs-review pr 109608 run on x86_64-linux 1

Result of nixpkgs-review pr 109608 run on x86_64-darwin 1

I've changed it so the patch is only included when the platform is Linux, which should hopefully unbreak darwin (since it seems that the extra pulseaudio dependency broke it).

I've changed it so the patch is only included when the platform is Linux, which should hopefully unbreak darwin (since it seems that the extra pulseaudio dependency broke it).

Good intention but normally we do not apply patches conditionally to prevent bit rot. I am not sure if it is okay here.

Good intention but normally we do not apply patches conditionally to prevent bit rot. I am not sure if it is okay here.

The patched file is only compiled in on Linux, so it doesn't seem like a problem to me. But I'm not as experienced, so maybe it's a problem here as well.

The patched file is only compiled in on Linux, so it doesn't seem like a problem to me. But I'm not as experienced, so maybe it's a problem here as well.

I thought a bit about it. Darwin does not work right now anyway so I would prefer to just apply the patch to all platforms and then I'll give this a merge.

The Darwin build does succeed right now (with the derivation on the main branch): https://hydra.nixos.org/build/135068035
Or do you mean that running the program doesn't actually work?

Or do you mean that running the program doesn't actually work?

I meant building. Can you adopt the patch so that it applies on all platforms and does not break darwin?

I've changed it so the binaries are injected with wrapProgram (but only when stdenv is Linux) and the hardcoded path is fixed with the patch. Since the darwin breakage was due to the pulseaudio injection this should work on all platforms. I wasn't able to test the Darwin build, since I don't have a Darwin machine.

This is a semi-automatic executed nixpkgs-review which is checked by a human on a best effort basis and does not build all packages (e.g. lumo, tensorflow or pytorch).
If you have any questions or problems please reach out to SuperSandro2000 on IRC.

Result of nixpkgs-review pr 109608 run on x86_64-darwin 1

This is a semi-automatic executed nixpkgs-review which is checked by a human on a best effort basis and does not build all packages (e.g. lumo, tensorflow or pytorch).
If you have any questions or problems please reach out to SuperSandro2000 on IRC.

Result of nixpkgs-review pr 109608 run on x86_64-linux 1