[bug#43204] [PATCH] gnu: taglib: Propagate zlib.

[bug#43204] [PATCH] gnu: taglib: Propagate zlib.

[Michael Rohleder]

* gnu/packages/mp3.scm (taglib)[inputs]: Move zlib to [propagated-inputs].
---
It seems, consumer of taglib (commit 89e1e44813) needs to be linked w/ libz
according to the installed pkg-config.

I noticed that emacs-emms-print-metadata fails to link with a missing -lz lib,
(I guess, all revdeps of taglib that don't have zlib as an input have that problem)

 gnu/packages/mp3.scm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gnu/packages/mp3.scm b/gnu/packages/mp3.scm
index 7ee009df74..8ea282be97 100644
--- a/gnu/packages/mp3.scm
+++ b/gnu/packages/mp3.scm
@@ -175,7 +175,7 @@ a highly stable and efficient implementation.")
     (arguments
       '(#:tests? #f ; Tests are not ran with BUILD_SHARED_LIBS on.
         #:configure-flags (list "-DBUILD_SHARED_LIBS=ON")))
-    (inputs `(("zlib" ,zlib)))
+    (propagated-inputs `(("zlib" ,zlib)))
     (home-page "https://taglib.org")
     (synopsis "Library to access audio file meta-data")
     (description
-- 
2.28.0

[bug#43204] [PATCH] gnu: taglib: Propagate zlib.

[Pierre Langlois]

Hi Michael,

Michael Rohleder writes:

> * gnu/packages/mp3.scm (taglib)[inputs]: Move zlib to [propagated-inputs].
> ---
> It seems, consumer of taglib (commit 89e1e44813) needs to be linked w/ libz
> according to the installed pkg-config.
>
> I noticed that emacs-emms-print-metadata fails to link with a missing -lz lib,
> (I guess, all revdeps of taglib that don't have zlib as an input have that problem)

Oh, indeed emacs-emms doesn't build, sorry for the breakage! :-/ I see
the pkg-config file was changed here https://github.com/taglib/taglib/commit/ef1312d62239f399c40233d76ef3328b8dadf984

Propagating zlib seems like the right thing to do (although I'm not a
maintainer), thanks for the patch!

Pierre

[bug#43204] [PATCH] gnu: taglib: Propagate zlib.

[Pierre Langlois]

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


Pierre Langlois writes:

> Hi Michael,
>
> Michael Rohleder writes:
>
>> * gnu/packages/mp3.scm (taglib)[inputs]: Move zlib to [propagated-inputs].
>> ---
>> It seems, consumer of taglib (commit 89e1e44813) needs to be linked w/ libz
>> according to the installed pkg-config.
>>
>> I noticed that emacs-emms-print-metadata fails to link with a missing -lz lib,
>> (I guess, all revdeps of taglib that don't have zlib as an input have that problem)
>
> Oh, indeed emacs-emms doesn't build, sorry for the breakage! :-/ I see
> the pkg-config file was changed here https://github.com/taglib/taglib/commit/ef1312d62239f399c40233d76ef3328b8dadf984
>
> Propagating zlib seems like the right thing to do (although I'm not a
> maintainer), thanks for the patch!

Actually, thinking about this a little more, I'm not sure I understand
upstream decision to propagate -lz. The commit fixes [0] which indicates
it's so that taglib can be linked statically, but then that means if
we're dynamically linking, the application will also dynamically link
with zlib when it doesn't need to (at least not directly). And in guix
we only build shared libs for taglib so we're never statically linking
it AFAIK.

So, here I'm a bit torn here, should we just follow what upstream is
indicating? Even it doesn't look right to me, but I might be wrong! Or,
should we revert the change that propagates -lz?

Thanks,
Pierre

[0]: https://github.com/taglib/taglib/issues/872

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

[bug#43204] [PATCH] gnu: taglib: Propagate zlib.

[Ludovic Courtès]

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

Hi!

Pierre Langlois <pierre.langlois@gmx.com> skribis:

> Actually, thinking about this a little more, I'm not sure I understand
> upstream decision to propagate -lz. The commit fixes [0] which indicates
> it's so that taglib can be linked statically, but then that means if
> we're dynamically linking, the application will also dynamically link
> with zlib when it doesn't need to (at least not directly). And in guix
> we only build shared libs for taglib so we're never statically linking
> it AFAIK.
>
> So, here I'm a bit torn here, should we just follow what upstream is
> indicating? Even it doesn't look right to me, but I might be wrong! Or,
> should we revert the change that propagates -lz?

I had the following patch that I intended to push, to avoid propagation.

WDYT?

Ludo’.


[-- Attachment #2: Type: text/x-patch, Size: 2038 bytes --]

commit d8124a707602980556fd33c7dbf9f7483fe1d0df
Author: Ludovic Courtès <ludo@gnu.org>
Date:   Mon Sep 7 09:56:08 2020 +0200

    gnu: taglib: 'taglib-config --libs' shows -L flag for zlib.
    
    Fixes compilation of emacs-emms-print-metadata.
    
    * gnu/packages/mp3.scm (taglib)[arguments]: Add #:phases.

diff --git a/gnu/packages/mp3.scm b/gnu/packages/mp3.scm
index 7ee009df74..a7574f0cf9 100644
--- a/gnu/packages/mp3.scm
+++ b/gnu/packages/mp3.scm
@@ -1,6 +1,6 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2013 Andreas Enge <andreas@enge.fr>
-;;; Copyright © 2014, 2015, 2017 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2014, 2015, 2017, 2020 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2015 Mark H Weaver <mhw@netris.org>
 ;;; Copyright © 2016 Efraim Flashner <efraim@flashner.co.il>
 ;;; Copyright © 2017 Thomas Danckaert <post@thomasdanckaert.be>
@@ -174,7 +174,18 @@ a highly stable and efficient implementation.")
     (build-system cmake-build-system)
     (arguments
       '(#:tests? #f ; Tests are not ran with BUILD_SHARED_LIBS on.
-        #:configure-flags (list "-DBUILD_SHARED_LIBS=ON")))
+        #:configure-flags (list "-DBUILD_SHARED_LIBS=ON")
+        #:phases (modify-phases %standard-phases
+                   (add-before 'configure 'adjust-zlib-ldflags
+                     (lambda* (#:key inputs #:allow-other-keys)
+                       ;; Make sure users of 'taglib-config --libs' get the -L
+                       ;; flag for zlib.
+                       (substitute* "CMakeLists.txt"
+                         (("set\\(ZLIB_LIBRARIES_FLAGS -lz\\)")
+                          (string-append "set(ZLIB_LIBRARIES_FLAGS -L"
+                                         (assoc-ref inputs "zlib")
+                                         " -lz)")))
+                       #t)))))
     (inputs `(("zlib" ,zlib)))
     (home-page "https://taglib.org")
     (synopsis "Library to access audio file meta-data")

[bug#43204] [PATCH] gnu: taglib: Propagate zlib.

[Michael Rohleder]

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

Hey Ludo,

Ludovic Courtès <ludo@gnu.org> writes:
> I had the following patch that I intended to push, to avoid propagation.
>
> WDYT?
>
> commit d8124a707602980556fd33c7dbf9f7483fe1d0df
> Author: Ludovic Courtès <ludo@gnu.org>
> Date:   Mon Sep 7 09:56:08 2020 +0200
>
>     gnu: taglib: 'taglib-config --libs' shows -L flag for zlib.
>     
>     Fixes compilation of emacs-emms-print-metadata.
>     
>     * gnu/packages/mp3.scm (taglib)[arguments]: Add #:phases.
>

Nice!
I think we (or upstream) should do something like this anyway.

-- 
"If you want to travel around the world and be invited to speak at a lot
of different places, just write a Unix operating system."
Linus Torvalds

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

[bug#43204] [PATCH] gnu: taglib: Propagate zlib.

[Pierre Langlois]

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


Ludovic Courtès writes:

> Hi!
>
> Pierre Langlois <pierre.langlois@gmx.com> skribis:
>
>> Actually, thinking about this a little more, I'm not sure I understand
>> upstream decision to propagate -lz. The commit fixes [0] which indicates
>> it's so that taglib can be linked statically, but then that means if
>> we're dynamically linking, the application will also dynamically link
>> with zlib when it doesn't need to (at least not directly). And in guix
>> we only build shared libs for taglib so we're never statically linking
>> it AFAIK.
>>
>> So, here I'm a bit torn here, should we just follow what upstream is
>> indicating? Even it doesn't look right to me, but I might be wrong! Or,
>> should we revert the change that propagates -lz?
>
> I had the following patch that I intended to push, to avoid propagation.
>
> WDYT?
>
> Ludo’.
>
> commit d8124a707602980556fd33c7dbf9f7483fe1d0df
> Author: Ludovic Courtès <ludo@gnu.org>
> Date:   Mon Sep 7 09:56:08 2020 +0200
>
>     gnu: taglib: 'taglib-config --libs' shows -L flag for zlib.
>     
>     Fixes compilation of emacs-emms-print-metadata.
>     
>     * gnu/packages/mp3.scm (taglib)[arguments]: Add #:phases.

LGTM!

I was originally thinking we could just drop the `-lz`, since it
/should/ only be needed for people who statically link with taglib, and
we only ship shared libs. But actually, it's probably safer to follow
what upstream is doing.

Thanks,
Pierre

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

bug#43204: [PATCH] gnu: taglib: Propagate zlib.

[Ludovic Courtès]

Pierre Langlois <pierre.langlois@gmx.com> skribis:

> Ludovic Courtès writes:

[...]

>> commit d8124a707602980556fd33c7dbf9f7483fe1d0df
>> Author: Ludovic Courtès <ludo@gnu.org>
>> Date:   Mon Sep 7 09:56:08 2020 +0200
>>
>>     gnu: taglib: 'taglib-config --libs' shows -L flag for zlib.
>>     
>>     Fixes compilation of emacs-emms-print-metadata.
>>     
>>     * gnu/packages/mp3.scm (taglib)[arguments]: Add #:phases.
>
> LGTM!
>
> I was originally thinking we could just drop the `-lz`, since it
> /should/ only be needed for people who statically link with taglib, and
> we only ship shared libs. But actually, it's probably safer to follow
> what upstream is doing.

Yeah.  Pushed as d2a7114e0b46fccad1e02e301c58a5124f361c5c, thanks!

Ludo’.