[bug#56766] gnu: exiv2: Fix test failure on ppc64-le

  ppc64 and arm do not raise exception and thus output and exit code for test is different.

* gnu/packages/patches/exiv2-ppc64.patch: Modify test for ppc64
* gnu/packages/image.scm (exiv2): add `patches` field for source if target is ppc64

See:

  https://github.com/Exiv2/exiv2/issues/365 and
  https://github.com/Exiv2/exiv2/issues/933

upstream.
---
 gnu/packages/image.scm                 |  5 ++++-
 gnu/packages/patches/exiv2-ppc64.patch | 11 +++++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)
 create mode 100644 gnu/packages/patches/exiv2-ppc64.patch


base-commit: 212ca81895b2baa819ea11a308ad21880b84a546
  

On 25-07-2022 21:47, Marcel van der Boom wrote:
> +       (patches
> +        (if (target-ppc64le?)
> +            (list (search-patch "exiv2-ppc64.patch"))))))

The second branch of the 'if' is missing -- as-is, *unspecified* is used 
when (not (target-ppc64le?)), which won't work.

The 'patches' field is delayed, not thunked, so only the first 
system+target it sees will take effect. This will break things if for 
whatever reason you compute the derivation of the package for multiple 
systems in the same process.

To solve things, I recommend:

 1. Inform upstream that the test (or the code it tests) is broken on
    ppc64le, such that a better test can be devised and everyone (not
    only Guix) benefits,
 2. and for now, modify the test file in a phase (using 'substitute*')
    -- phases are thunked instead of delayed or direct, so the issue
    mentioned above doesn't hold.

Greetings,
Maxime.
  

On 25-07-2022 21:47, Marcel van der Boom wrote:
> new file mode 100644
> index 0000000000..a74a7ac1b7
> --- /dev/null
> +++ b/gnu/packages/patches/exiv2-ppc64.patch
> @@ -0,0 +1,11 @@
> +--- /tests/bugfixes/github/test_CVE_2018_12265.py
> ++++ /tests/bugfixes/github/test_CVE_2018_12265.py
> +@@ -18,7 +18,6 @@
> + Warning: Directory Image, entry 0x0201: Strip 0 is outside of the data area; ignored.
> + Warning: Directory Image, entry 0x0201: Strip 7 is outside of the data area; ignored.
> + Error: Offset of directory Thumbnail, entry 0x0201 is out of bounds: Offset = 0x00000000; truncating the entry


IIUC, "guix lint" has a linter that verifies that the patch contains a 
link to the upstream issue.  It is also required to add an entry to 
gnu/local.mk, such that it is added to release tarballs.

Greetings,
Maxime.
  

Thanks for the review, some notes/questions inline below.

[Maxime Devos]:

> The 'patches' field is delayed, not thunked, so only the first 
> system+target it sees will take effect. This will break things 
> if for whatever reason you compute the derivation of the package 
> for multiple systems in the same process.

Where can I read up on 'delayed' vs 'thunked' to understand that 
concept? I have no idea what it is at the moment and the manual 
does not mention this.

> To solve things, I recommend:

> 1. Inform upstream that the test (or the code it tests) is 
> broken on
>    ppc64le, such that a better test can be devised and everyone 
>    (not
>    only Guix) benefits,

This has been done. Their reply, in short: ppc64 is not on their 
supported platforms list and they delegate the fix to others.


marcel
  

On 26-07-2022 20:38, Marcel van der Boom wrote:
>> The 'patches' field is delayed, not thunked, so only the first 
>> system+target it sees will take effect. This will break things if for 
>> whatever reason you compute the derivation of the package for 
>> multiple systems in the same process.
>
> Where can I read up on 'delayed' vs 'thunked' to understand that 
> concept? I have no idea what it is at the moment and the manual does 
> not mention this. 

AFAICT, it is not documented, though you could read the code at (guix 
records). Basically:

* thunked = field value is wrapped in a (lambda () the-value).

   This allows for target-specific inputs, as (inputs (list (if 
It's-this-architecture these those))) is internally translated to

   (inputs (lambda () (if [...] [...] [...]))).

   That way, the inputs are not decided when the package is being 
defined, but when it is compiled to a particular architecture on a 
particular architecture (or more precisely, a little before building, in 
what is called 'lowering', which is a bit of a low-level concept and 
hence probably not well-known).

* delayed = field value is wrapped in a (delay the-value).

    For documentation on 'delay', see the manual. This is useful for 
avoiding computation until it's really needed, but unlike 'lambda', it 
will only be computed once, so only the first value of the-value is 
taken in account. As such, this won't work well when target-specific 
things are required.

Greetings,
Maxime.
  

>> 1. Inform upstream that the test (or the code it tests) is broken on
>>    ppc64le, such that a better test can be devised and everyone    (not
>>    only Guix) benefits,
>
> This has been done. Their reply, in short: ppc64 is not on their 
> supported platforms list and they delegate the fix to others. 
OK, in that case ...

On 25-07-2022 21:47, Marcel van der Boom wrote:
> +--- /tests/bugfixes/github/test_CVE_2018_12265.py
> ++++ /tests/bugfixes/github/test_CVE_2018_12265.py
> +@@ -18,7 +18,6 @@
> + Warning: Directory Image, entry 0x0201: Strip 0 is outside of the data area; ignored.
> + Warning: Directory Image, entry 0x0201: Strip 7 is outside of the data area; ignored.
> + Error: Offset of directory Thumbnail, entry 0x0201 is out of bounds: Offset = 0x00000000; truncating the entry
> +-$uncaught_exception $addition_overflow_message
> + """
> +     ]
> +-    retval = [1]
> ++    retval = [0]

... this is your proposed fix for powerpc64le, but how do we know 
whether it is correct? Is this just rewriting the test until it passes, 
hiding the underlying overflow bug which even had an CVE so probably 
pretty important to not hide it and actually fix it, or do we know for a 
fact that on ppc64le, a retval = [0] is correct?

Maybe this is answered by:

> ppc64 and arm do not raise exception and thus output and exit code for test is different.
but I don't know if that's working around symptoms or addressing the 
cause, e.g. 
https://github.com/Exiv2/exiv2/issues/933#issuecomment-863333032 noticed 
something on offsets -- summarised, this is not a sufficiently 
convincing explanation for me.

Also, somehow this version of the package builds on Debian sid, so maybe 
Debian knows more, though I'm not finding anything relevant in the 
Debian package myself.

Greetings,
Maxime.
  

[Maxime Devos]

> Also, somehow this version of the package builds on Debian sid, 
> so maybe Debian knows more, though I'm not finding anything 
> relevant in the Debian package myself.

true, and I quickly ran a debian:sid container to see what they 
did, but they chose the same solution. That is, the exiv2 binary 
from their package returns the error as well (without the 
exception raising).  So, I guess they dont run the test suite then 
as there's no change in their packaging.

It gave me enough confidence though to use it locally and try to 
package it up in guix the same way. But I agree it's rather 
unsatisfactory.


marcel
  

Anything else needed for this?