[PATCH] gnu: gd: Requires.private to propagated inputs

  • Open
  • quality assurance status badge
Details
5 participants
  • Andreas Enge
  • Carlo Zancanaro
  • Marek Pa?nikowski
  • Tobias Geerinckx-Rice
  • Rutherther
Owner
unassigned
Submitted by
Rutherther
Severity
normal
Merged with
R
R
Rutherther wrote on 1 Sep 21:11 +0200
(address . guix-patches@gnu.org)(name . Rutherther)(address . rutherther@protonmail.com)
20240901191119.29870-1-rutherther@protonmail.com
The package gd provides a pkg-config file with all
its inputs in Requires.private. I think that this means that packages that depend
on gd also need these as inputs to build. This is causing trouble for example in php,
failing in configure phase with
```
checking for gdlib >= 2.1.0... no
configure: error: Package requirements (gdlib >= 2.1.0) were not met:

Package 'freetype2', required by 'gdlib', not found

Consider adjusting the PKG_CONFIG_PATH environment variable if you
installed software in a non-standard prefix.
```

After this, php is able to configure, though it still fails in the check
phase, where 3 tests fail. These tests are related to gd, so there seems
to be other problem as well, but I see also other tests for gd disabled,
so maybe it will be fine also disabling these three to fix this, I am not sure.
---
gnu/packages/gd.scm | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

Toggle diff (15 lines)
diff --git a/gnu/packages/gd.scm b/gnu/packages/gd.scm
index 98d34cfa71..7ced0774bd 100644
--- a/gnu/packages/gd.scm
+++ b/gnu/packages/gd.scm
@@ -85,7 +85,7 @@ (define-public gd
"\\.la$")))))))
(native-inputs
(list pkg-config))
- (inputs
+ (propagated-inputs
(list fontconfig
freetype
libjpeg-turbo
--
2.45.2
T
T
Tobias Geerinckx-Rice wrote on 2 Sep 00:24 +0200
(no subject)
(address . control@debbugs.gnu.org)
5507402b9ba5f0301534a1d092c63560@tobias.gr
merge 72943 72940
thanks
C
C
Carlo Zancanaro wrote on 2 Sep 01:33 +0200
Re: [bug#72943] [PATCH] gnu: gd: Requires.private to propagated inputs
(name . Rutherther)(address . rutherther@protonmail.com)(address . 72943@debbugs.gnu.org)
87mskrc5k8.fsf@zancanaro.id.au
Hi Rutherther,

I ran into the same issue with PHP, and prepared a slightly different
patch, which I have attached. Rather than propagating the inputs, I
reinstated a patch that we used to have, but which got removed in the
core-updates merge.

My assumption is that it was removed because the referenced GitHub issue
was closed, but the issue is not actually resolved upstream. They claim
it's a pkg-config bug (which has been open for 18 years), so I'm not
holding my breath for an upstream fix.

I also noticed the three failing tests, all related to libgd, but ran
out of time to investigate further.

Carlo
Attachment: 0001-gnu-gd-Restore-patch-to-fix-pkg-config-finding-libgd.patch
T
T
Tobias Geerinckx-Rice wrote on 2 Sep 13:53 +0200
(no subject)
(address . control@debbugs.gnu.org)
6921935c0d74dd5c7b96395f373f0085@tobias.gr
reassign 72962 guix-patches
merge 72943 72962
thanks
not
R
R
Rutherther wrote on 2 Sep 19:23 +0200
Re: [PATCH] gnu: gd: Requires.private to propagated inputs
(address . 72943@debbugs.gnu.org)
87v7zeezr1.fsf@protonmail.com
Hello Carlo,

thank you for finding that!
That explains why this issue was caused in the first place,
I was wondering how this could've been unspotted till now.

It seems much better to me to not use propagated-inputs whenever possible,
so I prefer your solution to this one. However, I do wonder about this: other packages that
have Requires.private do provide the libraries in these as propagated-inputs. Also
the cookbook shows to do this with such libraries. What is the correct approach here
then? Maybe every package that has Requires.private could be patched like this, possibly
somehow automatically instead of manual patches?

I am quite new here, this is my first patch. So I am not really sure how to go about this.
Will you open a new issue with your patch, and maintainers will decide patch from which
issue to use? Or is it enough it's in this issue?

I am sending an updated patch with a comment for why specifying
propagates-inputs would be necessary in the meantime,
as Tobias suggested me in a review in #guix IRC channel.

---
gnu/packages/gd.scm | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

Toggle diff (16 lines)
diff --git a/gnu/packages/gd.scm b/gnu/packages/gd.scm
index 98d34cfa71..1380493c44 100644
--- a/gnu/packages/gd.scm
+++ b/gnu/packages/gd.scm
@@ -85,7 +85,8 @@ (define-public gd
"\\.la$")))))))
(native-inputs
(list pkg-config))
- (inputs
+ ;; These libraries are in 'Requires.private' in libgd.pc.
+ (propagated-inputs
(list fontconfig
freetype
libjpeg-turbo
--
2.45.2
C
C
Carlo Zancanaro wrote on 3 Sep 02:19 +0200
Re: [bug#72943] [PATCH] gnu: gd: Requires.private to propagated inputs
(name . Rutherther)(address . rutherther@protonmail.com)
87bk15eghd.fsf@zancanaro.id.au
On Mon, Sep 02 2024, Rutherther via Guix-patches via wrote:
Toggle quote (7 lines)
> It seems much better to me to not use propagated-inputs whenever possible,
> so I prefer your solution to this one. However, I do wonder about this: other packages that
> have Requires.private do provide the libraries in these as propagated-inputs. Also
> the cookbook shows to do this with such libraries. What is the correct approach here
> then? Maybe every package that has Requires.private could be patched like this, possibly
> somehow automatically instead of manual patches?

Ah, I didn't look into it, so I didn't know that other packages in the
same situation just propagate their inputs. If that's more consistent
with the rest of Guix's packages then it would probably be better to do
that than restoring the patch.

Toggle quote (4 lines)
> I am quite new here, this is my first patch. So I am not really sure how to go about this.
> Will you open a new issue with your patch, and maintainers will decide patch from which
> issue to use? Or is it enough it's in this issue?

I wasn't planning to open a new issue. Ideally I think we would agree on
which change is more appropriate, then someone with commit access would
apply it.

If this problem is solved elsewhere in Guix with propagated-inputs, then
I think your change is a better idea than mine.

This still leaves the issue of the failing tests, though. I investigated
a bit and looking at [1] and [2], I believe the issue is that our
version of libgd doesn't support the BICUBIC interpolation method.

I'm rebuilding PHP now with those tests removed. I'll send through some
patches soon.

Carlo

C
C
Carlo Zancanaro wrote on 3 Sep 03:54 +0200
[PATCH 1/2] gnu: gd: Requires.private to propagated inputs
(address . 72943@debbugs.gnu.org)(name . Rutherther)(address . rutherther@protonmail.com)
e8531dfdc396ded5d5a93bf2687b79f731cb953a.1725328494.git.carlo@zancanaro.id.au
From: Rutherther <rutherther@protonmail.com>

* gnu/packages/gd.scm (gd): Change inputs to propagated-inputs.

Change-Id: Icb821d36a8250e7eee7d174dae8387949ac219a1
---
gnu/packages/gd.scm | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

Toggle diff (18 lines)
diff --git a/gnu/packages/gd.scm b/gnu/packages/gd.scm
index 98d34cfa71..1380493c44 100644
--- a/gnu/packages/gd.scm
+++ b/gnu/packages/gd.scm
@@ -85,7 +85,8 @@ (define-public gd
"\\.la$")))))))
(native-inputs
(list pkg-config))
- (inputs
+ ;; These libraries are in 'Requires.private' in libgd.pc.
+ (propagated-inputs
(list fontconfig
freetype
libjpeg-turbo

base-commit: b833aaaee7c95ec0339428a6b602f26831494798
--
2.45.2
C
C
Carlo Zancanaro wrote on 3 Sep 03:54 +0200
[PATCH 2/2] gnu: php: Disable tests relating to BICUBIC interpolation.
(address . 72943@debbugs.gnu.org)(name . Rutherther)(address . rutherther@protonmail.com)
ac7837e79805d82097b172389a614146cd47108b.1725328494.git.carlo@zancanaro.id.au
* gnu/packages/php.scm (php)[arguments]: Delete three tests that are known to
fail.

Change-Id: Ib684328654c75f37111d252fb0f9fb3356daff9a
---
gnu/packages/php.scm | 7 +++++++
1 file changed, 7 insertions(+)

Toggle diff (20 lines)
diff --git a/gnu/packages/php.scm b/gnu/packages/php.scm
index 8f879dbdca..ce7458d0e5 100644
--- a/gnu/packages/php.scm
+++ b/gnu/packages/php.scm
@@ -253,6 +253,13 @@ (define-public php
;; AVIF support disabled
"ext/gd/tests/imagecreatefromstring_avif.phpt"
+ ;; These tests fail due to issues in upstream gd
+ ;; 2.3.3 around BICUBIC interpolation. See
+ ;; https://github.com/libgd/libgd/issues/847
+ "ext/gd/tests/bug79676.phpt"
+ "ext/gd/tests/imageinterpolation_basic.phpt"
+ "ext/gd/tests/imagescale_preserve_ratio.phpt"
+
;; XXX: These test failures appear legitimate, needs investigation.
;; open_basedir() restriction failure.
"ext/curl/tests/curl_setopt_ssl.phpt"
--
2.45.2
M
M
Marek Pa?nikowski wrote on 4 Sep 08:50 +0200
Re: bug#72962: php-8.3.10 build failure
(name . Carlo Zancanaro)(address . carlo@zancanaro.id.au)
87cyljzzcw.fsf_-_@marekpasnikowski.pl
Carlo Zancanaro <carlo@zancanaro.id.au> writes:

Toggle quote (4 lines)
> * gnu/packages/php.scm (php)[arguments]: Delete three tests that are known to
> fail.
>

This is my first time composing a patch review — I am not sure if I am
doing it correctly.

I attempted to build php with the following command:
=
guix build php --with-patch=gd=patch-1.txt --with-patch=php=patch-2.txt
=

The text files are files saved by using the “download” option of the web
interface of Guix Issues. My understanding is that the patch to gd
applies cleanly, but the patch to php does not:

=
source is at 'php-8.3.10'
applying '/gnu/store/f2zxspb49cbcmcla3mbjrikl48kmbf52-patch-2.txt'...
can't find file to patch at input line 13
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|* gnu/packages/php.scm (php)[arguments]: Delete three tests that are known to
|fail.
|
|Change-Id: Ib684328654c75f37111d252fb0f9fb3356daff9a
|---
| gnu/packages/php.scm | 7 +++++++
| 1 file changed, 7 insertions(+)
|
|diff --git a/gnu/packages/php.scm b/gnu/packages/php.scm
|index 8f879dbdca..ce7458d0e5 100644
|--- a/gnu/packages/php.scm
|+++ b/gnu/packages/php.scm
--------------------------
No file to patch. Skipping patch.
1 out of 1 hunk ignored
patch unexpectedly ends in middle of line
=

Is this helpful?
C
C
Carlo Zancanaro wrote on 4 Sep 09:27 +0200
(name . Marek Pa?nikowski)(address . marek@marekpasnikowski.pl)
87mskn3mlk.fsf@zancanaro.id.au
Hi Marek!

On Wed, Sep 04 2024, Marek Pa?nikowski wrote:
Toggle quote (3 lines)
> This is my first time composing a patch review — I am not sure if I am
> doing it correctly.

Thanks for taking a look at my patch!

Toggle quote (5 lines)
> I attempted to build php with the following command:
> =
> guix build php --with-patch=gd=patch-1.txt --with-patch=php=patch-2.txt
> =

I don't expect this command to work. The --with-patch argument expects
to apply the patch to the source of the package, whereas the patches I
have prepared should be applied on a checkout of the Guix git repository

You should be able to apply them in a fresh clone of the repository with
"git am $patch-name". To build, you can follow the instructions in the
manual "(guix) Building from Git". After that, you can build PHP with
these patches with "./pre-inst-env guix build php".

This can take a bit of time, but things are easier once you're set up
with a working Guix clone.

Carlo
M
M
Marek Pa?nikowski wrote on 4 Sep 09:43 +0200
(name . Carlo Zancanaro)(address . carlo@zancanaro.id.au)
87plpjrhhq.fsf_-_@marekpasnikowski.pl
Carlo Zancanaro <carlo@zancanaro.id.au> writes:

Toggle quote (11 lines)
>> I attempted to build php with the following command:
>> =
>> guix build php --with-patch=gd=patch-1.txt --with-patch=php=patch-2.txt
>> =
>
> You should be able to apply them in a fresh clone of the repository with
> "git am $patch-name". To build, you can follow the instructions in the
> manual "(guix) Building from Git". After that, you can build PHP with
> these patches with "./pre-inst-env guix build php".
>

I had a guix clone from previous attempts to patch things, so I
proceeded to apply the patches. Patch 1 returned a message along the
lines (translation) of "apply: * gnu/packages/gd.scm (gd): Change inputs
to propagated-inputs". However,
=
LC_ALL=C git am ../Downloads/patch-2.txt
Patch format detection failed.
=
M
M
Marek Pa?nikowski wrote on 4 Sep 09:59 +0200
(name . Carlo Zancanaro)(address . carlo@zancanaro.id.au)
87ttevq273.fsf_-_@marekpasnikowski.pl
Upon further inspection, I noticed the second patch is missing the
From: <email> header. I copied it from the first patch and it applied.

The build is now in progress on my slow laptop, so I am sending this
report immediately.
M
M
Marek Pa?nikowski wrote on 4 Sep 12:32 +0200
(name . Carlo Zancanaro)(address . carlo@zancanaro.id.au)
878qw7zp37.fsf_-_@marekpasnikowski.pl
The build completed and passed all the tests.

Is this the right moment to say reviewed-looks-good? Would I do it by
writing the usertag followed by thanks in the beginning of the message?
Or am I confusing concepts?
A
A
Andreas Enge wrote on 5 Sep 22:41 +0200
Patches applied
ZtoXlEjn2BP-uDLu@jurong
Hello,

I have applied Carlos's second patch of
which gives an explanation why we skip the tests,
and Noￜ's patch of
adding the missing inputs.

As php currently does not build after the core-updates merge and this issue
has been turning up regularly over the past few days, I have taken the
liberty to push the commits directly without going through QA; I have
tested that the package builds and works with one of my local php projects.

I am closing the second issue, which is thus handled.
And I am leaving the first issue open; while the immediate php problem is
(hopefully) solved, it remains to be discussed whether we should propagate
the gd inputs in the longer term.

My understanding is that given the pkg-config file, we normally would
propagate the inputs. On the other hand, propagated inputs tend to create
problems (for instance, when two different packages propagate two different
versions of the same input library); and I do not quite understand why
with over 5000 packages depending on gd, most of them do not seem to be
affected. Maybe these do not use pkg-config to check for gd?
So it may be a better option to only patch the affected packages (if any
are left) and leave gd as it is.

Andreas
?
Your comment

Commenting via the web interface is currently disabled.

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

To respond to this issue using the mumi CLI, first switch to it
mumi current 72943
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