[bug#45252] gnu: libffi: Add unreleased patch to fix float128 on powerpc64le.

Hello!

Based on previous discussions to apply <
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=44778> on the master
branch instead I submit this new patch (attached) so that it does not
cause a world rebuild by not altering the package definition hash on
other platforms.

Thank you!

Hi,

dftxbs3e <dftxbs3e@free.fr> writes:

> Based on previous discussions to apply <
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=44778> on the master
> branch instead I submit this new patch (attached) so that it does not
> cause a world rebuild by not altering the package definition hash on
> other platforms.

My understanding is as follows:

- Patch 44778, linked above, was committed on core-updates branch in
  4fff5ab24126a152b50c036b9bf8dc6f2740f094.
- The libffi patch in this patch (45252) is the same as it was in 44778,
  but the scheme code has been changed so that we can apply this patch
  to the master branch without causing a rebuild of many packages.

Is that right?

> +             '())
> +       ,@(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))))))
>               '())))
>      (inputs
> -     (if (string-prefix? "powerpc-" (or (%current-target-system)
> +     (cond
> +      ((string-prefix? "powerpc-" (or (%current-target-system)
>                                          (%current-system)))
> -         `(("powerpc-patch" ,@(search-patches
> -                               "libffi-3.3-powerpc-fixes.patch")))
> -         '()))
> +       `(("powerpc-patch" ,@(search-patches
> +                             "libffi-3.3-powerpc-fixes.patch"))))
> +      ((string-prefix? "powerpc64le-" (or (%current-target-system)
> +                                          (%current-system)))
> +       `(("powerpc64le-patch" ,@(search-patches
> +                                 "libffi-float128-powerpc64le.patch"))))
> +      (else '())))

Looks good to me.  I'll test it locally and update here once I've
confirmed that it doesn't cause a full rebuild when applied to master.
Assuming all goes well, I intend to revert
4fff5ab24126a152b50c036b9bf8dc6f2740f094 on core-updates and apply this
patch to master.

> +++ b/gnu/packages/patches/libffi-float128-powerpc64le.patch

Based on...

https://patchwork.ozlabs.org/project/buildroot/patch/20191124090305.1015485-1-fontaine.fabrice@gmail.com/

...it sounds like upstream libffi maintainers may not have merged this
patch yet.  We should probably check with them to see when they plan to
merge it into upstream, but in the meantime there's no reason not to use
the patch if it works.  Based on what Fabrice said in that thread, it
sounds like the libffi maintainers may be a bit slow in responding to
power-related bugs.

On Sun, 2020-12-20 at 12:32 -0800, Chris Marusich wrote:
> Hi,
Hello!

> My understanding is as follows:
> 
> - Patch 44778, linked above, was committed on core-updates branch in
>   4fff5ab24126a152b50c036b9bf8dc6f2740f094.
> - The libffi patch in this patch (45252) is the same as it was in
> 44778,
>   but the scheme code has been changed so that we can apply this
> patch
>   to the master branch without causing a rebuild of many packages.
> 
> Is that right?

Exactly!

> Based on...
> 
> https://patchwork.ozlabs.org/project/buildroot/patch/20191124090305.1015485-1-fontaine.fabrice@gmail.com/
> 
> ...it sounds like upstream libffi maintainers may not have merged
> this
> patch yet.  We should probably check with them to see when they plan
> to
> merge it into upstream, but in the meantime there's no reason not to
> use
> the patch if it works.  Based on what Fabrice said in that thread, it
> sounds like the libffi maintainers may be a bit slow in responding to
> power-related bugs.
> 

That's not true, they've merged the patch: <
https://github.com/libffi/libffi/pull/561> - they just did not make a
release yet.

Leo

Hi,

dftxbs3e <dftxbs3e@free.fr> writes:

>> Based on...
>> 
>> https://patchwork.ozlabs.org/project/buildroot/patch/20191124090305.1015485-1-fontaine.fabrice@gmail.com/
>> 
>> ...it sounds like upstream libffi maintainers may not have merged
>> this
>> patch yet.  We should probably check with them to see when they plan
>> to
>> merge it into upstream, but in the meantime there's no reason not to
>> use
>> the patch if it works.  Based on what Fabrice said in that thread, it
>> sounds like the libffi maintainers may be a bit slow in responding to
>> power-related bugs.
>> 
>
> That's not true, they've merged the patch: <
> https://github.com/libffi/libffi/pull/561> - they just did not make a
> release yet.

Ah, OK.  That's good to know!

I've committed this patch in 7eaa2f24ea77cddbb4bbc2d6a6905673a36f8f99 on
master.  I've also reverted 4fff5ab24126a152b50c036b9bf8dc6f2740f094 in
commit b50341dba9811c048bed852c0279b828c7ddba66 on core-updates.  I
think we should be good to go to try building powerpc64-linux-gnu
bootstrap-tarballs now!

I confirmed that this commit on master would not cause dependents of
libffi to be rebuilt by verifying that the derivation for one of its
dependent packages, guile, did not change before/after the change.

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

Earlier, I wrote:
> When invoking 'patch' in Guix, you should *always* use "--force" instead
> of "--batch".

(See <https://bugs.gnu.org/45252#19> for my earlier message).

Since writing the message above, I've found another problem in the same
commit (7eaa2f24ea77cddbb4bbc2d6a6905673a36f8f99): it searches for the
'patch' program in 'inputs'.  This is a mistake, because when
cross-compiling, 'inputs' will contain software compiled to run on the
target system instead of the build system.

If 'native-inputs' is not #f, we should search for the 'patch' program
in 'native-inputs' instead of 'inputs'.  Unless there's now a better way
that I don't know, I suggest adding 'native-inputs' to the list of
keyword arguments accepted by the 'lambda*', and changing 'inputs' to
"(or native-inputs inputs)" in the call to 'assoc-ref'.  Something like
this (untested):

_ ,@(if (string-prefix? "powerpc64le-" (or (%current-target-system)
__________________________________________ (%current-system)))
_______ '(#:phases (modify-phases %standard-phases
____________________ (add-after 'unpack 'apply-patch2
______________________ (lambda* (#:key inputs native-inputs
________________________________ #:allow-other-keys)
________________________ (let ((patch (assoc-ref (or native-inputs inputs)
________________________________________________ "powerpc64le-patch")))
__________________________ (invoke "patch" "--force" "-p1"
__________________________________ "-i" patch))))))
_______ '())

I see now that both of these mistakes were already present in the
"powerpc-*" case immediately above the recently-added "powerpc64le-*"
case.  The "powerpc-*" case was added earlier this year in the following
commit:

  https://git.sv.gnu.org/cgit/guix.git/commit/?id=02f5ee01c96589fc13f1e21b85b0b48100aec4e8

I'm CC'ing 'guix-devel' to raise awareness about these common mistakes,
in the hopes that reviewers will be more likely to notice them in the
future.

      Thanks,
        Mark