Re: RFS: nuvolaplayer -- cloud music integration for Linux desktop
I forgot to mention I am the upstream author and all patches will be included
in the next release.
On Sat, Dec 31, 2011 at 05:59, Paul Wise <pabs@debian.org> wrote:
> 2011/12/31 Jiří Janoušek <janousek.jiri@gmail.com>:
>
>> To access further information about this package, please visit the following UR$
> ...
>> dget -x http://mentors.debian.net/debian/pool/main/n/nuvolaplayer/nuvolaplaye$
>
> Your MUA seems to be broken, it is truncating long lines instead of
> wrapping them.
Actually, it's my silly mistake. I wrote the body with nano in terminal to wrap
text around 80 chars and I copied the text from terminal instead of temporary
file. So, there is the whole line :-)
dget -x http://mentors.debian.net/debian/pool/main/n/nuvolaplayer/nuvolaplayer_1.0-1.dsc
> I don't intend to sponsor this but here is a quick review:
>
> Please use DEP-3 headers for the patches:
>
> http://dep.debian.net/deps/dep3/
Done. I've added also optional header "Forwarded: Author of this patch
is the upstream author.", because the default value is "no". Is it right?
> Ugh waf. waf has been removed from Debian because it is so horrible,
> you might want to get upstream to switch to something else.
>
> http://lists.debian.org/20100227195857.07540195@utumno
Unfortunately, waf is the only build system I have experiences with
and there is no plan to replace it in a short term. Is it a blocker for Debian?
> Please send the patches and manual page upstream.
The source of manual page is already in upstream, only its
build process is not in the build script.
> get-orig-source is not needed when there is a watch file present.
> The extra blank line in debian/watch isn't needed.
Both fixed.
> One file seems to be missing source code (Inkscape SVG):
>
> data/nuvolaplayer/selector/arrow.png
It's a fragment from graphics/old/working_files.svg. I've saved the fragment
as separate SVG and modified build script to use rsvg.
> One file seems to be missing source code (GIMP XCF):
>
> graphics/current/nuvolaplayer.png
This file is actually redundant, therefore I removed it. I've also removed old
icons used in 0.x series. The author didn't sent source SVG files anyway.
I will take more care about clean-up next time.
> This file contains pre-rendered text. I wonder which font was used and
> if that font is in Debian and is DFSG-free. If it is the Ubuntu font,
> IIRC that is not able to enter Debian main yet. It would be best to
> create this image at build time from the source SVG image, which seems
> to be missing.
>
> graphics/current/nuvola-playe.full-logo.png
The font is Ubuntu and the file was created in GIMP, because I had some issues
with shadows in the SVG file. This logo was created for website and there is no
need to distribute it in code tarball or Debian source package, because it is
not used by application. Removed.
> I would strongly recommend all the graphics be built from source SVG
> at build time instead of shipped in the package. You can use rsvg,
> inkscape or batik to create the PNG files, rsvg is probably the best
> one to use. At the very least there should be a way to rebuild them
> when they are removed. This will ensure they can be built on Debian.
> For the SVG files installed to /usr/share/icons/hicolor/scalable/apps/
> I would suggest running scour over them to reduce their size.
All icons are now generated from SVG files by rsvg, scalable icon is optimized
with scour. These tips are much appreciated, because it also makes graphics
easier to maintain.
Because I don't know how were the PNG files created (possibly directly from SVG,
from modified temporary SVG or from SVG and modified with GIMP) I think these
files are non-distributable, therefore I removed them from orig.tar.gz.
Did I understand this exception from "never modify orig.tar.gz" right?
Or should I remove them with patch instead?
> Why does the vapi/ dir contain gee-1.0.vapi? Shouldn't it be deleted
> and the one in libgee-dev be used instead?
As far I remember gee-1.0.vapi was not shipped in the past on Ubuntu
(Vala 0.10?), but you are right there is no reason to ship it now. Removed.
> Wow, vala generates C code that GCC really doesn't like.
Vala is known to produce code causing GCC warnings. However, the Vala developers
try reducing them, older versions produced more GCC-unfriendly code.
The GCC warnings can be ignored.
> Lots of dpkg-shlibdeps warnings.
I don't know how to solve this issue. I use pkg-config to resolve dependencies,
but for example gdk-2.0 returns extra dependencies gmodule, pangocairo,
gdk_pixbuf, ... Library pthread is added by valac.
$ /usr/bin/pkg-config gdk-2.0 --cflags --libs
-pthread -I/usr/include/gtk-2.0 -I/usr/lib/x86_64-linux-gnu/gtk-2.0/include
-I/usr/include/gio-unix-2.0/ -I/usr/include/cairo -I/usr/include/pango-1.0
-I/usr/include/gdk-pixbuf-2.0 -I/usr/include/glib-2.0
-I/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/usr/include/pixman-1
-I/usr/include/freetype2 -I/usr/include/libpng12 -pthread -lgdk-x11-2.0
-lpangocairo-1.0 -lgdk_pixbuf-2.0 -lpango-1.0 -lcairo -lgmodule-2.0
-lgobject-2.0 -lgthread-2.0 -lrt -lglib-2.0
> The package fails to build when built twice in a row,
> man/nuvolaplayer.1 needs to be removed on debian/rules clean (just add
> a debian/clean file).
Fixed. Can be "twice in a row building" tested with pbuilder?
> There are some duplicate files:
>
> graphics/old/scalable/apps/googlemusicframe.png
> graphics/old/256x256/apps/googlemusicframe.png
>
> data/icons/hicolor/48x48/apps/nuvolaplayer.png
> graphics/current/nuvola-player-48.png
>
> data/icons/hicolor/scalable/apps/nuvolaplayer.svg
> graphics/current/nuvola-player.orig.svg
>
> data/nuvolaplayer/services/googlemusic/description.html
> data/nuvolaplayer/services/googlemusic/description.txt
Fixed.
> debian/copyright misses copyright/license information for waf, the translations
Fixed.
Thank you very much for your time spent reviewing my package.
Cheers,
Jiří Janoušek
Reply to: