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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 56766 in the body.
You can then email your comments to 56766 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Marcel van der Boom <marcel <at> van-der-boom.nl>
To: guix-patches <at> gnu.org
Cc: Marcel van der Boom <marcel <at> van-der-boom.nl>
Subject: [PATCH] gnu: exiv2: Fix test failure on ppc64-le
Date: Mon, 25 Jul 2022 21:47:33 +0200

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

Message #8 received at 56766 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: Marcel van der Boom <marcel <at> van-der-boom.nl>, 56766 <at> debbugs.gnu.org
Subject: Re: [bug#56766] [PATCH] gnu: exiv2: Fix test failure on ppc64-le
Date: Tue, 26 Jul 2022 20:01:12 +0200

[Message part 1 (text/plain, inline)]

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.

[Message part 2 (text/html, inline)]

[OpenPGP_0x49E3EE22191725EE.asc (application/pgp-keys, attachment)]

[OpenPGP_signature (application/pgp-signature, attachment)]

Message #11 received at 56766 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: Marcel van der Boom <marcel <at> van-der-boom.nl>, 56766 <at> debbugs.gnu.org
Subject: Re: [bug#56766] [PATCH] gnu: exiv2: Fix test failure on ppc64-le
Date: Tue, 26 Jul 2022 20:02:14 +0200

[Message part 1 (text/plain, inline)]

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.

[OpenPGP_0x49E3EE22191725EE.asc (application/pgp-keys, attachment)]

[OpenPGP_signature (application/pgp-signature, attachment)]

Message #14 received at 56766 <at> debbugs.gnu.org (full text, mbox):

From: Marcel van der Boom <marcel <at> van-der-boom.nl>
To: Maxime Devos <maximedevos <at> telenet.be>
Cc: 56766 <at> debbugs.gnu.org
Subject: Re: [bug#56766] [PATCH] gnu: exiv2: Fix test failure on ppc64-le
Date: Tue, 26 Jul 2022 20:38:51 +0200

[Message part 1 (text/plain, inline)]

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

[signature.asc (application/pgp-signature, inline)]

Message #17 received at 56766 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: Marcel van der Boom <marcel <at> van-der-boom.nl>
Cc: 56766 <at> debbugs.gnu.org
Subject: Re: [bug#56766] [PATCH] gnu: exiv2: Fix test failure on ppc64-le
Date: Tue, 26 Jul 2022 20:55:13 +0200

[Message part 1 (text/plain, inline)]

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.

[Message part 2 (text/html, inline)]

[OpenPGP_0x49E3EE22191725EE.asc (application/pgp-keys, attachment)]

[OpenPGP_signature (application/pgp-signature, attachment)]

Message #20 received at 56766 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: Marcel van der Boom <marcel <at> van-der-boom.nl>, 56766 <at> debbugs.gnu.org
Subject: Re: [bug#56766] [PATCH] gnu: exiv2: Fix test failure on ppc64-le
Date: Tue, 26 Jul 2022 21:11:16 +0200

[Message part 1 (text/plain, inline)]

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

[Message part 2 (text/html, inline)]

[OpenPGP_0x49E3EE22191725EE.asc (application/pgp-keys, attachment)]

[OpenPGP_signature (application/pgp-signature, attachment)]

Message #23 received at 56766 <at> debbugs.gnu.org (full text, mbox):

From: Marcel van der Boom <marcel <at> van-der-boom.nl>
To: Maxime Devos <maximedevos <at> telenet.be>
Cc: 56766 <at> debbugs.gnu.org
Subject: Re: [bug#56766] [PATCH] gnu: exiv2: Fix test failure on ppc64-le
Date: Tue, 26 Jul 2022 21:34:00 +0200

[Message part 1 (text/plain, inline)]

[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

[signature.asc (application/pgp-signature, inline)]

Message #26 received at 56766 <at> debbugs.gnu.org (full text, mbox):

From: Marcel van der Boom <marcel <at> van-der-boom.nl>
To: 56766 <at> debbugs.gnu.org
Cc: Marcel van der Boom <marcel <at> van-der-boom.nl>
Subject: [PATCH] Adjust patch according to review
Date: Wed, 27 Jul 2022 09:41:44 +0200

* 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

Message #29 received at 56766 <at> debbugs.gnu.org (full text, mbox):

From: Marcel van der Boom <marcel <at> van-der-boom.nl>
To: 56766 <at> debbugs.gnu.org
Subject: [PATCH] gnu: exiv2: Fix test failure on ppc64-le
Date: Mon, 01 Aug 2022 09:32:10 +0200

Anything else needed for this?

Message #34 received at 56766-done <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Marcel van der Boom <marcel <at> van-der-boom.nl>
Cc: 56766-done <at> debbugs.gnu.org, Maxime Devos <maximedevos <at> telenet.be>
Subject: Re: bug#56766: [PATCH] gnu: exiv2: Fix test failure on ppc64-le
Date: Mon, 01 Aug 2022 14:10:52 +0200

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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.