Nix: 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).