Support patching nixpkgs

[basvandijk]

Motivation for this change

Sometimes you want to track a specific branch of nixpkgs (like nixpkgs-unstable or some release branch) but you would like to apply some unmerged pull requests or some other patches to that revision.

Note that overlays are not always sufficient in this case because they don't work for NixOS modules and you're forced to duplicate code which already exists in some commit.

In this case you could fork nixpkgs, create a branch based on your desired upstream branch and cherry-pick the commits you need on top of that branch. Then everytime you need to upgrade nixpkgs you rebase your branch on the latest upstream branch that you're tracking. This process works but is a
bit involved and somewhat untransparent.

This PR implements support in nixpkgs for a simpler and more transparent mechanism to apply some patches to an existing revision. First you fetch your desired revision of nixpkgs like for example:

  let
    nixpkgs =
      builtins.fetchTarball {
	url = "https://github.com/NixOS/nixpkgs/archive/86d58407a6bb8ef2e1a58ab50318b149ffd4feda.tar.gz";
	sha256 = "0a2lkvacn78nza9hlmdx9znp1my3c3jf3xyhk9ydw6lxa348b7sl";
      };

Then you import that revision and apply it to a list of patches, for example:

    patchedPkgs = import nixpkgs {
      patches = pkgs : [
	# https://github.com/NixOS/nixpkgs/pull/59950
	(pkgs.fetchpatch {
	  url = https://github.com/NixOS/nixpkgs/commit/1f770d20550a413e508e081ddc08464e9d08ba3d.patch;
	  sha256 = "1nlzx171y3r3jbk0qhvnl711kmdk57jlq4na8f8bs8wz2pbffymr";
	})
      ];
    };
  in patchedPkgs...

Note that patches should be a function from a nixpkgs set pkgs to a list of patch files that can be applied to a nixpkgs directory tree. The pkgs set should mainly be used to get the fetchpatch function from. Note that pkgs is a nixpkgs set for the specified localSystem but without overlays and crossOverlays defined.

Overlays are applied after patching which means you can have overlays which can depend on the patched content.

Things done

• Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
• Built on platform(s)
  • NixOS
  • macOS
  • other Linux distributions
• 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 nix-review --run "nix-review wip"
• Tested execution of all binary files (usually in ./result/bin/)
• Determined the impact on package closure size (by running nix path-info -S before and after)
• Assured whether relevant documentation is up to date
• Fits CONTRIBUTING.md.

---

[nixos-discourse]

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/support-patching-nixpkgs/2737/1

[basvandijk]

Some design decisions I took:

• I perform the patching as early as possible, right after the Nix version check. So the order of imports is:

  • ./default.nix
  • ./patched.nix EDIT: I moved the patching to ./default.nix
  • ./pkgs/top-level/impure.nix
  • ./pkgs/top-level/default.nix

  I think this makes sense because now we can use this patch mechanism to patch the later phases.

*I choose to implement this in a separate file ./patched.nix instead of in ./default.nix to separate concerns. However, I also don't mind merging this logic in ./default.nix because then we end up with a single .nix again in the root of nixpkgs which might be nice.

• The bootstrapPkgs set, which the user can use to get the fetchpatch function from and which is used to perform the actual patching (i.e.bootstrapPkgs.stdenvNoCC.mkDerivation {...}), is a nixpkgs set configured for the specified localSystem. I think it makes sense to perform all work, including patching, on a builder for the specified localSystem. Although I'm not completely sure about this. Maybe preferLocalBuild = true might even be better here. I appreciate comments on this!

• bootstrapPkgs has no overlays defined. This makes it possible to depend overlays on the patched content which I think is way more common than the other way around.

[ryantm]

@basvandijk Nice to see you standardizing this patching idea you presented at NixCon a while back. I've been happily using that method on my job's nixops nixpkgs repo. I think it is great for encouraging upstreaming of contributions, because the patch is in the same format as whatever should eventually be applied to nixpkgs.

One issue we ran into is that the default fetchpatch implementation strips away information that is necessary for renaming files. ( #32084 ) Is that a problem with this implementation?

Our workaround is to use fetchurl on the patches that need to rename files, but I guess that opens us up to some instability that fetchpatch is trying to avoid.

[basvandijk]

One issue we ran into is that the default fetchpatch implementation strips away information that is necessary for renaming files. ( #32084 ) Is that a problem with this implementation?

@ryantm thanks for pointing me to #32084. Fortunately, this implementation lets the user decide how to fetch patches. So users deciding to use fetchpatch could potentially suffer from #32084. In that case they could fall back to using fetchurl or include patches literally in their repos.

[basvandijk]

I liked @volth idea of having a separate fix function which applies a list of patches to a source directory. So I added this function to trivaial-builders.nix but gave it the more descriptive name: applyPatches.

I also decided to move the patching code to ./default.nix instead of having it in a separate file. I believe it's a bit cleaner this way.

[FRidh]

Please avoid the use of you when writing documentation

[hraban]

Hi Frederik, can I ask why? Do you have a link to more info on this? Is it a nix convention?

I ask because I personally prefer reading this style of documentation, and all style guides I find on Google encourage using the second person.

[FRidh]

Actually I don't think we had a style guide at the time, but it was the common style at the time of writing and the way of writing I was used to from academia.

[matthewbauer]

This is using IFD, which seems especially dangerous for the entire Nixpkgs. Not necessarily disqualifying, but we should probably have some idea of what that means.

[edolstra]

I'm 👎 on this. It seems very ad hoc and hacky to make Nixpkgs rewrite its own source tree. That's the job of the version management tool (e.g. Git), which can do a much better job of managing sources and patches.

Maybe flakes could provide this functionality (e.g. by having a flake reference like github:NixOS/nixpkgs?patch=./foo.patch) but I'm convinced it's a good idea.

[ryantm]

I would like to be able to declare a base version of nixpkgs and a list of patches. Maybe we need to make a separate community tool that makes it easy to do that.

Reasons I like using something like @basvandijk's proposal:

1. Easy to downstream changes: You can pick patches from unstable to apply to a stable version.
2. Easy to upstream changes: Changes you make are done as patches (e.g. instead of overrides), so you can easily convert them into a pull request.
3. You don't have to maintain an internal fork of nixpkgs.

[edolstra]

@ryantm I feel that that tool already exists, namely Git. I typically have a branch like local-19.03 containing local / cherry-picked patches that gets rebased on top of nixos-channels/nixos-19.03 periodically. It might be nice if the rebasing were done automatically, though.

[ryantm]

@edolstra maintaining the local-19.03 branch you are describing doesn't sound like what I mean when I say "declare". I would like a declarative description of a publicly available version of nixpkgs and a list of publicly available patches. That way I can share this declarative description with collaborators who can each arrive at the same copy of nixpkgs without sharing it directly.

I use this at work to keep the nixpkgs we use for NixOps consistent among all my co-workers.

You may be right that Git is the tool we need to do this! I'll try to look into it some.

[edolstra]

The problem with maintaining such a list of patches is that you have to resolve any conflicts manually, e.g. if you try to update the base Nixpkgs version. Whereas with git rebase, you get a more-or-less friendly way to resolve conflicts.

[edolstra]

I do see the usefulness of that, but I think relying on IFD is a bad idea. It would be better to extend builtins.fetchGit / builtins.fetchTarball with a patches argument, e.g.

builtins.fetchTarball {
  url = https://github.com/NixOS/nixpkgs-channels/archive/nixos-19.03.tar.gz;
  patches = [ ./meltdown.patch ]
}

since this does not rely on IFD.

[edolstra]

That's not IFD because builtins.fetchTarball is not a derivation :-)

[danbst]

I find this a temporary solution, but still a useful change. I wish there was also a notion of "reverted" patch.

I agree with @edolstra that nixpkgs source trees should be managed by outer program. Eventually, in future, whenever someone comes to do that...

[nbp]

While I understand the need for this feature, as I my-self tried to solved it in the past.
Import-From-Derivation is a banned pattern in Nixpkgs repository.

Then everytime you need to upgrade nixpkgs you rebase your branch on the latest upstream branch that you're tracking. This process works but is a bit involved and somewhat untransparent.

@P-E-Meunier could this be a case where Pijul could help? As a way to add/substract a set of changes on top of a frequently updated repository.

[P-E-Meunier]

That is indeed the main use case for Pijul. Don't use it for Nixpkgs before the next version, the repository will be huge and slow. However, if all goes as planned, the next version will be competitive with Git on these two things.

[deliciouslytyped]

I want to do ad hoc patching a lot so I really like the idea here regardless of if it should be used in nixpkgs. I should have found this PR earlier. :)

I find it useful to just vary nix expressions during a development/prototyping process instead of futzing imperatively with git.

[blaggacao]

The solution (tbd): NixOS/nix#4433.

[stale]

I marked this as stale due to inactivity. → More info

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/how-to-try-a-pr/15410/6

[Artturin]

This is possible now, see this comment for a example #142273 (comment)