fetchpatch: add relative

[ncfavier]

Allows restricting patches to a specific subdirectory, à la git diff --relative=subdir.

This cannot be done (cleanly) currently because the includes logic happens after stripLen is applied, so we can't match on subdir/*. This change adds a preIncludes argument to make this possible, as well as a relative meta-argument that takes the name of a subdirectory and sets preIncludes, stripLen and extraPrefix accordingly.

(I turned meta.broken into assertions as it seems cleaner (#43538 (review)).)

As a proof of concept, I changed every instance of fetchpatch { stripLen > 0; includes = [ ... ]; } in nixpkgs (checked by adding a builtins.trace and evaluating everything) to use relative instead. This changes some hashes because (besides the conversion to SRI) previously people would use stripLen to transform a/subdir/file into subdir/file, while with relative you get a/file instead.

There might be more applications in the haskell-updates branch, and in fact the whole motivation for this was #165014 (comment).

Example

fetchpatch {
  url = "https://github.com/lambdabot/lambdabot/pull/204.patch";
  relative = "lambdabot-core";
  sha256 = "...";
}

[maralorn]

This is so beautiful. I am not sure if we need all of those options, but relative is just amazing.

[maralorn]

Do we have a way to test if this invalidates any other hashes in nixpkgs? (Although I assume a test like that would give us false positives because of unstable sources …)

[ncfavier]

Do we have a way to test if this invalidates any other hashes in nixpkgs?

Look at the source while assuming relative == null and preIncludes == []. The only difference is that now extraPrefix is escaped, which from a look at rg extraPrefix doesn't change anything. (Well, and lsdiff -p1 but that doesn't do anything if there are no includes.)

[maralorn]

I just randomly invalidated a random hash. something called guitarix uses a fetchpatch with stripLen = 1.

Got this:

-p given without -i or -x; guessing that you meant --strip instead.
error: Normalized patch '/build/patch' is empty (while the fetched file was not)!
Did you maybe fetch a HTML representation of a patch instead of a raw patch?
Fetched file was:
From d8f003484c57d808682025dfb07a7a1fb848afdc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Hubert=20Figui=C3=A8re?= <hub@figuiere.net>
Date: Sun, 25 Apr 2021 11:21:23 -0400
Subject: [PATCH] Issue #63 - Fix build on gcc 11

---
 trunk/src/headers/gx_system.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/trunk/src/headers/gx_system.h b/trunk/src/headers/gx_system.h
index d334ecfc..88d97567 100644
--- a/trunk/src/headers/gx_system.h
+++ b/trunk/src/headers/gx_system.h
@@ -132,7 +132,7 @@ inline T *atomic_get(T*& p) {
 
 template <class T>
 inline bool atomic_compare_and_exchange(T **p, T *oldv, T *newv) {
-    return g_atomic_pointer_compare_and_exchange(reinterpret_cast<void* volatile*>(p), static_cast<void*>(oldv), newv);
+    return g_atomic_pointer_compare_and_exchange(reinterpret_cast<void**>(p), static_cast<void*>(oldv), newv);
 }

[ncfavier]

(Well, and lsdiff -p1 but that doesn't do anything if there are no includes.)

-p given without -i or -x; guessing that you meant --strip instead.

🤦

Fixed now.

[maralorn]

I have invalidated a 6 interesting looking hashes for different fetchpatches all over nixpkgs, with and without stripLen. They all seemed fine.

I think this change is really nice. But because this is a rather central piece of infrastructure I would prefer to have another pair of eyes on this.

[roberth]

Please add tests in tests.fetchpatch aka pkgs/build-support/fetchpatch/tests.nix.

Testing fixed-output derivations is tricky without invalidateFetcherByDrvHash, because successes are cached, so we really need those test cases.

[sternenseemann]

I'd drop preIncludes, it is unnecessarily confusing and usually not needed. If needs be you can either use includes and an appropriate -p flag in patchFlags or get the patch using multiple relative fetchpatches.

[ncfavier]

I'd drop preIncludes, it is unnecessarily confusing and usually not needed. If needs be you can either use includes and an appropriate -p flag in patchFlags or get the patch using multiple relative fetchpatches.

Agreed, dropped it. I also realised there's no good reason to disallow using stripLen with relative, so that simplifies things quite a bit.

Please add tests in tests.fetchpatch aka pkgs/build-support/fetchpatch/tests.nix.

Added a simple relative test and a full test that uses everything. The produced patch looks "correct" (maybe it's overkill though).

[roberth]

@ofborg build tests.fetchpatch

[7c6f434c]

Hm, it looks like you drop a few includes restrictions; is it intentional?

[ncfavier]

What do you mean? The only restriction on includes is that it cannot be used with excludes. I moved that assertion up into a throwIfNot.

[7c6f434c]

I look at the package changes. It looks like the goal was to move from stripLen`` to relativewhere it simplifies things, but commit messages contain no indication whyincludes` parameters were removed. Weere they checked to be redundant?

[ncfavier]

Oh, yes, I looked at each patch carefully. The only one whose includes wasn't redundant is the first CGAL patch, but I saw that the patch was only replacing NULL with nullptr (CGAL/cgal#3979) and the build still succeeded so I left the whole directory in. (Sorry, should have mentioned that)