fetchpatch breaks patches by reoordering hunks

[9999years]

Describe the bug

When fetchpatch sorts hunks, it can move hunks that change a file before the hunk that creates the file. This can cause the patch to fail to apply when patch attempts to modify a file that doesn't exist yet.

For example, this patch URL:

https://gitlab.haskell.org/ghc/ghc/-/merge_requests/9897.patch

Gets written to the Nix store like this; note the hunks which modify m4/fp_ld_supports_response_files.m4 have been moved before the hunk that creates it, an issue not present with the original commit ordering.

GHC Merge Request 9897

--- a/compiler/GHC/Settings.hs
+++ b/compiler/GHC/Settings.hs
@@ -19,6 +19,7 @@
   , sGlobalPackageDatabasePath
   , sLdSupportsCompactUnwind
   , sLdSupportsFilelist
+  , sLdSupportsResponseFiles
   , sLdIsGnuLd
   , sGccSupportsNoPie
   , sUseInplaceMinGW
@@ -87,6 +88,7 @@
 data ToolSettings = ToolSettings
   { toolSettings_ldSupportsCompactUnwind :: Bool
   , toolSettings_ldSupportsFilelist      :: Bool
+  , toolSettings_ldSupportsResponseFiles :: Bool
   , toolSettings_ldIsGnuLd               :: Bool
   , toolSettings_ccSupportsNoPie         :: Bool
   , toolSettings_useInplaceMinGW         :: Bool
@@ -189,6 +191,8 @@
 sLdSupportsCompactUnwind = toolSettings_ldSupportsCompactUnwind . sToolSettings
 sLdSupportsFilelist :: Settings -> Bool
 sLdSupportsFilelist = toolSettings_ldSupportsFilelist . sToolSettings
+sLdSupportsResponseFiles :: Settings -> Bool
+sLdSupportsResponseFiles = toolSettings_ldSupportsResponseFiles . sToolSettings
 sLdIsGnuLd :: Settings -> Bool
 sLdIsGnuLd = toolSettings_ldIsGnuLd . sToolSettings
 sGccSupportsNoPie :: Settings -> Bool
--- a/compiler/GHC/Settings/IO.hs
+++ b/compiler/GHC/Settings/IO.hs
@@ -95,6 +95,7 @@
       cxx_args = words cxx_args_str
   ldSupportsCompactUnwind <- getBooleanSetting "ld supports compact unwind"
   ldSupportsFilelist      <- getBooleanSetting "ld supports filelist"
+  ldSupportsResponseFiles <- getBooleanSetting "ld supports response files"
   ldIsGnuLd               <- getBooleanSetting "ld is GNU ld"
   arSupportsDashL         <- getBooleanSetting "ar supports -L"
 
@@ -163,6 +164,7 @@
     , sToolSettings = ToolSettings
       { toolSettings_ldSupportsCompactUnwind = ldSupportsCompactUnwind
       , toolSettings_ldSupportsFilelist      = ldSupportsFilelist
+      , toolSettings_ldSupportsResponseFiles = ldSupportsResponseFiles
       , toolSettings_ldIsGnuLd               = ldIsGnuLd
       , toolSettings_ccSupportsNoPie         = gccSupportsNoPie
       , toolSettings_useInplaceMinGW         = useInplaceMinGW
--- a/compiler/GHC/SysTools/Tasks.hs
+++ b/compiler/GHC/SysTools/Tasks.hs
@@ -29,7 +29,6 @@
 import GHC.Utils.Misc
 import GHC.Utils.Logger
 import GHC.Utils.TmpFs
-import GHC.Utils.Constants (isWindowsHost)
 import GHC.Utils.Panic
 
 import Data.List (tails, isPrefixOf)
@@ -350,9 +349,7 @@
             , "does not support object merging." ]
         optl_args = map Option (getOpts dflags opt_lm)
         args2     = args0 ++ args ++ optl_args
-    -- N.B. Darwin's ld64 doesn't support response files. Consequently we only
-    -- use them on Windows where they are truly necessary.
-    if isWindowsHost
+    if toolSettings_ldSupportsResponseFiles (toolSettings dflags)
       then do
         mb_env <- getGccEnv args2
         runSomethingResponseFile logger tmpfs dflags id "Merge objects" p args2 mb_env
--- a/configure.ac
+++ b/configure.ac
@@ -663,6 +663,8 @@
 FP_LD_NO_FIXUP_CHAINS([target], [CONF_GCC_LINKER_OPTS_STAGE1])
 FP_LD_NO_FIXUP_CHAINS([target], [CONF_GCC_LINKER_OPTS_STAGE2])
 
+FP_LD_SUPPORTS_RESPONSE_FILES()
+
 GHC_LLVM_TARGET_SET_VAR
 # we intend to pass trough --targets to llvm as is.
 LLVMTarget_CPP=`    echo "$LlvmTarget"`
--- a/configure.ac
+++ b/configure.ac
@@ -663,7 +663,7 @@
 FP_LD_NO_FIXUP_CHAINS([target], [CONF_GCC_LINKER_OPTS_STAGE1])
 FP_LD_NO_FIXUP_CHAINS([target], [CONF_GCC_LINKER_OPTS_STAGE2])
 
-FP_LD_SUPPORTS_RESPONSE_FILES()
+FP_LD_SUPPORTS_RESPONSE_FILES
 
 GHC_LLVM_TARGET_SET_VAR
 # we intend to pass trough --targets to llvm as is.
--- a/distrib/configure.ac.in
+++ b/distrib/configure.ac.in
@@ -180,6 +180,8 @@
 FP_LD_NO_FIXUP_CHAINS([target], [CONF_GCC_LINKER_OPTS_STAGE1])
 FP_LD_NO_FIXUP_CHAINS([target], [CONF_GCC_LINKER_OPTS_STAGE2])
 
+FP_LD_SUPPORTS_RESPONSE_FILES
+
 AC_SUBST(CONF_CC_OPTS_STAGE0)
 AC_SUBST(CONF_CC_OPTS_STAGE1)
 AC_SUBST(CONF_CC_OPTS_STAGE2)
--- a/hadrian/bindist/Makefile
+++ b/hadrian/bindist/Makefile
@@ -92,6 +92,7 @@
 	@echo ',("ld flags", "$(SettingsLdFlags)")' >> $@
 	@echo ',("ld supports compact unwind", "$(LdHasNoCompactUnwind)")' >> $@
 	@echo ',("ld supports filelist", "$(LdHasFilelist)")' >> $@
+	@echo ',("ld supports response files", "$(LdSupportsResponseFiles)")' >> $@
 	@echo ',("ld is GNU ld", "$(LdIsGNULd)")' >> $@
 	@echo ',("Merge objects command", "$(SettingsMergeObjectsCommand)")' >> $@
 	@echo ',("Merge objects flags", "$(SettingsMergeObjectsFlags)")' >> $@
--- a/hadrian/bindist/config.mk.in
+++ b/hadrian/bindist/config.mk.in
@@ -236,6 +236,7 @@
 
 GccExtraViaCOpts = @GccExtraViaCOpts@
 LdHasFilelist = @LdHasFilelist@
+LdSupportsResponseFiles = @LdSupportsResponseFiles@
 LdHasBuildId = @LdHasBuildId@
 LdHasFilelist = @LdHasFilelist@
 LdIsGNULd = @LdIsGNULd@
--- a/hadrian/cfg/system.config.in
+++ b/hadrian/cfg/system.config.in
@@ -140,6 +140,7 @@
 gcc-extra-via-c-opts = @GccExtraViaCOpts@
 ld-has-no-compact-unwind = @LdHasNoCompactUnwind@
 ld-has-filelist = @LdHasFilelist@
+ld-supports-response-files = @LdSupportsResponseFiles@
 ld-is-gnu-ld = @LdIsGNULd@
 ar-args = @ArArgs@
 
--- a/hadrian/src/Rules/Generate.hs
+++ b/hadrian/src/Rules/Generate.hs
@@ -427,6 +427,7 @@
         , ("ld flags", expr $ settingsFileSetting SettingsFileSetting_LdFlags)
         , ("ld supports compact unwind", expr $ lookupSystemConfig "ld-has-no-compact-unwind")
         , ("ld supports filelist", expr $ lookupSystemConfig "ld-has-filelist")
+        , ("ld supports response files", expr $ lookupSystemConfig "ld-supports-response-files")
         , ("ld is GNU ld", expr $ lookupSystemConfig "ld-is-gnu-ld")
         , ("Merge objects command", expr $ settingsFileSetting SettingsFileSetting_MergeObjectsCommand)
         , ("Merge objects flags", expr $ settingsFileSetting SettingsFileSetting_MergeObjectsFlags)
--- a/m4/fp_ld_supports_response_files.m4
+++ b/m4/fp_ld_supports_response_files.m4
@@ -1,6 +1,6 @@
 # FP_LD_SUPPORTS_RESPONSE_FILES
 # --------------------
-# See if whether we are using a version of ld64 on darwin platforms which
+# See if whether we are using a version of ld which
 # supports response files.
 AC_DEFUN([FP_LD_SUPPORTS_RESPONSE_FILES], [
     AC_MSG_CHECKING([whether $LD supports response files])
--- a/m4/fp_ld_supports_response_files.m4
+++ b/m4/fp_ld_supports_response_files.m4
@@ -6,7 +6,7 @@
     AC_MSG_CHECKING([whether $LD supports response files])
     echo 'int main(void) {return 0;}' > conftest.c
     $CC -c -o conftest.o conftest.c > /dev/null 2>&1
-    if $LD @<(printf '%q\n' -o conftest conftest.o) > /dev/null 2>&1
+    if $LD @<(printf '%q\n' -shared -o conftest conftest.o) > /dev/null 2>&1 || $LD @<(printf '%q\n' -dylib -o conftest conftest.o) > /dev/null 2>&1
     then
         LdSupportsResponseFiles=YES
         AC_MSG_RESULT([yes])
--- a/m4/fp_ld_supports_response_files.m4
+++ b/m4/fp_ld_supports_response_files.m4
@@ -1,12 +1,11 @@
 # FP_LD_SUPPORTS_RESPONSE_FILES
 # --------------------
-# See if whether we are using a version of ld which
-# supports response files.
+# See if whether we are using a version of ld which supports response files.
 AC_DEFUN([FP_LD_SUPPORTS_RESPONSE_FILES], [
     AC_MSG_CHECKING([whether $LD supports response files])
     echo 'int main(void) {return 0;}' > conftest.c
-    $CC -c -o conftest.o conftest.c > /dev/null 2>&1
-    if $LD @<(printf '%q\n' -shared -o conftest conftest.o) > /dev/null 2>&1 || $LD @<(printf '%q\n' -dylib -o conftest conftest.o) > /dev/null 2>&1
+    "$CC" -c -o conftest.o conftest.c > /dev/null 2>&1
+    if "$LD" @<(printf '%q\n' -shared -o conftest conftest.o) > /dev/null 2>&1 || "$LD" @<(printf '%q\n' -dylib -o conftest conftest.o) > /dev/null 2>&1
     then
         LdSupportsResponseFiles=YES
         AC_MSG_RESULT([yes])
--- a/m4/fp_ld_supports_response_files.m4
+++ b/m4/fp_ld_supports_response_files.m4
@@ -5,7 +5,8 @@
     AC_MSG_CHECKING([whether $LD supports response files])
     echo 'int main(void) {return 0;}' > conftest.c
     "$CC" -c -o conftest.o conftest.c > /dev/null 2>&1
-    if "$LD" @<(printf '%q\n' -shared -o conftest conftest.o) > /dev/null 2>&1 || "$LD" @<(printf '%q\n' -dylib -o conftest conftest.o) > /dev/null 2>&1
+    printf '%q\n' -shared -o conftest conftest.o > args.txt
+    if "$LD" -shared @args.txt > /dev/null 2>&1 || "$LD" -dylib @args.txt > /dev/null 2>&1
     then
         LdSupportsResponseFiles=YES
         AC_MSG_RESULT([yes])
--- a/m4/fp_ld_supports_response_files.m4
+++ b/m4/fp_ld_supports_response_files.m4
@@ -5,7 +5,7 @@
     AC_MSG_CHECKING([whether $LD supports response files])
     echo 'int main(void) {return 0;}' > conftest.c
     "$CC" -c -o conftest.o conftest.c > /dev/null 2>&1
-    printf '%q\n' -shared -o conftest conftest.o > args.txt
+    printf '%q\n' -o conftest conftest.o > args.txt
     if "$LD" -shared @args.txt > /dev/null 2>&1 || "$LD" -dylib @args.txt > /dev/null 2>&1
     then
         LdSupportsResponseFiles=YES
@@ -14,6 +14,6 @@
         LdSupportsResponseFiles=NO
         AC_MSG_RESULT([no])
     fi
-    rm -f conftest.c conftest
+    rm -f conftest.c conftest args.txt
     AC_SUBST(LdSupportsResponseFiles)
 ])
--- /dev/null
+++ b/m4/fp_ld_supports_response_files.m4
@@ -0,0 +1,19 @@
+# FP_LD_SUPPORTS_RESPONSE_FILES
+# --------------------
+# See if whether we are using a version of ld64 on darwin platforms which
+# supports response files.
+AC_DEFUN([FP_LD_SUPPORTS_RESPONSE_FILES], [
+    AC_MSG_CHECKING([whether $LD supports response files])
+    echo 'int main(void) {return 0;}' > conftest.c
+    $CC -c -o conftest.o conftest.c > /dev/null 2>&1
+    if $LD @<(printf '%q\n' -o conftest conftest.o) > /dev/null 2>&1
+    then
+        LdSupportsResponseFiles=YES
+        AC_MSG_RESULT([yes])
+    else
+        LdSupportsResponseFiles=NO
+        AC_MSG_RESULT([no])
+    fi
+    rm -f conftest.c conftest
+    AC_SUBST(LdSupportsResponseFiles)
+])

Additional Context

This reordering is meant to make fetched patches more robust to changes in the source URL, but in some cases it can break the patches and make them unusable. There is also no mechanism in nixpkgs for disabling this behavior. Introducing an option to disable this sorting would be very useful.

Related Issues

• fetchpatch silently strips binary sections #204320
• fetchpatch: ignores file renames #32084

[Artturin]

Try #226279