-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support flake references to patches #3920
Comments
I seemed to have used the wrong template for a feature request 😬 |
@edolstra already suggested to add a {
inputs.nixpkgs = {
url = github:NixOS/nixpkgs/nixos-20.03;
patches = [
./foo.patch
./bar.patch
];
};
} Not sure (yet) how this can use |
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 |
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:
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:
This is not necessarily related to flakes. I suggested to simply pass this argument to the underlying code which uses the |
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):
If possible, I would prefer to have a more builtin way, like the proposed |
After several frustrating experiences with nixpkgs PRs, I've come around to this idea. I remove my apprehension |
That's great to hear 👍 |
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... (?) |
Resolves NixOS#3785 Might help NixOS#3920
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";
}
];
} |
This issue has been mentioned on NixOS Discourse. There might be relevant details there: |
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. |
If "primop" means avoid |
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 |
(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. |
@blaggacao Hence #3785. If patches are also flake inputs, half the battle is won |
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 As for patching source trees ( |
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. |
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. |
Are we actually +- concludently agreeing on a spec for this already? The use cases are:
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;
}
];
};
} |
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. |
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. |
I implemented patch support for |
You can now write fetchTree { type = "github"; owner = "NixOS"; repo = "nixpkgs"; rev = "0f316e4d72daed659233817ffe52bf08e081b5de"; patches = [ ./thunderbird-1.patch ./thunderbird-2.patch ]; }; to apply a list of patches to a tree. These are applied lazily - the patched tree is not materialized unless you do something that causes the entire tree to be copied to the store (like 'src = fetchTree { ... }'). The equivalent of '-p1' is implied. File additions/deletions/renames are not yet handled. Issue NixOS#3920.
You can now write fetchTree { type = "github"; owner = "NixOS"; repo = "nixpkgs"; rev = "0f316e4d72daed659233817ffe52bf08e081b5de"; patches = [ ./thunderbird-1.patch ./thunderbird-2.patch ]; }; to apply a list of patches to a tree. These are applied lazily - the patched tree is not materialized unless you do something that causes the entire tree to be copied to the store (like 'src = fetchTree { ... }'). The equivalent of '-p1' is implied. File additions/deletions/renames are not yet handled. Issue NixOS#3920.
You can now write fetchTree { type = "github"; owner = "NixOS"; repo = "nixpkgs"; rev = "0f316e4d72daed659233817ffe52bf08e081b5de"; patches = [ ./thunderbird-1.patch ./thunderbird-2.patch ]; }; to apply a list of patches to a tree. These are applied lazily - the patched tree is not materialized unless you do something that causes the entire tree to be copied to the store (like 'src = fetchTree { ... }'). The equivalent of '-p1' is implied. File additions/deletions/renames are not yet handled. Issue NixOS#3920.
You can now write fetchTree { type = "github"; owner = "NixOS"; repo = "nixpkgs"; rev = "0f316e4d72daed659233817ffe52bf08e081b5de"; patches = [ ./thunderbird-1.patch ./thunderbird-2.patch ]; }; to apply a list of patches to a tree. These are applied lazily - the patched tree is not materialized unless you do something that causes the entire tree to be copied to the store (like 'src = fetchTree { ... }'). The equivalent of '-p1' is implied. File additions/deletions/renames are not yet handled. Issue NixOS#3920.
You can now write fetchTree { type = "github"; owner = "NixOS"; repo = "nixpkgs"; rev = "0f316e4d72daed659233817ffe52bf08e081b5de"; patches = [ ./thunderbird-1.patch ./thunderbird-2.patch ]; }; to apply a list of patches to a tree. These are applied lazily - the patched tree is not materialized unless you do something that causes the entire tree to be copied to the store (like 'src = fetchTree { ... }'). The equivalent of '-p1' is implied. File additions/deletions/renames are not yet handled. Issue NixOS#3920.
You can now write fetchTree { type = "github"; owner = "NixOS"; repo = "nixpkgs"; rev = "0f316e4d72daed659233817ffe52bf08e081b5de"; patches = [ ./thunderbird-1.patch ./thunderbird-2.patch ]; }; to apply a list of patches to a tree. These are applied lazily - the patched tree is not materialized unless you do something that causes the entire tree to be copied to the store (like 'src = fetchTree { ... }'). The equivalent of '-p1' is implied. File additions/deletions/renames are not yet handled. Issue NixOS#3920.
It seems using the changed I wanted to patch # 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
The resulting attrset has none of the flake metadata but does have the I suppose this is only viable because the |
@dbaynard The downside with that approach is that it requires import-from-derivation (namely |
Would this UX make sense? {
inputs = rec {
patch1 = { url = "https://github.com/nix-community/nixops-vbox/pull/27.patch"; flake=false;};
patch2 = { url = "https://github.com/nix-community/nixops-vbox/pull/28.patch"; flake=false;};
nixops-vbox = { url = github:nix-community/nixops-vbox; patches = [patch1 patch2];};
};
outputs = { nixops-vbox, ... }: {};
} Possibly related to #4945 because I'd just use a mix of |
@yajo, I like the concept of grabbing patches remotely, but perhaps a nicer ux would be to just assume that if a patch starts with "http(s)://" that it's a remote resource and should be fetched. That way, we could just write:
The resource could then be locked in the lockfile under the input |
This issue has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/proper-way-of-applying-patch-to-system-managed-via-flake/21073/18 |
This issue has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/how-to-override-let-variables/23741/7 |
This issue has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/proper-way-of-applying-patch-to-system-managed-via-flake/21073/26 |
I just ran into this while trying to test out a PR to nixpkgs master. I guess the alternative is to create my own nixpkgs fork with the PR applied. |
In the real world, when someone doesnt do his job right, others have to do it for him. |
Is there any recommended pattern for doing this with multiple unmerged PRs, until this fix exists? Comparing making a single "everything merged" fork-of-forks versus overlaying multiple nixpkgs repos and overriding, both have significant manual overhead. |
I came across this comment a while ago NixOS/nixpkgs#142273 (comment); if you refine it to just include stuff related to patches, you get the following which I've used in my let
system = "x86_64-linux";
patches = [
{
url = "https://patch-diff.githubusercontent.com/raw/NixOS/nixpkgs/pull/268168.patch";
hash = "sha256-WIMDnmZV0eL1eFVD0ldHUBrulZWsjdFOmcN4i8+RgFA=";
}
];
originPkgs = nixpkgs.legacyPackages.${system};
patchednixpkgs = originPkgs.applyPatches {
name = "nixpkgs-patched";
src = nixpkgs;
patches = map originPkgs.fetchpatch patches;
};
in
{
nixpkgs = import patchednixpkgs { inherit system; };
}; It's not as nice as it being available at the flake inputs level but the syntax is not too bad. Also, I'm not sure of all the implications of doing this, maybe there are some use cases not supported. But I thought it might be worth mentioning here. |
That's an IFD AFAIK. Not nice indeed. |
This issue has been mentioned on NixOS Discourse. There might be relevant details there: |
Any progress here? I wanted to patch arbitrary inputs, so the |
I made a workaround for keeping my "fork" up to date using https://github.com/katrinafyi/nix-patcher , added notes on it at my README.md |
Describe the bug
There is a patch in nixpkgs NixOS/nixpkgs#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 👎 from @edolstra who commented the following as the most early retort:
This idea with flakes piqued my interest as possible, though.
@edolstra did eventually see the usefulness of such a functionality after the following comments NixOS/nixpkgs#59990 (comment) NixOS/nixpkgs#59990 (comment)
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 NixOS/nixpkgs#59990
@basvandijk @volth @edolstra
The text was updated successfully, but these errors were encountered: