GNU bug report logs - #47413
[PATCH] Added DeaDBeeF package to music.scm

Previous Next

To reply to this bug, email your comments to 47413 AT debbugs.gnu.org.

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

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

From: Charlie Ruppe <ruppe.charlie <at> gmail.com>
To: guix-patches <at> gnu.org
Cc: Charlie Ruppe <ruppe.charlie <at> gmail.com>
Subject: [PATCH] Added DeaDBeeF package to music.scm
Date: Fri, 26 Mar 2021 11:56:59 -0400

---
 gnu/packages/music.scm | 53 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/gnu/packages/music.scm b/gnu/packages/music.scm
index 98cd3583cc..eb3933a5d6 100644
--- a/gnu/packages/music.scm
+++ b/gnu/packages/music.scm
@@ -598,6 +598,59 @@ It is a fork of Clementine aimed at music collectors and audiophiles.")
 many input formats and provides a customisable Vi-style user interface.")
      (license license:gpl2+)))
 
+(define-public deadbeef
+  (package 
+   (name "deadbeef")
+   (version "1.8.4")
+   (source (origin
+            (method git-fetch)
+            (uri (git-reference
+                  (url "git://github.com/Alexey-Yakovenko/deadbeef.git")
+                  (commit version)
+                  (recursive? #t)))
+            (sha256
+             (base32
+              "1mwiblsfzlp2jhhj5mfcy84wxfna9nmx2ia0i5l570hnhj3gxpwn"))))
+   (build-system glib-or-gtk-build-system)
+                  
+   (arguments '(#:configure-flags '("--enable-silent-rules")
+                #:tests? #f))
+   (inputs `(("gettext" ,gettext-minimal)
+             ("libtool" ,libtool)
+             ("intltool" ,intltool)
+             ("autoconf" ,autoconf)
+             ("automake" ,automake)
+             ("pkg-config" ,pkg-config)
+             ("libsamplerate" ,libsamplerate)
+             ("gtk+" ,gtk+)
+             ("jansson" ,jansson)
+             ("alsa-lib" ,alsa-lib)
+             ("libvorbis" ,libvorbis)
+             ("libogg" ,libogg)
+             ("curl" ,curl)
+             ("imlib2" ,imlib2)
+             ("libjpeg-turbo" ,libjpeg-turbo)
+             ("libmad" ,libmad)
+             ("mpg123" ,mpg123)
+             ("flac" ,flac)
+             ("wavpack" ,wavpack)
+             ("libsndfile" ,libsndfile)
+             ("libcdio" ,libcdio)
+             ("ffmpeg" ,ffmpeg)
+             ("xlib" ,xorg-server)
+             ("dbus" ,dbus)
+             ("pulseaudio" ,pulseaudio)
+             ("faad2" ,faad2)
+             ("zlib" ,zlib)
+             ("libzip" ,libzip)
+             ))
+   (synopsis "Modular audio player for desktop operating systems")
+   (description "DeaDBeeF (as in 0xDEADBEEF) is a modular audio player for GNU/Linux, *BSD, OpenSolaris, macOS, and other UNIX-like systems.
+
+DeaDBeeF lets you play variety of audio formats, convert between them, customize the UI almost any way you want, and use many additional plugins which can extend it even more.")
+   (home-page "https://deadbeef.sourceforge.io/")
+   (license (list license:gpl2+ license:lgpl2.1 license:zlib))))
+
 (define-public denemo
   (package
     (name "denemo")
-- 
2.31.0

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

From: Maxime Devos <maximedevos <at> telenet.be>
To: Charlie Ruppe <ruppe.charlie <at> gmail.com>, 47413 <at> debbugs.gnu.org
Subject: Re: [bug#47413] [PATCH] Added DeaDBeeF package to music.scm
Date: Fri, 26 Mar 2021 19:44:01 +0100

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

On Fri, 2021-03-26 at 11:56 -0400, Charlie Ruppe wrote:
> ---
>  gnu/packages/music.scm | 53 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)

You need to write a commit message (see the git repository of guix
for plenty of examples).  It's in the manual: ‘16.1 Submitting Patches’.

> diff --git a/gnu/packages/music.scm b/gnu/packages/music.scm
> index 98cd3583cc..eb3933a5d6 100644
> --- a/gnu/packages/music.scm
> +++ b/gnu/packages/music.scm
> @@ -598,6 +598,59 @@ It is a fork of Clementine aimed at music collectors and audiophiles.")
>  many input formats and provides a customisable Vi-style user interface.")
>       (license license:gpl2+)))
>  
> +(define-public deadbeef
> +  (package 
> +   (name "deadbeef")
> +   (version "1.8.4")
> +   (source (origin
> +            (method git-fetch)
> +            (uri (git-reference
> +                  (url "git://github.com/Alexey-Yakovenko/deadbeef.git")
> +                  (commit version)
> +                  (recursive? #t)))

I looked at <https://github.com/DeaDBeeF-Player/deadbeef/blob/master/.gitmodules>,
and it seems this repository does not pin the commits used, so this will break
when the submodule repositories are updated.

> +            (sha256
> +             (base32
> +              "1mwiblsfzlp2jhhj5mfcy84wxfna9nmx2ia0i5l570hnhj3gxpwn"))))
> +   (build-system glib-or-gtk-build-system)
> +                  
> +   (arguments '(#:configure-flags '("--enable-silent-rules")
> +                #:tests? #f))

Why are tests disabled?  When tests are disabled for a package,
a comment explaining why should be added. 

> +   (inputs `(("gettext" ,gettext-minimal)

These packages ...
> +             ("libtool" ,libtool)
> +             ("intltool" ,intltool)
> +             ("autoconf" ,autoconf)
> +             ("automake" ,automake)
> +             ("pkg-config" ,pkg-config)
... should be in native-inputs (see 8.2.1 ‘‘package‘ Reference’
in the manual).

These following packages should be sorted by name.
> +             ("libsamplerate" ,libsamplerate)
> +             ("gtk+" ,gtk+)
> +             ("jansson" ,jansson)
> +             ("alsa-lib" ,alsa-lib)
> +             ("libvorbis" ,libvorbis)
> +             ("libogg" ,libogg)
> +             ("curl" ,curl)
> +             ("imlib2" ,imlib2)
> +             ("libjpeg-turbo" ,libjpeg-turbo)
> +             ("libmad" ,libmad)
> +             ("mpg123" ,mpg123)
> +             ("flac" ,flac)
> +             ("wavpack" ,wavpack)
> +             ("libsndfile" ,libsndfile)
> +             ("libcdio" ,libcdio)
> +             ("ffmpeg" ,ffmpeg)
> +             ("xlib" ,xorg-server)
> +             ("dbus" ,dbus)
> +             ("pulseaudio" ,pulseaudio)
> +             ("faad2" ,faad2)
> +             ("zlib" ,zlib)
> +             ("libzip" ,libzip)
> +             ))

Also, ("xlib" ,xorg-server) does not make much sense to me.
Did you mean "libx11" or "xorg-server" here?
Please move the trailing )) to after ,libzip).

> +   (synopsis "Modular audio player for desktop operating systems")

‘for desktop operating systems’ seems redundant.  Since when can I
run Guix System on a so-called ‘smartphone’? (Why ‘quotes’?  They are
just small computers that can access the wireless telephone network.)

> +   (description "DeaDBeeF (as in 0xDEADBEEF) is a modular audio player for GNU/Linux, *BSD, OpenSolaris, macOS, and other UNIX-like systems.
> +DeaDBeeF lets you play variety of audio formats, convert between them, customize the UI almost any way you want, and use many additional plugins which can extend it even more.")
> 
This line is way to long.  This would be pointed out to you if you ran "guix lint"
(see the manual, 16.1 ‘Submitting Patches’)

Avoid marketing wording (‘almost any way you want’, ‘even more’, ‘modular’).
Everthing new is (supposedly) modular, well-written, supports many file formats,
are customisable ... It's like it's the law or something.  Just saying it is
extensible should be ok, though.  (See the manual, ‘Synopses and Descriptions’

Also, I don't see much reason to mention the various operating systems
it supports.  GNU Guix only supports GNU (/Linux & /Hurd).

It doesn't make much sense to mention DeaDBeeF has many plugins, if they
aren't packaged in Guix.

> +   (home-page "https://deadbeef.sourceforge.io/")

> +   (license (list license:gpl2+ license:lgpl2.1 license:zlib))))

*** Bundling problems

There is some bundling going on here!  In tools/glade, there is an
outdated copy of glade from 2007 (https://glade.gnome.org/), there
are also a copy of gettext from 2010 (see intl/ChangeLog)

The submodule deadbeef-osx-deps purely consists of bundling.
There may be some other bundling issues as well (I think I saw
some VLC code somewhere but I don't recall where.

Some bundling of cURL: plugins/artwork/escape.c

*** (Security) bugs

Look e.g. at plugins/artwork/artwork.c.  It assumes malloc always succeeds,
never returning NULL.  The function local_image_file does simply call
strcpy and does some iteration with assignement without any comments on
why everything is in bounds.  I probably could find similar issues elsewhere.

This project has some systematic bad security practices, so I believe a package
for this software should *not* be added to Guix proper (though feel free to
define it in your own channel, see the manual), particularily as there are plenty
of other sound / video players in Guix already.

That said, if "deadbeef" clears up it code base and the bundling issues are addressed
(as well as the much less serious issues of inputs vs. native-inputs and what exactly
goes into synopsis & description), I see no reason why the package couldn't go into
Guix.

I'm sorry if this is rather harsh, but attackers are a very real thing *and*
we have alternative software (e.g. vlc and gnome-music) that works just fine
and (I presume) are much more security-conscious and have more eyeballs looking
at the code ...

Greetings,
Maxime

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

Previous Next

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