Support patching nixpkgs by basvandijk · Pull Request #59990 · NixOS/nixpkgs

basvandijk · 2019-04-21T22:29:02Z

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

nixos-discourse · 2019-04-21T22:36:03Z

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 · 2019-04-21T23:14:35Z

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 · 2019-04-22T15:45:23Z

@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 · 2019-04-23T08:05:30Z

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.

applyPatches applies a list of patches to a source directory.

basvandijk · 2019-04-23T09:55:34Z

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.

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. nixpkgs also supports 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 : [ # NixOS#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.

FRidh · 2019-04-27T05:12:19Z

doc/patching-nixpkgs.xml

+         xml:id="chap-patching-nixpkgs">
+ <title>Patching nixpkgs</title>
+ <para>
+   Sometimes you want to track a specific branch of nixpkgs (like


Please avoid the use of you when writing documentation

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.

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 · 2019-05-08T18:22:29Z

default.nix

+
+  unpatchedPkgs = impure unpatchedArgs;
+  bootstrapPkgs = impure bootstrapArgs;
+  patchedPkgs = import patchedNixpkgsDir unpatchedArgs;


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 · 2019-05-08T18:37:47Z

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 · 2019-05-08T20:29:49Z

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:

Easy to downstream changes: You can pick patches from unstable to apply to a stable version.
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.
You don't have to maintain an internal fork of nixpkgs.

edolstra · 2019-05-09T09:00:24Z

@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 · 2019-05-09T11:29:33Z

@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 · 2019-05-09T11:43:07Z

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 · 2019-05-09T12:27:00Z

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 · 2019-05-09T13:31:46Z

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 · 2019-08-28T17:47:33Z

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 · 2019-08-28T17:52:28Z

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 · 2020-04-15T15:37:29Z

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 · 2021-01-07T16:05:40Z

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

stale · 2021-07-08T00:22:13Z

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

nixos-discourse · 2021-10-08T16:43:24Z

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 · 2022-05-17T00:34:32Z

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

basvandijk requested a review from nbp as a code owner April 21, 2019 22:29

GrahamcOfBorg added 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Apr 21, 2019

basvandijk requested a review from edolstra April 21, 2019 22:41

trivial-builders: add the applyPatches function

d348190

applyPatches applies a list of patches to a source directory.

basvandijk force-pushed the nixpkgs-patches branch from 3bd4df1 to 2b1bd50 Compare April 23, 2019 09:42

basvandijk force-pushed the nixpkgs-patches branch from 2b1bd50 to 20b8e9e Compare April 23, 2019 11:30

ofborg bot added the 8.has: documentation This PR adds or changes documentation label Apr 23, 2019

basvandijk force-pushed the nixpkgs-patches branch from 20b8e9e to ea21757 Compare April 23, 2019 11:33

FRidh reviewed Apr 27, 2019

View reviewed changes

matthewbauer reviewed May 8, 2019

View reviewed changes

danbst approved these changes Jun 5, 2019

View reviewed changes

basvandijk mentioned this pull request Sep 2, 2019

trivial-builders: add the applyPatches function #67930

Merged

10 tasks

worldofpeace mentioned this pull request Aug 11, 2020

Support flake references to patches NixOS/nix#3920

Open

ryantm added 2.status: merge conflict This PR has merge conflicts with the target branch and removed 2.status: merge conflict This PR has merge conflicts with the target branch labels Oct 3, 2020

573 mentioned this pull request Feb 3, 2021

qemu-5.2.0 build on unstable possibly broken. #111651

Closed

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 8, 2021

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Oct 8, 2021

SuperSandro2000 added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Nov 16, 2021

Artturin closed this May 17, 2022

Uh oh!

Comments

Conversation

basvandijk commented Apr 21, 2019

Motivation for this change

Things done

Uh oh!

nixos-discourse commented Apr 21, 2019

Uh oh!

basvandijk commented Apr 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryantm commented Apr 22, 2019

Uh oh!

basvandijk commented Apr 23, 2019

Uh oh!

basvandijk commented Apr 23, 2019

Uh oh!

FRidh Apr 27, 2019

Choose a reason for hiding this comment

Uh oh!

hraban Dec 27, 2022

Choose a reason for hiding this comment

Uh oh!

FRidh Dec 29, 2022

Choose a reason for hiding this comment

Uh oh!

matthewbauer May 8, 2019

Choose a reason for hiding this comment

Uh oh!

edolstra commented May 8, 2019

Uh oh!

ryantm commented May 8, 2019

Uh oh!

edolstra commented May 9, 2019

Uh oh!

ryantm commented May 9, 2019

Uh oh!

edolstra commented May 9, 2019

Uh oh!

edolstra commented May 9, 2019

Uh oh!

edolstra commented May 9, 2019

Uh oh!

danbst left a comment

Choose a reason for hiding this comment

Uh oh!

nbp commented Aug 28, 2019

Uh oh!

P-E-Meunier commented Aug 28, 2019

Uh oh!

deliciouslytyped commented Apr 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blaggacao commented Jan 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stale bot commented Jul 8, 2021

Uh oh!

nixos-discourse commented Oct 8, 2021

Uh oh!

Artturin commented May 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

15 participants

basvandijk commented Apr 21, 2019 •

edited

Loading

deliciouslytyped commented Apr 15, 2020 •

edited

Loading

blaggacao commented Jan 7, 2021 •

edited

Loading