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

Hi Mark,

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?

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:


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.

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

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.

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

> 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

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

      Regards,
        Mark

Hi,

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.

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

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

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

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