Re: [PATCH] gnu: libffi: Add unreleased patch to fix float128 on powerpc64le.

[Mark H Weaver <mhw@netris.org>]

Hi,

There's a problem with the following commit:

> commit 7eaa2f24ea77cddbb4bbc2d6a6905673a36f8f99
> Author: John Doe <dftxbs3e@free.fr>
> Date:   Tue Dec 15 10:23:44 2020 +0100
> 
>   gnu: libffi: Add unreleased patch to fix float128 on powerpc64le.
>   
>   Fixes <https://bugs.gnu.org/45252>.
>   
>   * gnu/packages/patches/libffi-float128-powerpc64le.patch: Import patch file
>   from <https://github.com/libffi/libffi/pull/561.patch>.
>   * gnu/packages/libffi.scm (libffi)[arguments]: Apply patch conditionally for
>   powerpc64le-* systems in a phase.
>   [inputs]: Add patch as input conditionally for powerpc64le-* systems.
>   * gnu/local.mk (dist_patch_DATA): Add patch file to build system.
>   
>   Signed-off-by: Chris Marusich <cmmarusich@gmail.com>

The problem is in how the 'patch' program is invoked, here:

> diff --git a/gnu/packages/libffi.scm b/gnu/packages/libffi.scm
> index d324892330..66239e0363 100644
> --- a/gnu/packages/libffi.scm
> +++ b/gnu/packages/libffi.scm
[...]
> @@ -67,13 +68,28 @@
>                                                        "powerpc-patch")))
>                                  (invoke "patch" "--batch" "-p1"
>                                          "-i" patch))))))
> +             '())
> +       ,@(if (string-prefix? "powerpc64le-" (or (%current-target-system)
> +                                                (%current-system)))
> +             '(#:phases (modify-phases %standard-phases
> +                          (add-after 'unpack 'apply-patch2
> +                            (lambda* (#:key inputs #:allow-other-keys)
> +                              (let ((patch (assoc-ref inputs
> +                                                      "powerpc64le-patch")))
> +                                (invoke "patch" "--batch" "-p1"
> +                                        "-i" patch))))))
>               '())))

When invoking 'patch' in Guix, you should *always* use "--force" instead
of "--batch".  There's a crucial difference between these two options:
If 'patch' finds that the given patch has already been applied, then
"--batch" will automatically *revert* the patch, whereas "--force" will
raise an error.  Here's the relevant section of the 'diffutils' manual:

> 10.11.2 Inhibiting Keyboard Input
> ---------------------------------
> 
> There are two ways you can prevent 'patch' from asking you any
> questions.  The '--force' ('-f') option assumes that you know what you
> are doing.  It causes 'patch' to do the following:
> 
>    * Skip patches that do not contain file names in their headers.
> 
>    * Patch files even though they have the wrong version for the
>      'Prereq:' line in the patch;
> 
>    * Assume that patches are not reversed even if they look like they
>      are.
> 
> The '--batch' ('-t') option is similar to '-f', in that it suppresses
> questions, but it makes somewhat different assumptions:
> 
>    * Skip patches that do not contain file names in their headers (the
>      same as '-f').
> 
>    * Skip patches for which the file has the wrong version for the
>      'Prereq:' line in the patch;
> 
>    * Assume that patches are reversed if they look like they are.

Now consider what will happen when we upgrade 'libffi' to a newer
version that already includes this fix.  If the Guix developer who
performs the upgrade forgets to remove this patch, the 'patch'
invocation above will start silently re-inserting the old bug.

We ran into this exact problem in the early years of Guix, and
henceforth changed all of the invocations of 'patch' to use '--force'.

Can we fix this right away, before many powerpc64le-* binaries are built
on top of it?

In any case, thanks very much for working on the powerpc64le port!

      Regards,
        Mark