[bug#47413] [PATCH] Added DeaDBeeF package to music.scm

[Maxime Devos]

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
Description: This is a digitally signed message part