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

  • Done
  • quality assurance status badge
Details
3 participants
  • Ludovic Courtès
  • Marcel van der Boom
  • Maxime Devos
Owner
unassigned
Submitted by
Marcel van der Boom
Severity
normal

Debbugs page

Marcel van der Boom wrote 3 years ago
(address . guix-patches@gnu.org)(name . Marcel van der Boom)(address . marcel@van-der-boom.nl)
2b700a6dc4b5b7dd09465c0ef7c04f73b055d463.1658777229.git.marcel@hsdev.com
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:


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

Toggle diff (37 lines)
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
Maxime Devos wrote 3 years ago
(name . Marcel van der Boom)(address . marcel@van-der-boom.nl)(address . 56766@debbugs.gnu.org)
9487ab96-82a7-8260-43bb-db3ac6b73688@telenet.be
On 25-07-2022 21:47, Marcel van der Boom wrote:
Toggle quote (3 lines)
> + (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: file
Attachment: OpenPGP_0x49E3EE22191725EE.asc
Attachment: OpenPGP_signature
Maxime Devos wrote 3 years ago
(name . Marcel van der Boom)(address . marcel@van-der-boom.nl)(address . 56766@debbugs.gnu.org)
61a863e3-c862-a799-c7b1-ba0d56bd92d0@telenet.be
On 25-07-2022 21:47, Marcel van der Boom wrote:
Toggle quote (11 lines)
> 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: OpenPGP_0x49E3EE22191725EE.asc
Attachment: OpenPGP_signature
Marcel van der Boom wrote 3 years ago
(name . Maxime Devos)(address . maximedevos@telenet.be)(address . 56766@debbugs.gnu.org)
877d3zemrq.fsf@van-der-boom.nl
Thanks for the review, some notes/questions inline below.

[Maxime Devos]:

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

Toggle quote (8 lines)
> 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
-----BEGIN PGP SIGNATURE-----

iQEzBAEBCAAdFiEE82U2v1mzEPiKRr69PyiQ+QAAY6AFAmLgNjkACgkQPyiQ+QAA
Y6CRBQf+PVAvxQp71c1D9KfDsU8pIQbnWiQKqUXZtUYVasVhidW95G5nc33jLxzC
xNUxJ/0vgjNvME85ZYjJcjmkA4LZLYP9KEZTDbWIHm9Mfu1qxRBzc1Ohw8GgrIdr
5Jhk6ODr4fv5YyrxjFERv5+WWSPYiSkRIWf58EBYLI8uEMElDs25YA2kC50yDPao
XbfK3g1q0rmpyksOyjwOx+NQsPNqwLMtHiWyyBmya8zdkuJH4gV6BXABH+Qd/pNl
7uZD5wh67qfMs6E9/VetNEBsy+KPmloCqmBv3pXBo7RtYahlGCgam7wTgkgdaVwb
yJxss5vTe8x5rvPVX1PqAMvoddzk+g==
=mfNZ
-----END PGP SIGNATURE-----

Maxime Devos wrote 3 years ago
(name . Marcel van der Boom)(address . marcel@van-der-boom.nl)(address . 56766@debbugs.gnu.org)
73c26ae4-d0f3-1dfb-99f0-1234b3020256@telenet.be
On 26-07-2022 20:38, Marcel van der Boom wrote:
Toggle quote (8 lines)
>> 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: file
Attachment: OpenPGP_0x49E3EE22191725EE.asc
Attachment: OpenPGP_signature
Maxime Devos wrote 3 years ago
(name . Marcel van der Boom)(address . marcel@van-der-boom.nl)(address . 56766@debbugs.gnu.org)
4f61b4a9-a299-dc9a-c817-a74909091a6c@telenet.be
Toggle quote (6 lines)
>> 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:
Toggle quote (11 lines)
> +--- /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:
Toggle quote (1 lines)
> 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.
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: file
Attachment: OpenPGP_0x49E3EE22191725EE.asc
Attachment: OpenPGP_signature
Marcel van der Boom wrote 3 years ago
(name . Maxime Devos)(address . maximedevos@telenet.be)(address . 56766@debbugs.gnu.org)
8735eneg6o.fsf@van-der-boom.nl
[Maxime Devos]

Toggle quote (4 lines)
> 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
-----BEGIN PGP SIGNATURE-----

iQEzBAEBCAAdFiEE82U2v1mzEPiKRr69PyiQ+QAAY6AFAmLgV48ACgkQPyiQ+QAA
Y6B2bggAq5jXEYmjXoe4EmqW1h2+m/6/vN11V/S0n1j0gnUwry15GpGDZSFI6rT2
uym3UAH11hVPE4b5G2NA/gSR2vIKxGqrHDbCAbRZEQ7IlW3Q/GimQq7n8asL0XwD
SfqiOiQtq6SeR0rN9eaJaxnaxiR2pQjrJ8nL3RnFojJFf39F0FoPdvFYaNDtXIjv
LF9eOPsUEELc7mVI6U4QsDYVuTBhrh5ds9cOGZBWzF47yDFJpC2o398y4uHT2pRA
+fOiG9hB2SYpAixXcmZcCsysgqfct4P8ycYl9BfScHjl4OdATloQyv4284edYOYh
1IMuFoRUrpo6UWBuRc7GcwhBtZQUDA==
=P2R3
-----END PGP SIGNATURE-----

Marcel van der Boom wrote 3 years ago
[PATCH] Adjust patch according to review
(address . 56766@debbugs.gnu.org)(name . Marcel van der Boom)(address . marcel@van-der-boom.nl)
4af56c93a8d990220918c65bc58c32f1c8c07aa3.1658907553.git.marcel@van-der-boom.nl
* 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

Toggle diff (60 lines)
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
Marcel van der Boom wrote 3 years ago
[PATCH] gnu: exiv2: Fix test failure on ppc64-le
(address . 56766@debbugs.gnu.org)
878ro8ctbf.fsf@van-der-boom.nl
Anything else needed for this?
Ludovic Courtès wrote 3 years ago
(name . Marcel van der Boom)(address . marcel@van-der-boom.nl)(address . 56766-done@debbugs.gnu.org)(name . Maxime Devos)(address . maximedevos@telenet.be)
87ilncyxir.fsf_-_@gnu.org
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’.
Closed
?
Your comment

This issue is archived.

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

To respond to this issue using the mumi CLI, first switch to it
mumi current 56766
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch
You may also tag this issue. See list of standard tags. For example, to set the confirmed and easy tags
mumi command -t +confirmed -t +easy
Or, remove the moreinfo tag and set the help tag
mumi command -t -moreinfo -t +help