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

DoneSubmitted by dftxbs3e.
Details
4 participants
  • Chris Marusich
  • dftxbs3e
  • Efraim Flashner
  • Mark H Weaver
Owner
unassigned
Severity
normal
D
D
dftxbs3e wrote on 15 Dec 2020 10:32
(address . guix-patches@gnu.org)
8fc771beb3f0f1e2886a238e0ef9087908c98fc1.camel@free.fr
Hello!

Based on previous discussions to apply <
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!
From 05c3dc588745240fb790f9a39111be70c153d70c Mon Sep 17 00:00:00 2001
From: John Doe <dftxbs3e@free.fr>
Date: Tue, 15 Dec 2020 10:24:11 +0100
Subject: [PATCH] gnu: libffi: Add unreleased patch to fix float128 on
powerpc64le.

* gnu/packages/patches/libffi-float128-powerpc64le.patch: Import 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.
---
gnu/local.mk | 1 +
gnu/packages/libffi.scm | 25 ++++++--
.../patches/libffi-float128-powerpc64le.patch | 58 +++++++++++++++++++
3 files changed, 79 insertions(+), 5 deletions(-)
create mode 100644 gnu/packages/patches/libffi-float128-powerpc64le.patch

Toggle diff (124 lines)
diff --git a/gnu/local.mk b/gnu/local.mk
index 0b4cf23838..e190df0667 100644
--- a/gnu/local.mk
+++ b/gnu/local.mk
@@ -1208,6 +1208,7 @@ dist_patch_DATA =						\
   %D%/packages/patches/julia-SOURCE_DATE_EPOCH-mtime.patch	\
   %D%/packages/patches/kdbusaddons-kinit-file-name.patch	\
   %D%/packages/patches/libffi-3.3-powerpc-fixes.patch		\
+  %D%/packages/patches/libffi-float128-powerpc64le.patch	\
   %D%/packages/patches/libvirt-create-machine-cgroup.patch	\
   %D%/packages/patches/libziparchive-add-includes.patch		\
   %D%/packages/patches/localed-xorg-keyboard.patch		\
diff --git a/gnu/packages/libffi.scm b/gnu/packages/libffi.scm
index d324892330..e4bfe6731b 100644
--- a/gnu/packages/libffi.scm
+++ b/gnu/packages/libffi.scm
@@ -57,7 +57,7 @@
        ;; compiler.  See "ax_cc_maxopt.m4" and "ax_gcc_archflag.m4".
        #:configure-flags '("--enable-portable-binary" "--without-gcc-arch")
 
-       ;; TODO: Inline patch on next rebuild cycle.
+       ;; TODO: Inline patches on next rebuild cycle.
        ,@(if (string-prefix? "powerpc-" (or (%current-target-system)
                                             (%current-system)))
              '(#:phases (modify-phases %standard-phases
@@ -67,13 +67,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))))))
              '())))
     (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 '())))
     (outputs '("out" "debug"))
     (synopsis "Foreign function call interface library")
     (description
diff --git a/gnu/packages/patches/libffi-float128-powerpc64le.patch b/gnu/packages/patches/libffi-float128-powerpc64le.patch
new file mode 100644
index 0000000000..4fd32b0102
--- /dev/null
+++ b/gnu/packages/patches/libffi-float128-powerpc64le.patch
@@ -0,0 +1,58 @@
+From de93adfb6f48100946bba2c3abad2a77a0cfde0b Mon Sep 17 00:00:00 2001
+From: Fabrice Fontaine <fontaine.fabrice@gmail.com>
+Date: Sun, 24 Nov 2019 09:52:01 +0100
+Subject: [PATCH] ffi_powerpc.h: fix build failure with powerpc7
+
+This is a patch pulled down from the following:
+https://github.com/buildroot/buildroot/blob/78926f610b1411b03464152472fd430012deb9ac/package/libffi/0004-ffi_powerpc.h-fix-build-failure-with-powerpc7.patch
+
+This issue is being hit on OpenBMC code when pulling the latest
+libffi tag and building on a P8 ppc64le machine. I verified this
+patch fixes the issue we are seeing.
+
+Below is the original commit message:
+
+Sicne commit 73dd43afc8a447ba98ea02e9aad4c6898dc77fb0, build on powerpc7
+fails on:
+
+In file included from ../src/powerpc/ffi.c:33:0:
+../src/powerpc/ffi_powerpc.h:61:9: error: '_Float128' is not supported on this target
+ typedef _Float128 float128;
+         ^~~~~~~~~
+
+Fix this build failure by checking for __HAVE_FLOAT128 before using
+_Float128, as _Float128 is enabled only on specific conditions, see
+output/host/powerpc64-buildroot-linux-gnu/sysroot/usr/include/bits/floatn.h:
+
+ /* Defined to 1 if the current compiler invocation provides a
+    floating-point type with the IEEE 754 binary128 format, and this glibc
+    includes corresponding *f128 interfaces for it.  */
+ #if defined _ARCH_PWR8 && defined __LITTLE_ENDIAN__ && (_CALL_ELF == 2) \
+     && defined __FLOAT128__ && !defined __NO_LONG_DOUBLE_MATH
+ # define __HAVE_FLOAT128 1
+ #else
+ # define __HAVE_FLOAT128 0
+ #endif
+
+Fixes:
+ - http://autobuild.buildroot.org/results/5c9dd8fb3b6a128882b6250f197c80232d8a3b53
+
+Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>
+Signed-off-by: Andrew Geissler <geissonator@yahoo.com>
+---
+ src/powerpc/ffi_powerpc.h | 2 +-
+ 1 file changed, 1 insertion(+), 1 deletion(-)
+
+diff --git a/src/powerpc/ffi_powerpc.h b/src/powerpc/ffi_powerpc.h
+index 8e2f2f0e..960a5c42 100644
+--- a/src/powerpc/ffi_powerpc.h
++++ b/src/powerpc/ffi_powerpc.h
+@@ -57,7 +57,7 @@ typedef union
+   double d;
+ } ffi_dblfl;
+ 
+-#if defined(__FLOAT128_TYPE__)
++#if defined(__FLOAT128_TYPE__) && defined(__HAVE_FLOAT128)
+ typedef _Float128 float128;
+ #elif defined(__FLOAT128__)
+ typedef __float128 float128;
-- 
2.29.2
C
C
Chris Marusich wrote on 20 Dec 2020 21:32
(name . dftxbs3e)(address . dftxbs3e@free.fr)(address . 45252@debbugs.gnu.org)
87zh289q1h.fsf@gmail.com
Hi,

dftxbs3e <dftxbs3e@free.fr> writes:

Toggle quote (6 lines)
> 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?

Toggle quote (27 lines)
> + '())
> + ,@(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.

Toggle quote (2 lines)
> +++ b/gnu/packages/patches/libffi-float128-powerpc64le.patch

Based on...


...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.

--
Chris
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEEy/WXVcvn5+/vGD+x3UCaFdgiRp0FAl/ftOsACgkQ3UCaFdgi
Rp2WxxAAg2V8isAwAIRoASg5C6Gu3d2P/isgegSNDosAt1OBIpiW6lVNvjfG1HY0
U6M7A142y7K4Gsc+68xnjWJ4kdepws7xQVX8S7Ae6Zba5KXma287gacrd9fLr8GS
2xd26wUxLjkz6vcjnaPsxjA38dTrxXjuc1Xi2icnlbkotLO4RyaBa9S2UW/na0Qa
txDrEXIz0U+dKm4kFI09cETKIX6bciyO15V/DslFaEdEe3cWGefVCp86bCzRpYqV
piyfxftADaX/IZ9ysG2LdcJ6bfyhpD/71tx6Fpgm73Rq+ldwv6DsCFQrKsT7JBOg
6+L8ujO/3ccTMKDUuQ48S/aDdft+7it/LU6qhcZk5Q4JdY9GZhrEeC/JA12RM/mP
pbrFcCC+255IB4L3tNWlKQoUYLxtam8YLVzSX2DZ45T+eaicEKeuZlqIYXFxFEMw
3TULzBhSk5dNv9FjdBc1MUrfhGeQmO9z1t9J9STh8vO/g7ZuxhQFGFemYbRLpDMv
DOLDKnVEeYl7P6aKujkKpsQvx3uIcSI6Plf3Jg+5o+Vaeliu6s0EXaNGFoXlmduY
+HyOEsNo4i2d8cohjEM4jp2eCr5wK/BHDyCO9seX0dcm9Rnm4A5V9wAtY1phb0TO
3Q524lkPXUBGrJia0lf2tX7O2nCepklCQ4v5X6qLi5cB3y+xU1c=
=TQRQ
-----END PGP SIGNATURE-----

D
D
dftxbs3e wrote on 20 Dec 2020 21:36
(name . Chris Marusich)(address . cmmarusich@gmail.com)(address . 45252@debbugs.gnu.org)
d82d4406f67b632219508ddadeba9651fa3243cb.camel@free.fr
On Sun, 2020-12-20 at 12:32 -0800, Chris Marusich wrote:
Toggle quote (1 lines)
> Hi,
Hello!

Toggle quote (12 lines)
> 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!

Toggle quote (15 lines)
> 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: <
release yet.

Leo
-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQQozTys6hLca3Ekt8u3HhLxn6GoewUCX9+15gAKCRC3HhLxn6Go
ex44AQCYB7ssqTwoHLFvnxsmAqUx5TvZXUhK3I5ChsduOWB+/AEAugnI1B28dK8y
Zw6xYTcw7Uaft0Kb+II2uoDUeXFdsQA=
=jCvy
-----END PGP SIGNATURE-----


C
C
Chris Marusich wrote on 21 Dec 2020 02:30
(name . dftxbs3e)(address . dftxbs3e@free.fr)(address . 45252-close@debbugs.gnu.org)
87k0tct07s.fsf@gmail.com
Hi,

dftxbs3e <dftxbs3e@free.fr> writes:

Toggle quote (19 lines)
>> 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.

--
Chris
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEEy/WXVcvn5+/vGD+x3UCaFdgiRp0FAl/f+qcACgkQ3UCaFdgi
Rp3inRAAi07WW3D31YInYIyU5bv8EfHC4lVKBj1Tk7aoT4jqlzDWLXQg8NuHyOK9
b7GeyYFhTtHRm0PzCFUZsxT0rLRBPk3rO/nf+1i/toL3TU69aKTqQjwjMpH1PzTR
Ieilx55BIQM0g/8CDC4+kvfZwpn5m4m3+yU5cZxozoHmjD9uaVfjwc3rSOxesLDQ
Ju0JlVh0JRCSjUFXOs2P59axwlkdlTvRNnQDIW1L3+Wjo10NeqR0Dx6ghRw+NSsr
EbLykCltn/FUQcZ9IRXnQtfM+BiVae04GkViZi0fwkl2p+vYDWgDwG7d6h3WDJxT
n6ykRakKRviwryxpdXqY8N0fYbUduqjrJ/zpiBDDfpULSCgFS/XZosQkcKkCLk7W
fXPQ7vDl5IV+LL8N+duryEMo2zqDZrxyJd6mEdZ0QP69zbtM8IdsCZc28V8F5k8+
2jyQoSmybARGQmXADEkMTL00fLaH0Gfcn9dzIBJCQ/SYhCgG1cnm1k5dfC79QRUt
R8HSlkY+BO2+MHhiK57mTC7sf3TXpIHXy9gee093lwxMEEv6mD/jze07EQwIV7Mg
tmCdstPMdMw1ahSnIXIGNHXwXB+Ln7LLVGtQgV+FFVl5a1W7a4IqeNclnhJWq1d2
cFl+226faLlzGQXsyqBRxqK6qOaQmGb2hN+cGpGd01wxyIuoDsM=
=6p4X
-----END PGP SIGNATURE-----

M
M
Mark H Weaver wrote on 22 Dec 2020 06:15
Re: [PATCH] gnu: libffi: Add unreleased patch to fix float128 on powerpc64le.
87y2hqifp3.fsf@netris.org
Hi,

There's a problem with the following commit:

Toggle quote (17 lines)
> 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:

Toggle quote (4 lines)
> 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
[...]
Toggle quote (16 lines)
> @@ -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:

Toggle quote (26 lines)
> 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
M
M
Mark H Weaver wrote on 22 Dec 2020 07:00
87tuseidlk.fsf@netris.org
Earlier, I wrote:
Toggle quote (3 lines)
> 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:


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
C
C
Chris Marusich wrote on 23 Dec 2020 07:38
(name . Mark H Weaver)(address . mhw@netris.org)
87r1nhdo2i.fsf@gmail.com
Hi Mark,

Mark H Weaver <mhw@netris.org> writes:

Toggle quote (6 lines)
> 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).

Thank you for letting me know about this. I didn't know about the
difference between "--batch" and "--force". I agree we should use
"--force" instead of "--batch". How do you recommend that I proceed?

I can definitely make another commit on the master branch to change the
option from "--batch" to "--force". However, I'm reluctant to change
the option in the existing code on the master branch (introduced in
commit 02f5ee01c96589fc13f1e21b85b0b48100aec4e8), since I'm not sure how
many packages would be rebuilt as a result. The powerpc64le
architecture is not bootstrapped at all yet, so it's not a problem for
that architecture, but I don't know the status for all the other
architectures beginning with "powerpc".

Before I make a new commit to change the patch option on the master
branch, I'd appreciate your advice on how to proceed. Do you think it
would be better to make a commit on the master branch to fix just the
option I introduced in commit 7eaa2f24ea77cddbb4bbc2d6a6905673a36f8f99?
Or, do you think it would be better to make a commit on the core-updates
branch to change all the "--batch" options to "--force" options in the
libffi package definition? If we did it on core-updates, we could just
replace the manual invocation of the "patch" tool with a change that
looks more like 4fff5ab24126a152b50c036b9bf8dc6f2740f094, in particular
this part:

Toggle diff (69 lines)
diff --git a/gnu/packages/libffi.scm b/gnu/packages/libffi.scm
index 0e6a31d78c..0db8fa3e82 100644
--- a/gnu/packages/libffi.scm
+++ b/gnu/packages/libffi.scm
@@ -51,7 +51,8 @@
               (sha256
                (base32
                 "0mi0cpf8aa40ljjmzxb7im6dbj45bb0kllcd09xgmp834y9agyvj"))
-              (patches (search-patches "libffi-3.3-powerpc-fixes.patch"))))
+              (patches (search-patches "libffi-3.3-powerpc-fixes.patch"
+                                       "libffi-float128-powerpc64le.patch"))))
     (build-system gnu-build-system)
     (arguments
      `(;; Prevent the build system from passing -march and -mtune to the

I thought we wanted to apply patch 45252 on the master branch.  That's
why I made commit 7eaa2f24ea77cddbb4bbc2d6a6905673a36f8f99.  Afterwords,
I actually reverted commit 4fff5ab24126a152b50c036b9bf8dc6f2740f094 on
the core-updates branch in commit
b50341dba9811c048bed852c0279b828c7ddba66.  I reverted it because I
thought it would be undesirable to solve the same problem in two
different ways on two separate branches, and I thought that reverting it
would reduce the risk of merge conflicts later.

However, now that I think about it, I'm not sure the reversion was
necessary.  I'm actually not sure what the normal procedure is for
merging to/from core-updates and master (I've done many merges in my own
projects, but I've never done a merge in the Guix project), so I'm not
sure how a "TODO" task like the one mentioned in commit
7eaa2f24ea77cddbb4bbc2d6a6905673a36f8f99 ("Inline patches on next
rebuild cycle") would normally be resolved.  I would welcome any advice
you have about that.

By the way, a wip-ppc64le branch also exists, but I don't know what its
status is or whether I'm allowed to touch it.  I just assumed things
would be simpler if we applied patches to master branch when possible.

> 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.

Is it searching for the "patch" program, or is it searching for the
patch file?  It looks to me like the code is searching for the patch
file in inputs, not the "patch" program.  The relevant code is here:

       ,@(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))))))

The code invokes the "patch" program in the usual way.  My understanding
is that whatever version of the "patch" program that Guix has placed in
the PATH environment variable will be used.  Therefore, Guix will use
the correct "patch" program, regardless of whether or not the package is
being cross-compiled.  Am I misunderstanding something?

Again, thank you for taking the time to bring these topics up.  I'm
always trying to make sure I do things the best way I can in Guix, so I
appreciate the feedback.

-- 
Chris
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEEy/WXVcvn5+/vGD+x3UCaFdgiRp0FAl/i5eUACgkQ3UCaFdgi
Rp1v4RAAracW0Kd8BJu2tOtBvKTzULu1xxZa2pG3hnABpvnLpAbWFRT5ZIF3dlv1
68QAFyK0WtzKXlSdOFa2b46COLPFh12yCm4gA7NVJuzqOIu8iF5XGoqu5C7r/03h
k7W1d4cEgTRkPagjKOH8MX2DpBF9hlGm3P0jOXwyV3DZGH9ZcRDxXznJpcsRXV4g
9nYHOV3Lpv6PvFzhf4r6L0H8FCKF34lBsYPeePF8jrKgyGNzpAEz1mE0Ev5cLCt5
L8gALNNR889gaQ4W3yHLYZUFIOe/8DsZrBVJlnbWy8Kf+pW939pr7Woer32pBfTn
+yO0Q3rCNWALWSDItxAr9YenyhEpH81bpnEY9GsrfXS5zVRK9UVv8bfZHameGjq5
FgE0LRkXkMSfRDhF8hv1TfFtXaWsmrTOnkbTx9i7KwVo0iLFvJ0XkRqUW3SSR9/g
LCz46z7vIhlkvsR/dcUca1MDUgy80Xn85yeRbut5cU8iZ6Wj0bxbEl8hCp+X8ppL
cOwbkUqKuoUEkmXj/e3VFUZNy1ILGSPLNQA+ET+xlM6bWwfKv/lENdTiOoSwF1wN
DqDn0kseTkyinhRMzrU1atrYQMi6BNrn2jDHaPKEiQOlyaVfSzLv6XjOfLB6uQu1
cr9L+bboP4aR1nlgZEWbdduHMhRbJtHV+Ja+dAbfUQvuo8itE4k=
=CGAf
-----END PGP SIGNATURE-----

E
E
Efraim Flashner wrote on 23 Dec 2020 08:50
(name . Chris Marusich)(address . cmmarusich@gmail.com)
X+L2w13wSyFv9UC2@E5400
"regular powerpc", ie macppc/ppc32/powerpc-linux-gnu, does have some
bootstrap binaries built but isn't near ready for merging. Go ahead and
make any changes necessary.


--
Efraim Flashner <efraim@flashner.co.il> אפרים פלשנר
GPG key = A28B F40C 3E55 1372 662D 14F7 41AA E7DC CA3D 8351
Confidentiality cannot be guaranteed on emails sent or received unencrypted
-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEoov0DD5VE3JmLRT3Qarn3Mo9g1EFAl/i9r8ACgkQQarn3Mo9
g1G2sg/8Dk1VZVFY+GWETbGGMZ4d6Usjc+qLsS+XgQfDsvVqAlhl7gnoMlZJL/+l
aZAykOdrJm1O2qyFMgn4K77TSDFAFhAalJJUcHwOXUnDa6/yIkZXKEoT4XMwk4MO
2rG/A1eDZ6FldO2PCpsCvglzoJow5JlFcSSuO7Y9Hl8nZqjGBV00s/2nR/Riv5Gc
wetweXLsNIP1a3x52SBwUPgU+Zrgw5BuI0ar0DincmbAgbcOhY2xQA0kIWCfkSI4
6gQmV6DhhB2FQWUxWS9zN67JkCP2MklPtKaJL6vr8sw3K1WUtd6aUcuzBuWN2AZz
kDNV3PkD7ZrVN3QY9M29zHCtAkUjPx76WJUWyPimvhnePyC8Vxfz/CXMTl97DG+n
Vdifr4uNNf4X+uRWhSDilpP4p7jOy8KQ5IKD3XFhGoB/zQFqrXdQq0iox2vaAGor
vXpU+FEWLegbCYVh3DJ7JLaq1/pt1ayphnnypq2npWVlJOLwvFfYmaiJF6yin542
KTs1VsC6ee1x2f2Lg6FkLrxmDjXsCWwJHmWXVwEBLwlWvjMAQG9xKJSyoj8bWamO
8rnrN/rNw2reMbJlhjBIMn8k+RlUaKkXdomOjLcBJWuxw6DvZTYCT+U5IXrDV7I3
kiSLL1KabNyyAjdm/Jkbn9aL7IZOukmcihoP5NcMdapLzxQulg8=
=wdD7
-----END PGP SIGNATURE-----


M
M
Mark H Weaver wrote on 23 Dec 2020 22:22
(name . Chris Marusich)(address . cmmarusich@gmail.com)
87r1ngi5ef.fsf@netris.org
Hi Chris,

Chris Marusich <cmmarusich@gmail.com> writes:

Toggle quote (12 lines)
> Mark H Weaver <mhw@netris.org> writes:
>
>> 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).
>
> Thank you for letting me know about this. I didn't know about the
> difference between "--batch" and "--force". I agree we should use
> "--force" instead of "--batch". How do you recommend that I proceed?

Simply changing "--batch" to "--force" on line 79 (in the powerpc64le-*
case, i.e. the one that was just added) seems like the right thing.
That will force a rebuild of almost everything on the powerpc64le-*
architecture, but should not cause any rebuilds on other architectures.

Toggle quote (10 lines)
>> 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.
>
> Is it searching for the "patch" program, or is it searching for the
> patch file? It looks to me like the code is searching for the patch
> file in inputs, not the "patch" program.

LOL, you're right, I got confused. Please disregard my second email in
this thread, and apologies for that noise.

Toggle quote (4 lines)
> Again, thank you for taking the time to bring these topics up. I'm
> always trying to make sure I do things the best way I can in Guix, so I
> appreciate the feedback.

Thank you, Chris.

Warm regards,
Mark
M
M
Mark H Weaver wrote on 23 Dec 2020 22:34
87o8iki4u7.fsf@netris.org
Hi Efraim,

Efraim Flashner <efraim@flashner.co.il> writes:
Toggle quote (4 lines)
> "regular powerpc", ie macppc/ppc32/powerpc-linux-gnu, does have some
> bootstrap binaries built but isn't near ready for merging. Go ahead and
> make any changes necessary.

I appreciate that, but if rebuilding the world on regular powerpc would
significantly add to the burden of even a single developer, then it's
probably not worth it. I suggested fixing the powerpc64le case now only
because it was just added a few days ago, and more generally to raise
awareness about how best to run the 'patch' program in Guix.

If it's truly no extra burden, then you could change "--batch" to
"--force" on line 69 of libffi.c (in the "powerpc-*" case).

Regards,
Mark
C
C
Chris Marusich wrote on 28 Dec 2020 05:17
Re: [bug#45252] [PATCH] gnu: libffi: Add unreleased patch to fix float128 on powerpc64le., [bug#45252] [PATCH] gnu: libffi: Add unreleased patch to fix float128 on powerpc64le., [bug#45252] [PATCH] gnu: libffi: Add unreleased patch to fix float128 on powerpc64le.
871rfa36oy.fsf@gmail.com
Hi,

Efraim Flashner <efraim@flashner.co.il> writes:

Toggle quote (4 lines)
> "regular powerpc", ie macppc/ppc32/powerpc-linux-gnu, does have some
> bootstrap binaries built but isn't near ready for merging. Go ahead and
> make any changes necessary.

Mark H Weaver <mhw@netris.org> writes:

Toggle quote (16 lines)
> Hi Efraim,
>
> Efraim Flashner <efraim@flashner.co.il> writes:
>> "regular powerpc", ie macppc/ppc32/powerpc-linux-gnu, does have some
>> bootstrap binaries built but isn't near ready for merging. Go ahead and
>> make any changes necessary.
>
> I appreciate that, but if rebuilding the world on regular powerpc would
> significantly add to the burden of even a single developer, then it's
> probably not worth it. I suggested fixing the powerpc64le case now only
> because it was just added a few days ago, and more generally to raise
> awareness about how best to run the 'patch' program in Guix.
>
> If it's truly no extra burden, then you could change "--batch" to
> "--force" on line 69 of libffi.c (in the "powerpc-*" case).

OK. I've made this change on master in commit
662e7e28d576ada91fc9dec7d27c100666114f03.

Mark H Weaver <mhw@netris.org> writes:

Toggle quote (21 lines)
> Hi Chris,
>
> Chris Marusich <cmmarusich@gmail.com> writes:
>
>> Mark H Weaver <mhw@netris.org> writes:
>>
>>> 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).
>>
>> Thank you for letting me know about this. I didn't know about the
>> difference between "--batch" and "--force". I agree we should use
>> "--force" instead of "--batch". How do you recommend that I proceed?
>
> Simply changing "--batch" to "--force" on line 79 (in the powerpc64le-*
> case, i.e. the one that was just added) seems like the right thing.
> That will force a rebuild of almost everything on the powerpc64le-*
> architecture, but should not cause any rebuilds on other architectures.

OK, I've made this change on master in commit
fdb90e9ee8a578c88ef3a33067e8a532e43ae7b8.

Toggle quote (13 lines)
>>> 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.
>>
>> Is it searching for the "patch" program, or is it searching for the
>> patch file? It looks to me like the code is searching for the patch
>> file in inputs, not the "patch" program.
>
> LOL, you're right, I got confused. Please disregard my second email in
> this thread, and apologies for that noise.

No worries! Thanks again for your help.

--
Chris
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEEy/WXVcvn5+/vGD+x3UCaFdgiRp0FAl/pXF0ACgkQ3UCaFdgi
Rp1ioQ//W6XnPvkS0isAW+iOAObLeWcNmjN91bKJFKvmNPIxEb2tyVOda/Lv5dJp
NFFOziVorczwIORpRJqPSdXIhOfq+bU6ZN9RfY8d20f3Zs7AH7N/rvH7PQUx+N9L
GcDHMsDV+fTycgnU9bwPo6q5GGfd9TQy5FGABa5bTsRjCabb/W4iWVW2Vbh3ZVIn
PjVmYjq27lLwWTBhyix7GA37sGkIHfm8C4NOFuGaiId9sx5UYjO8nu9vtES53Wq6
frCZ+UHjP2zMfaDA68Q0VonaHA3uqTGpxqdA0EzBJukleDsSl5Tzjnk5MPDGPaHx
m/pCoD32IEjOM0q1EWz2EMoop6hackMhtbFVcy7tM06ili4A/XX7I0b2bpZhsbCj
8gbDvNyZqr9w01K1HVyAsepaXzNfV8cNOxAyoQKcFGxUvV+F+EOZPv1eKWEh6ZFd
p+h1iJnsjY/xvrCwwh0rTgkw/P4k0o32dROMhF/lqS79pWxFLkXId+kNwwum84Cg
1vhnjpSRIBx5W6c6+xavQovAaSNC9TpuirKeszeG5y6mUpxjAC31oGAI+6NYlJb5
Vw/gQKvVylIS6ma0KkcHlIzcSqqcNX8DlczsV8w5lW+RuzTQ8D5PblFH1zngEH0Y
Dgcb/C3BnR1uPUbO4S8IpAv1r2YfaYBlA2146iGn7RGQIHuINt0=
=bxVh
-----END PGP SIGNATURE-----

?
Your comment

This issue is archived.

To comment on this conversation send email to 45252@debbugs.gnu.org