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

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

[Marcel van der Boom]

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

diff --git a/gnu/packages/image.scm b/gnu/packages/image.scm
index d52d57b3b1..dc4bf76790 100644
--- a/gnu/packages/image.scm
+++ b/gnu/packages/image.scm
@@ -1342,7 +1342,10 @@ (define-public exiv2
        (uri (string-append "https://www.exiv2.org/builds/exiv2-" version
                            "-Source.tar.gz"))
        (sha256
-        (base32 "1qm6bvj28l42km009nc60gffn1qhngc0m2wjlhf90si3mcc8d99m"))))
+        (base32 "1qm6bvj28l42km009nc60gffn1qhngc0m2wjlhf90si3mcc8d99m"))
+       (patches
+        (if (target-ppc64le?)
+            (list (search-patch "exiv2-ppc64.patch"))))))
     (build-system cmake-build-system)
     (arguments
      '(#:test-target "tests"
diff --git a/gnu/packages/patches/exiv2-ppc64.patch b/gnu/packages/patches/exiv2-ppc64.patch
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
+-$uncaught_exception $addition_overflow_message
+ """
+     ]
+-    retval = [1]
++    retval = [0]

base-commit: 212ca81895b2baa819ea11a308ad21880b84a546
-- 
2.37.1

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

[Maxime Devos]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 940 bytes --]


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.


[-- Attachment #1.1.1.2: Type: text/html, Size: 1465 bytes --]

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

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

[Marcel van der Boom]

[-- Attachment #1: Type: text/plain, Size: 825 bytes --]


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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

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

[Maxime Devos]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 1537 bytes --]


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.


[-- Attachment #1.1.1.2: Type: text/html, Size: 2108 bytes --]

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

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

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 851 bytes --]


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.


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

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

[Maxime Devos]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 1855 bytes --]

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


[-- Attachment #1.1.1.2: Type: text/html, Size: 2810 bytes --]

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

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

[Marcel van der Boom]

[-- Attachment #1: Type: text/plain, Size: 641 bytes --]


[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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

[bug#56766] [PATCH] Adjust patch according to review

[Marcel van der Boom]

* gnu/packages/image.scm (exiv3): use substitution instead of patch
---
 gnu/packages/image.scm                 | 18 ++++++++++++------
 gnu/packages/patches/exiv2-ppc64.patch | 11 -----------
 2 files changed, 12 insertions(+), 17 deletions(-)
 delete mode 100644 gnu/packages/patches/exiv2-ppc64.patch

diff --git a/gnu/packages/image.scm b/gnu/packages/image.scm
index dc4bf76790..9febb883b4 100644
--- a/gnu/packages/image.scm
+++ b/gnu/packages/image.scm
@@ -1342,13 +1342,10 @@ (define-public exiv2
        (uri (string-append "https://www.exiv2.org/builds/exiv2-" version
                            "-Source.tar.gz"))
        (sha256
-        (base32 "1qm6bvj28l42km009nc60gffn1qhngc0m2wjlhf90si3mcc8d99m"))
-       (patches
-        (if (target-ppc64le?)
-            (list (search-patch "exiv2-ppc64.patch"))))))
+        (base32 "1qm6bvj28l42km009nc60gffn1qhngc0m2wjlhf90si3mcc8d99m"))))
     (build-system cmake-build-system)
     (arguments
-     '(#:test-target "tests"
+     `(#:test-target "tests"
        #:configure-flags (list "-DEXIV2_BUILD_UNIT_TESTS=ON"
                                ;; darktable needs BMFF to support
                                ;; CR3 files.
@@ -1359,7 +1356,16 @@ (define-public exiv2
            (lambda* (#:key outputs #:allow-other-keys)
              (let* ((out (assoc-ref outputs "out"))
                     (lib (string-append out "/lib")))
-               (for-each delete-file (find-files lib "\\.a$"))))))))
+               (for-each delete-file (find-files lib "\\.a$")))))
+         (add-after 'unpack 'adjust-ppc6-tests
+           (lambda _
+             ,@(if (target-ppc64le?)
+                   ;; Adjust test on ppc64
+                   ;; See: https://github.com/Exiv2/exiv2/issues/933
+                   '((substitute* "tests/bugfixes/github/test_CVE_2018_12265.py"
+                        (("\\$uncaught_exception \\$addition_overflow_message\n") "")
+                        (("retval = \\[1\\]") "retval = [0]")))
+                   '()))))))
     (propagated-inputs
      (list expat zlib))
     (native-inputs
diff --git a/gnu/packages/patches/exiv2-ppc64.patch b/gnu/packages/patches/exiv2-ppc64.patch
deleted file mode 100644
index a74a7ac1b7..0000000000
--- a/gnu/packages/patches/exiv2-ppc64.patch
+++ /dev/null
@@ -1,11 +0,0 @@
---- /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]

base-commit: 212ca81895b2baa819ea11a308ad21880b84a546
prerequisite-patch-id: a7093ef8ccbab6d6dd7474a08f75970bcf3b9d4b
-- 
2.37.1

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

[Ludovic Courtès]

Hi Marcel,

I squashed the two patches into one, tweaked it to make the new build
phase conditional as a whole, added it for aarch64-linux as well, added
comments taken from your commit log, and tweaked the commit log.

Thank you, and thanks Maxime for reviewing!

Ludo’.

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

[Marcel van der Boom]

Anything else needed for this?