diff mbox series

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

Message ID 875z8pkck2.fsf@gnu.org
State Accepted
Headers show
Series [bug#43204] gnu: taglib: Propagate zlib. | expand

Checks

Context Check Description
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/applying patch success View Laminar job

Commit Message

Ludovic Courtès Sept. 7, 2020, 12:15 p.m. UTC 
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’.

Comments

Michael Rohleder Sept. 7, 2020, 1:26 p.m. UTC | #1 
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.
Pierre Langlois Sept. 7, 2020, 1:41 p.m. UTC | #2 
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
Ludovic Courtès Sept. 7, 2020, 10:55 p.m. UTC | #3 
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’.
diff mbox series

Patch

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")