slock: adds ability to add custom patches

[Gerschtli]

Motivation for this change

I needed to add patches to slock so I added the patches argument.
Usage:

{
  nixpkgs.config.packageOverrides = pkgs: {
    slock = pkgs.slock.override {
      patches = [ ./patch.diff ];
    };
  };
}

Things done

• Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
• Built on platform(s)
  • NixOS
  • macOS
  • Linux
• Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
• Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
• Tested execution of all binary files (usually in ./result/bin/)
• Fits CONTRIBUTING.md.

---

[mention-bot]

@Gerschtli, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mt-caret, @astsmtl and @pSub to be potential reviewers.

[Mic92]

How about using:

{
  nixpkgs.config.packageOverrides = pkgs: {
    slock = pkgs.slock.overrideDerivation (old: {
      patches = [ ./patch.diff ];
    });
  };
}

instead?

[Mic92]

The postPatch looks good to me though. So I would only remove inherit patches;.

[Gerschtli]

@Mic92 This does not work. My current workaround is

{
  nixpkgs.config.packageOverrides = pkgs: {
    slock = pkgs.lib.overrideDerivation pkgs.slock (attrs: {
      patchPhase = attrs.patchPhase + " && patch < " + ./patch.diff;
    });
  };
}

Not really a nice solution I think..

[Mic92]

To be sure we are on the same page: You say if you rename patchPhase to postPatch, you cannot set patches = [ ./patch.diff ]; in overrideDerivation?

[Gerschtli]

That would work, but why use this method if it isn't a problem to make the usage easier without any drawbacks?

[Gerschtli]

Hey, whats the status?

[Mic92]

The drawbacks is, that it pulls patches from all-package.nix, where it can have a different value.
We have for example a package called src. See dwm package for alternatives.

[Mic92]

It also promotes an API, where it is unclear, if we can add our own patches. So it should be at least named extraPatches.

[Gerschtli]

I can't see the difference why this ( https://github.com/NixOS/nixpkgs/blob/master/pkgs/applications/window-managers/dwm/default.nix ) is okay, but here it isn't

[Mic92]

The difference between dwm and this pr is here: https://github.com/NixOS/nixpkgs/blob/master/pkgs/top-level/all-packages.nix#L13962

[Gerschtli]

So if I would edit the line for slock in all-packages.nix, would it be okay?

[Mic92]

You can also expose conf the same way and I would still called it config.slock.extraPatches there because we might need to add patches too in future.

[Gerschtli]

Sounds good to me. Could you please tell me, where the slock package is called? I can't find the line in all-packages.nix...
EDIT: Found the entry, updating the PR

[Gerschtli]

I have updated the PR, but I really can't see the benefit of using extraPatches instead of the widely used patches name..

[oxzi]

I'd love to see this PR merged and be available in 17.09. Thanks for creating this patch and reviewing it.

[globin]

I'm strongly opposed to this. I don't want to promote this kind of API.

I'd propose renaming patchPhase to postPatch which is the way it should be and then you can use overrideDerivation/overrideAttrs to add custom patches. There is no need to add a config parameter for this because on could argue the same for each and every package causing huge bloat.

Sorry for not stating this earlier but this PR had slipped past me.

[sifmelcara]

Is this parameter meant to be used with https://tools.suckless.org/slock/patches/ ?
If yes, IMHO, it is a little bit strange that user need to write something like the following in order to enable extra patches provided by upstream:

{
  nixpkgs.config.packageOverrides = pkgs: {
    slock = pkgs.slock.overrideDerivation (old: {
      patches = [ (pkgs.fetchurl {
                     url = "https://tools.suckless.org/slock/patches/slock-capscolor.diff" ;
                     sha256 = "0nnakvsnzfvq0s6kyvmf0fql29bnls75hli2vkm8smr1by86c9k6";
                   })];
    });
  };
}

Maybe extra options like enableCapscolor, enableControlClear would be better?

Edit: I mean, to write

{
  nixpkgs.config.packageOverrides = pkgs: {
    slock = pkgs.slock.overrideDerivation (old: {
      enableCapscolor = true;
      enableControlClear = true;
    });
  };
}

instead.

[oxzi]

I'm strongly against adding options for every possible patch out there. This would imply that you know every patch out there (not just the patches on the slock-webpage) and that other patches aren't worth it. I'm a friend of a simple patch-list like in the dwm-package.

[sifmelcara]

@geistesk I think these patches provided by upstream are more like "extra features" than "extra patches". If we let user to fetch the patches themselves or let user to use their own patches, the patch may not be applicable anymore when slock receive a version update. If we provide options like enableCapscolor and enableControlClear, then who update the slock package can also update the version of these patches provided by upstream.

And if users really need to add custom patches, they can use method mentioned by @globin

[oxzi]

@sifmelcara These patches are created against different versions (tags and commits) and are user-submitted. So it's perhaps not possible to merge them automatically into the latest version.
The situation with dwm is similar, where you've got hundreds of patches (against different versions). Some of those patches won't work together.
So I'm against offering those patches. If a user of slock/dwm/… wants to use them, they can apply them manually.

Honestly I don't care how I'll patch it, as long as I'm able to patch it. My current slock-patching looks like this, but this won't work. So I'm overwriting the src…

nixpkgs.config.packageOverrides = pkgs: {
    slock = pkgs.slock.overrideAttrs(oldAttrs: rec {
        patches = [ /etc/nixos/patches/slock-dpms-20170923-fa11589.diff ];
    });
};

[Gerschtli]

I think the way, I have adapted the PR should be okay because

1. the conf parameter can not mistakenly be set via callPackage
2. the ability to add patches with overrideDerivation / overrideAttrs is guaranteed

@globin I understand now why you don't want such interfaces, sounds logical ;)