Support flake references to patches

[worldofpeace]

Describe the bug

There is a patch in nixpkgs https://github.com/NixOS/nixpkgs/pull/59990 "Support patching nixpkgs" which is meant to support users who 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. This is really common flow for linux users in general. What people currently do is 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. IMHO a kinda consuming process if you just want a few patches.

Overall, this patch was a :-1: from @edolstra who commented the following as the most early retort:

I'm -1 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.

This idea with flakes piqued my interest as possible, though.

@edolstra did eventually see the usefulness of such a functionality after the following comments https://github.com/NixOS/nixpkgs/pull/59990#issuecomment-490876722 https://github.com/NixOS/nixpkgs/pull/59990#issuecomment-490882052

I've opened this issue, as I'm not entirely sure how this should look like with flakes, but it is certainly something I'd like to have and would be an instant user of (and judging from the thumbs up on the PR in nixpkgs it would be welcome to many others).

Pinging people from https://github.com/NixOS/nixpkgs/pull/59990 @basvandijk @volth @edolstra

[worldofpeace]

I seemed to have used the wrong template for a feature request :grimacing:

[Ma27]

@edolstra already suggested to add a patches-argument to fetchTarball[1]. As this is basically part of the new fetchTree-API, it could look like this with flakes:

{
  inputs.nixpkgs = {
    url = github:NixOS/nixpkgs/nixos-20.03;
    patches = [
      ./foo.patch
      ./bar.patch
    ];
  };
}

Not sure (yet) how this can use fetchpatch then as this is part of nixpkgs.

[1] https://github.com/NixOS/nixpkgs/pull/59990#issuecomment-490882052

[bqv]

I'm not sure I'm a fan of this idea. It's simple enough to do that patching outside the context of flake setup, not to mention nixpkgs itself supports disabledModules etc. anyway. I think this would overcomplicate the mechanics of flakes

[Ma27]

It's simple enough to do that patching outside the context of flake setup

I'm currently doing this for my personal machines and my infra and I don't really think it is.

Also, when people ask me how I deal with my patches (that aren't on NixOS master/stable) I explain my approach (with those tracking branches) to them and we usually agree pretty fast that this is not really suitable for someone who's new to NixOS or not a poweruser (which I think I am). So we should either add a feature as suggested above or document those approaches pretty well.

Additionally, I'd like to quote @worldofpeace's original comment here:

@edolstra did eventually see the usefulness of such a functionality after the following comments NixOS/nixpkgs#59990 (comment) NixOS/nixpkgs#59990 (comment)

not to mention nixpkgs itself supports disabledModules etc. anyway

I don't like this kind of monkey-patching approach (and I think I've heard similar opinions from a few more NixOS users) for the following reasons:

• When I have e.g. a small bugfix that is in a pending PR (or not backported yet), I don't want to maintain a full copy of a module, a small patch applied on top would be way easier.
• You rely on implementation details like the file-structure of nixpkgs/nixos.
• Modules tend to go through many changes during e.g. a release-cycle. If you only want to use a specific feature on your stable NixOS from the master-version of a module, it's IMHO way easier to use the patch rather than disabledModules. This will eventually lead to even more patching of the module-system since modules can affect each other and you'll probably have to "replace" even more modules.

I think this would overcomplicate the mechanics of flakes

This is not necessarily related to flakes. I suggested to simply pass this argument to the underlying code which uses the libfetchers-API that is also used for builtins.fetchTree/builtins.fetchTarball/etc and it was already suggested to add a patches-functionality to those primops, so this is something that can be implemented on the libfetchers-level (in Nix's source) and thus also used for flake-inputs.

[ju1m]

If that may help, here's the workaround I'm currently using to cherry-pick patches against Nixpkgs (not sure at all that everything is correct wrt. using flakes, but at least it's not breaking obviously to me):

{
inputs.nixpkgs.url = "github:NixOS/nixpkgs/a332da8588aeea4feb9359d23f58d95520899e3c";
inputs.flake-utils.url = "github:numtide/flake-utils";
outputs = flakes: let
  remoteNixpkgsPatches = [
    { meta.description = "sanoid: fix sanoid.conf generation";
      url = "https://github.com/NixOS/nixpkgs/pull/83904.diff";
      sha256 = "Sy9wPmL+Lfl7hMEeXYOEMk3KlfdL21aL92v6MiGajds=";
    }
    { meta.description = "nixos/public-inbox: init";
      url = "https://github.com/NixOS/nixpkgs/pull/77450.diff";
      sha256 = "13ikg7chpbf6rrg5sngbdb95q3awhdgl4g8vci42xmqyf208hzzd";
    }
    { meta.description = "apparmor: fix and improve the service";
      url = "https://github.com/NixOS/nixpkgs/pull/93457.diff";
      sha256 = "8QNOKDWNJiZujAvmDtZPZI5VRc+oh40IqjTYn/RCUi4=";
    }
    { meta.description = "nixos/security.gnupg: provisioning GnuPG-protected secrets through the Nix store";
      url = "https://github.com/NixOS/nixpkgs/pull/93659.diff";
      sha256 = "8prVG0VYXsTPwnGyAv7FlMh2M5MENX1NxkX/ql1lmHs=";
    }
  ];
  localNixpkgsPatches = [
    #nixpkgs/patches/fix-ld-nix.diff
    #nixpkgs/patches/fix-ld-nix-apparmor.diff
  ];
  originPkgs = flakes.nixpkgs.legacyPackages."x86_64-linux";
  nixpkgs = originPkgs.applyPatches {
    name = "nixpkgs-patched";
    src = flakes.nixpkgs;
    patches = map originPkgs.fetchpatch remoteNixpkgsPatches ++ localNixpkgsPatches;
    postPatch = ''
      patch=$(printf '%s\n' ${builtins.concatStringsSep " "
         (map (p: p.sha256) remoteNixpkgsPatches ++ localNixpkgsPatches)} |
        sort | sha256sum | cut -c -7)
      echo "+patch-$patch" >.version-suffix
    '';
  };
  lib = originPkgs.lib;
  machines = builtins.mapAttrs (machineName: machineConfig:
    let cfg = import machineConfig { inherit flakes; }; in
    import (nixpkgs + "/nixos/lib/eval-config.nix") (cfg // {
      extraArgs = {
        inherit machineName flakes;
        machines = flakes.self.nixosConfigurations;
      } // (cfg.extraArgs or {});
      modules = cfg.modules ++ [({pkgs, ...}: {
        system.nixos.versionSuffix = ".${
          lib.substring 0 8 (flakes.self.lastModifiedDate or flakes.self.lastModified)}.${
          flakes.self.shortRev or "dirty"}";
        system.nixos.revision = lib.mkIf (flakes.self ? rev) flakes.self.rev;
        nix.registry.nixpkgs.flake = nixpkgs;
        nix.package = pkgs.nixFlakes;
        nix.extraOptions = "experimental-features = nix-command flakes";
        nixpkgs.config = {};
        nixpkgs.overlays = import nixpkgs/overlays.nix;
        # Let 'nixos-version --json' know about the Git revision of this flake.
        system.configurationRevision = lib.mkIf (flakes.self ? rev) flakes.self.rev;
      })];
    }));
  in
  {
    nixosConfigurations = machines {
      losurdo = machines/losurdo.nix;
      mermet  = machines/mermet.nix;
    };
  }
  // flakes.flake-utils.lib.eachDefaultSystem (system:
    #let pkgs = flakes.nixpkgs.legacyPackages.${system}; in
    let
      pkgs = import nixpkgs {
        inherit system;
        config = {};
        overlays = import nixpkgs/overlays.nix;
      };
    in {
    devShell = import ./shell.nix { inherit flakes pkgs; };
    }
  );
}

If possible, I would prefer to have a more builtin way, like the proposed patches= attribute to inputs: it would feel less hacky and could maybe avoid to slow down builds by having to load originPkgs to get a NixOS configuration or pkgs in devShell.

[bqv]

After several frustrating experiences with nixpkgs PRs, I've come around to this idea. I remove my apprehension

[worldofpeace]

After several frustrating experiences with nixpkgs PRs, I've come around to this idea. I remove my apprehension

That's great to hear :+1:

[bqv]

It seems sensible to me that the patches themselves would also be flake references, to support being urls that can be updated like any other input, rather than having to manually download and keep the files up to date

Edit: nevermind, it seems flake references can't be files... (?)

[blaggacao]

While this proposal is not immediately clear to me, I strongly endorse the following:

(for sake of conciseness I opened another issue with my user story)

{
  inputs.foo.patches = [ # in order merged
     "abgc6h" # same remote
     "ref/pr/123" # same remote pull request
    { # long form (just like normal inputs)
       type = "github";
       org = "foo";
       repo = "bar";
       rev = "abgckh";
    }
  ];
}

[nixos-discourse]

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

https://discourse.nixos.org/t/flake-patch-inputs/10854/4

[FRidh]

I wonder then whether we need a new primop for applying a patch (a content-addressable path) to a content-addressable path producing a new content-addressable path. That could be used with non-flake code as well.

[blaggacao]

If "primop" means avoid bin/patch and possibly (in the far [nickel-]future) support semantic merges (so that potential merge conflicts for nickel-type source artifacts are reduced), then this represents an enticing through to me.

[FRidh]

It would mean there is a Nix builtin you call to apply one or more patches. How this is implemented, I don't know. Maybe it invokes patch impurely just as fetchGit invokes git. The important part is that this way you can apply patches anywhere in Nix code on any content addressable path. If we look at flakes, it would make it possible to not patch an input directly, but to conditionally apply it later on (which means a reimport) because, while you sometimes need a patch, the patch may not always be appropriate under all conditions.

[blaggacao]

If we look at flakes, it would make it possible to not patch an input directly, but to conditionally apply it later on (which means a reimport) because, while you sometimes need a patch, the patch may not always be appropriate under all conditions.

(At least remote) patches still ought to be inputs of a flake's encapsulation, so at any rate we need a input semantic for (at least fetching) them.

What would be the practical merits of conditional patch application? That rather seems like violating flake's encapsulation: any given input produces exactly one output and flake commands (afaik) to not provide arguments. Should variants be needed based on some exogenous conditions, those ought to be provided by git references. Their accessor is the fqdn of the flake's derivation itself, and hence the exogenous condition needs to be factored in by the user.

[bqv]

@blaggacao Hence #3785. If patches are also flake inputs, half the battle is won

[eadwu]

I do see how it can be useful, but I'm not sure if it's actually needed or if it should be even added since I don't think it'll be useful to anyone who isn't a "power-user".

For the direct use-case of patching nixpkgs here, I'm not too sure about any real advantages other than increased locality for a project configuration. Haphazardly monkey-patching nixpkgs can cause more problems (i.e. in the case where a new person to Nix just pulls random PRs that bump versions on master while using a stable channel and ends up missing dependencies bumps from other PRs and failing builds because of it) than just using the specific branch as a flake input (though we do have to consider API limits if we're pinging GitHub a lot). In the case of modules, the patching flow may be more convenient (to a certain extent) since without it'll probably require some disabledModules and import combinations otherwise. I guess my point is that when someone generally makes a PR, it'll probably (or it at least have a better chance of) work on the tree it's currently on (which probably isn't the commit your nixpkgs input is on) than your own tree (though this has the disadvantage of ballooning the closure size from derivations from different trees).

As for patching source trees (flake = false), I'm not sure if there's any real difference to just doing it in a derivation, though I guess it can be considered to be overkill somewhat if a derivation is created exclusively for patching, doubling the space usage of the input (though I suppose we can probably reduce by symlinks to everything that isn't patched).

[gytis-ivaskevicius]

It is possible to patch channels using something like this:

    patchChannel = channel: patches:
      if patches == [ ] then channel else
      (import channel { inherit system; }).pkgs.applyPatches {
        name = "nixpkgs-patched-${channel.shortRev}";
        src = channel;
        patches = patches;
      };

I used this technique for loading a couple of PR's and during development of nixpkgs (this allowed me to easily test changes without updating my channel or anything)

I'd love to see such change implemented properly.

[worldofpeace]

Exactly, I feel like it would be a greater joy to use nix if things didn't have to be done soo manually all the time.

[blaggacao]

Are we actually +- concludently agreeing on a spec for this already?

The use cases are:

• generating patches from the tip of remote branches, prominently: pull requests, but also forks (non-power-users)
• applying individual commits or a splice thereof (power user / tricky requirements)
• prototype and apply local patches (power-user)

Tentative UX

{
  inputs.nixpkgs = {
    url = github:NixOS/nixpkgs/nixos-20.03;
    patches = [
      ./foo.patch
      ./bar.patch
     "abctgf657" # commit-ish
     "ref/pr/123" # same remote pull request
    { # long form (just like normal inputs)
       type = "github";
       org = "foo";
       repo = "bar";
       rev = "abgckh";
      }
    ];
  };
}

EDIT

There is another one from the sister issue #3785

{
  inputs.nixpkgs = {
    url = github:NixOS/nixpkgs/nixos-20.03;
    patches = [
      "https://url.example.com/some.patch"
      {
        type =​ "​file​"​;
        urL =​ "​https://url.example.com/some.patch​"​;
        flake = false​;
      }
    ];
  };
}

[bqv]

I don't understand how long form would sensibly interact there.

[blaggacao]

I don't understand how long form would sensibly interact there.

Currently, you always need an escape hatch:

{ 
   org = "different";
   ref = "​my/branch/with/slashes​"​;
}

This is not parseable by the short form url.

---

To be consistent with current URL formatting, implementation might choose to omit the "special purpose" short forms

{
  patches = [
    "​abctgf657​" # cocommit-is
    "​ref/pr/123​" # same remote pull
  ];
}

which could also be fully qualified as

{
  patches = [
    "github:NixOS/nixpkgs/nixos-20.03/abctgf657​"​
    {
        type = "​github​"​;
        org = "​NixOS​"​;
        repo = "​nixpkgs​"​;
        ref = "​ref/pr/123​"​;
    }
  ];
}

If something resides on the same repo, maybe this can be shortened instead to:

{
  patches = [
    { rev = "​abctgf657​"​; }
    { ref = "​ref/pr/123​"​; }
  ];
}

That might be overall a saner implementation versus brittely extracting type from the shape of a string.

[blaggacao]

An extended / derivated use case: we might want to recommend this as a "blessed" community collaboration model to prototype library additions "as a (draft) PR", cutting out plumbing workflows for potential contributors.

For somebody with dedicated domain knowledge, but insufficient plumbing knowledge, burden the design those workflows onto the contributor has prevented valuable contributions in the past.

[milahu]

good news everyone, we seem to have the same dream ; )

https://github.com/milahu/nixos-patch-installer i use overlayfs to overlay new files over my /nix/store/*-nixpkgs this is fast, space-efficient, and allows to 'simply replace anything', to patch existing packages, and to patch existing modules without having to insert things like disabledModules into the new files

workflow: i select a pull request in https://github.com/NixOS/nixpkgs/pulls in the navigation bar, there is a new button "install patch" when i click that button, my backend downloads all the changed files (not the diffs, but the complete files) in the frontend of my app, i can audit the code changes (diff vs local files, diff vs github base, file contents, ...) when im happy with the patch, i click "confirm install" and my backend copies all the new files to the overlayfs, so when i call sudo nixos-rebuild switch, the new files are used .... magic : )

(the last "confirm install" step is not exposed to the frontend, but it does already work from the backend CLI)

point-and-click for the win! ; )

another motivation is: make it easier to test new code from pull requests, the help review the ~2000 PRs

[blaggacao]

In the context of devos / flake-utils-plus there are a bunch of people who would have very much interest in this to polish our social sharing model, while encouraging upstream contributions as much as possible by making them trivial to conceptualize and maintain.

If you have enough C skills and are interested in working together on this, please contact anyone of:

@gytis-ivaskevicius @nrdxp, @Pacman99 or me

[gytis-ivaskevicius]

As much of pain all this patching gave us - now thanks to @chvp flake-utils-plus has quite a solid patching support! Please do check it out :)

[blaggacao]

While this thread has all the discussion, I updated https://github.com/NixOS/nix/issues/4433 for a consolidated "spec" / TLDR view of this proposal.

[nixos-discourse]

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

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

[nixos-discourse]

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

https://discourse.nixos.org/t/flakes-nix3-0-issues/15626/1

[edolstra]

I implemented patch support for fetchTree, which could be extended to flake inputs (https://github.com/edolstra/nix/commit/ffd26226b81e6cc51197ca101ffb926b5e44a841). Since this is part of the lazy trees branch, it doesn't require writing a complete copy of e.g. nixpkgs to disk just to apply the patch.

[dbaynard]

It seems using the changed fetchTree will be a great solution, but for now I have written a short workaround. I haven’t seen it elsewhere, possibly because a one line flake evaluator feels a little evil…

I wanted to patch nix-darwin.

  # No `darwin` here as I define it later
  outputs = { self, nixpkgs, ... }@inputs:
    let
      # Store just the contents of my `patches` directory
      patches = builtins.path {
        name = "patches";
        path = ./patches;
      };

      darwin =
        let
          src = nixpkgs.legacyPackages."aarch64-darwin".applyPatches {
            name = "nix-darwin";
            src = inputs.darwin;
            patches = [ "${patches}/fonts.patch" ];
          };
        in
        # The evaluator
        nixpkgs.lib.fix (self: (import "${src}/flake.nix").outputs { inherit self nixpkgs; });
    in
      … # Expressions which use `darwin.lib.darwinSystem`

So that is

1. import "${src}/flake.nix" to import the flake.nix file from the patched directory, as an expression
2. _.outputs to extract the outputs.
3. self: _ { inherit self nixpkgs; } to evaluate outputs in terms of itself, passing nixpkgs from my own flake inputs.
4. nixpkgs.lib.fix _ to find the fixed point — in this case, an attrset.

The resulting attrset has none of the flake metadata but does have the check. darwinModules and lib attrs, like the source darwin flake. This should be fine? As long as I don’t need any of the flake metadata — I think I don’t.

I suppose this is only viable because the flake.nix is so restricted in terms of its contents (so well done with the design).