stdenv: add applyPatches and support patch directories #335579
Conversation
github-actions
bot
added
8.has: documentation
ofborg
bot
added
10.rebuild-darwin-stdenv
|
I'm of two minds about this PR. On the one hand, I like the way that On the other hand, as a reviewer, I don't like how it's harder to validate which patches get applied. I like that I can be assured that adding a patch means I get a cue in the Adding a patch file and "magically" having it be applied feels not great to me. Maybe my intuition is off here. I'd love to here from others. |
|
Edit: related to https://savannah.gnu.org/bugs/?57717 I really have a bad luck for encountering bugs in every piece of software I touch… I’m adding tests for an existing applyPatches trivial builder (and applyPatches function by extension) and the following test case fails (as expected) because patchWithIncorrectFilePath = testers.testBuildFailure (applyPatches {
src = writeTextDir "file1.txt" "content";
patches = [
(writeText "incorrect-path.patch" ''
diff --git a/file2.txt b/file2.txt
index e69de29..d20e9cd 100644
--- a/file2.txt
+++ b/file2.txt
@@ -1 +1 @@
-old content
+new content
'')
];
})Standalone reproducer: $ uname -a
Linux utm 6.6.32 #1-NixOS SMP Sat May 25 14:22:56 UTC 2024 aarch64 GNU/Linux$ uname -a
Linux saitama 6.6.32 #1-NixOS SMP PREEMPT_DYNAMIC Sat May 25 14:22:56 UTC 2024 x86_64 GNU/Linux$ patch --version
GNU patch 2.7.6
(GNU copyright notice omitted)$ patch -p1 <<'EOF'
diff --git a/file2.txt b/file2.txt
index e69de29..d20e9cd 100644
--- a/file2.txt
+++ b/file2.txt
@@ -1 +1 @@
-old content
+new content
EOFSpams To be fair, the patch file for this test case was generated by LLM, and |
An existing patch file can be updated to add more changes without updating package’s Patch directories are useful for managing patches using tools like Also, this is useful for applying patch sets from other derivations, e.g. using fetchurl from Debian (that uses That is, for simple one-off patches in Nixpkgs, I’d expect maintainers to continue adding individual patch file paths to the |
tie
force-pushed
the
stdenv-patch-dir
branch
2 times, most recently
from
7bd20ea to
a822353
Compare
|
Pushed a commit to fix #335579 (comment). This PR ready for review. Let me know if I should open separate PRs for 66082f9814e2085d90b0df4b940664f886523d54 and e245b88996d8bbf6939ab37aeb18f675cbc2e91e. |
tie
requested review from
kalbasit,
katexochen,
Mic92 and
zowoq
as code owners
|
Seems like a reasonable change to me, and the Bash code seems high quality as always. I take it the I’m wondering if we should require a |
| When processing a directory without a `series` file, only files with standard | ||
| patch extensions `.patch` and `.diff` will be considered for application, | ||
| optionally with compressed file name extension (e.g. `.patch.gz`). |
There was a problem hiding this comment.
Will they be sorted or is there non-determinism based on inode order? 👀
There was a problem hiding this comment.
I agree with #335579 (comment), I’ll update applyPatches to return an error if series file does not exist to avoid implicit ordering.
There was a problem hiding this comment.
Requiring a series file addresses this possibility definitively. I can get on board with that.
| # Docs in doc/build-helpers/trivial-build-helpers.chapter.md | ||
| # See https://nixos.org/manual/nixpkgs/unstable/#trivial-builder-applyPatches |
| # We support a simple series file with a list of patches. Empty | ||
| # lines and comments are ignored. Guards-like conditions (see | ||
| # guards(1) from quilt) are explicitly forbidden to avoid breaking | ||
| # changes if support is added in the future. |
There was a problem hiding this comment.
Stupid line breaking suggestions to avoid enjambment.
| # We support a simple series file with a list of patches. Empty | |
| # lines and comments are ignored. Guards-like conditions (see | |
| # guards(1) from quilt) are explicitly forbidden to avoid breaking | |
| # changes if support is added in the future. | |
| # We support a simple series file with a list of patches. | |
| # Empty lines and comments are ignored. Guards-like conditions | |
| # (see guards(1) from quilt) are explicitly forbidden | |
| # to avoid breaking changes if support is added in the future. |
| # NB nullglob is set in stdenv. | ||
| patchFiles+=( "$p"/*.{patch,diff}{,.gz,.bz2,.xz,.lzma} ) |
There was a problem hiding this comment.
Here's where I suspect that inode order non-determinism is creeping in.
There was a problem hiding this comment.
Bash filename expansions are sorted alphabetically.
https://www.gnu.org/software/bash/manual/html_node/Filename-Expansion.html#:~:text=If%20one%20of%20these%20characters%20appears,matching%20the%20pattern
This is locale dependent (not an issue in Nix sandbox) but not affected by inode order.
There was a problem hiding this comment.
That might be painful for nix develop. We should probably set locale variables for that, I guess…
I’d consider this kind of thing another (weak) argument for requiring a series file.
There was a problem hiding this comment.
Ugh, locales. This pushes me over the line into series-file-required land.
There was a problem hiding this comment.
Just double checking that it is indeed locale-dependent.
There was a problem hiding this comment.
you could sort it after the fact?
| @@ -0,0 +1,51 @@ | |||
| From b7b028a77bd855f6f56b17c8837fc1cca77b469d Mon Sep 17 00:00:00 2001 | |||
| From: Andreas Gruenbacher <agruen@gnu.org> | |||
| Date: Fri, 28 Jun 2019 00:30:25 +0200 | |||
There was a problem hiding this comment.
Has there not been a patch release since this!?
https://git.savannah.gnu.org/cgit/patch.git shows the answer is yes: 2.7.6 was tagged 7 years ago.
There was a problem hiding this comment.
Yes, no releases for 7 years, Debian applies this patch as well.
There was a problem hiding this comment.
I would be in favour of bumping to a Git revision given the number of fixes since then.
I think we should split out the two commits you mentioned into separate PRs so we can discuss things like that.
There was a problem hiding this comment.
Yes, absolutely agreed that a git version is the right call vs. vendoring the accumulated patches.
There was a problem hiding this comment.
https://git.savannah.gnu.org/cgit/patch.git/commit/?id=dce4683cbbe107a95f1f0d45fabc304acfb5d71a looks like the only potentially‐breaking interface change at a glance, but we can probably handle it. We have so many patches for patch right now and five of them are CVEs, we should just bump and nag upstream to cut a release.
(But yeah let’s talk about this on another PR!)
|
Another potential reason to require a |
|
The one downside of the series file is that you can delete the entry in the series and orphan a patch file in the repository. Removing that possibility was one reason that this PR exists, as I understand it. I do broadly agree with the sentiment that a series file is a good idea, following @emilazy's argumentation. But if it's a good idea, isn't it basically just another level of indirection on listing out the patch files in the |
Perhaps it could error out if there are files in the directory not mentioned in the series file. That would be restrictive, and maybe we’d regret it down the line, but it’d be easier to loosen the constraint than to add it later. |
|
I’m ambivalent about the source‐of‐truth thing. It doesn’t feel too different to a Nix file importing another. Being able to use other tools to work with series files might be helpful. One thing this change lets us do that might be cool is adding an entire Debian package’s patches directory to |
|
OK, I can say ✅ to this with series file required in the directory case and a check that there aren't excess patches hanging about. Think that's reasonable? |
I agree with that. I can't find a better solution to support those It really comes down to this I guess:
I'm not sure. |
|
Not sure about the additional check for excess patches. I’m OK with that, but not sure what a potential implementation should do. E.g. directory can have a I think it’s fine to omit the check. It’s a lot easier to miss a patch that should be removed when it’s stored alongside a bunch of other required files that are conditionally included from multiple Nix files. Patch set directory, on the other hand, is functionally equivalent to concatenating multiple patches into single file, and ideally managed with some tooling (be that quilt or manual git format-patch exports). |
See https://savannah.gnu.org/bugs/?57717 Reproducer: ```console $ patch -p1 <<'EOF' diff --git a/file2.txt b/file2.txt index e69de29..d20e9cd 100644 --- a/file2.txt +++ b/file2.txt @@ -1 +1 @@ -old content +new content EOF ``` ``` (repeated lines omitted) patch: **** Can't create file file2.txt.orig : Too many open files patch: **** Can't create file file2.txt.orig : Too many open files patch: **** Can't create file file2.txt.origSegmentation fault (core dumped) ```
wegank
added
the
2.status: merge conflict
|
(I think you mean another PR) |
|
@philiptaron, sorry for the late reply, I’m taking a small break from open source contributions until October. |
|
Take care; Nixpkgs will be here when you get back :) |
Description of changes
This PR adds applyPatches function to the stdenv that supports patch directories and refactors the default
patchPhaseto use it. Also documentsapplyPatchestrivial builder in Nixpkgs manual.Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.