Bug 999268 - Review Request: gnome-music - Music player and management application for GNOME
Review Request: gnome-music - Music player and management application for GNOME
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Ankur Sinha (FranciscoD)
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2013-08-21 00:50 EDT by Mathieu Bridon
Modified: 2013-08-22 22:09 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-08-22 22:09:11 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
sanjay.ankur: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Mathieu Bridon 2013-08-21 00:50:40 EDT
Spec URL: http://bochecha.fedorapeople.org/packages/gnome-music.spec
SRPM URL: http://bochecha.fedorapeople.org/packages/gnome-music-3.9.90-1.fc21.src.rpm

Description:
Music player and management application for GNOME.

Fedora Account System Username: bochecha
Comment 1 Ralf Corsepius 2013-08-21 02:08:19 EDT
Building/compiling a package in Fedora is required to be verbose/non-silent.

Please append --disable-silent-rules to %configure to make it verbose.
Comment 2 Mathieu Bridon 2013-08-21 02:26:40 EDT
(In reply to Ralf Corsepius from comment #1)
> Building/compiling a package in Fedora is required to be verbose/non-silent.

I can't find anything about that in the guidelines, do you have a link?
Comment 3 Ralf Corsepius 2013-08-21 03:12:43 EDT
(In reply to Mathieu Bridon from comment #2)
> (In reply to Ralf Corsepius from comment #1)
> > Building/compiling a package in Fedora is required to be verbose/non-silent.
> 
> I can't find anything about that in the guidelines, do you have a link?

No I don't have a link at hand, but verbosity has been a requirement in Fedora rpm-specs ever since Fedora exits, because packages without are unmaintainable and maintains not doing so are working irresponsible.

How do you want to check your build.log for correctness if it doesn't contain the necessary informantion (Like your build.logs) E.g. how do you prove/check your CC calls receive the correct defines and includes?

You're in error to believe a CC call not erroring out means "clean compilation".
Comment 4 Ankur Sinha (FranciscoD) 2013-08-21 04:01:34 EDT
Hi,

I'll review this one. Thank you for your initial comments Ralf.

Warm regards,
Ankur
Comment 5 Elad Alfassa 2013-08-21 04:13:10 EDT
I see that your spec talks about "bundled libgd". Do we have any proper reason to bundle it?
Comment 6 Mathieu Bridon 2013-08-21 04:25:17 EDT
(In reply to Ralf Corsepius from comment #3)
> (In reply to Mathieu Bridon from comment #2)
> > (In reply to Ralf Corsepius from comment #1)
> > > Building/compiling a package in Fedora is required to be verbose/non-silent.
> > 
> > I can't find anything about that in the guidelines, do you have a link?
> 
> No I don't have a link at hand, but verbosity has been a requirement in
> Fedora rpm-specs ever since Fedora exits, because packages without are
> unmaintainable and maintains not doing so are working irresponsible.

You know, nobody forces you to maintain packages if you find them unmaintainable...

> How do you want to check your build.log for correctness if it doesn't
> contain the necessary informantion (Like your build.logs) E.g. how do you
> prove/check your CC calls receive the correct defines and includes?
> 
> You're in error to believe a CC call not erroring out means "clean
> compilation".

What makes you think that I believe this from the one line I wrote above?

I asked you for a link because I'm actually interested in learning if I missed something in the guidelines. No need to call me irresponsible, or to try and guess what people think...

When you assert that something is required, please add a link to the relevant part of the guidelines, to educate your fellow packagers.

I did search for it before asking, and could not find anything about it.

Now, as it happens, I do disagree with you: if it's not required, then I'd rather keep silent rules. Verbose build just spews way too much stuff to be useful in the general case, I prefer enabling it when I really want it (e.g when a build fails weirdly, or before submitting this review request ;)
Comment 7 Mathieu Bridon 2013-08-21 04:28:22 EDT
(In reply to Elad Alfassa from comment #5)
> I see that your spec talks about "bundled libgd". Do we have any proper
> reason to bundle it?

libgd is meant to be used that way. It's maintained upstream in its own git module, then bundled to a few gnome-* projects as a git submodule.

So the library is in fact maintained only in one place, but because it changes rapidly and is meant to be temporary (the stuff it contains slowly moves to Gtk once it matures) then it gets bundled into the source tarballs.

So yes, it's entirely expected to be bundled this way, it really wouldn't make any sense to package it on its own.

Note that it gets installed in a private directory so it isn't picked up automatically by the linker, and (unless I made a mistake) rpmbuild doesn't add any Provides on it.
Comment 8 Ankur Sinha (FranciscoD) 2013-08-21 05:31:19 EDT
Review:

[+] OK
[-] NA
[?] Issue

** Mandatory review guidelines: **
 [?] rpmlint output:
  [asinha@ankur  result]$ rpmlint *.rpm ~/rpmbuild/SPECS/gnome-music.spec ~/rpmbuild/SRPMS/gnome-music-3.9.90-1.fc21.src.rpm
gnome-music.src: W: invalid-url URL: http://wiki.gnome.org/Music HTTP Error 404: NOT FOUND
gnome-music.src:39: W: unversioned-explicit-provides bundled(libgd)
gnome-music.x86_64: W: invalid-url URL: http://wiki.gnome.org/Music HTTP Error 404: NOT FOUND
gnome-music.x86_64: W: no-manual-page-for-binary gnome-music
gnome-music-debuginfo.x86_64: W: invalid-url URL: http://wiki.gnome.org/Music HTTP Error 404: NOT FOUND
/home/asinha/rpmbuild/SPECS/gnome-music.spec:39: W: unversioned-explicit-provides bundled(libgd)
gnome-music.src: W: invalid-url URL: http://wiki.gnome.org/Music HTTP Error 404: NOT FOUND
gnome-music.src:39: W: unversioned-explicit-provides bundled(libgd)
4 packages and 1 specfiles checked; 0 errors, 8 warnings.
[asinha@ankur  result]$

The URL needs to be updated.

 [+] License is acceptable (...)
 [?] License field in spec is correct
Needs to add "with exceptions" in the GPLv2+ part

 [?] License files included in package %docs if included in source package
Not all the licenses are included. Not a blocker though.

 [-] License files installed when any subpackage combination is installed
 [+] Spec written in American English
 [+] Spec is legible
 [ ] Sources match upstream unless altered to fix permissibility issues
[asinha@ankur  SPECS]$ review-md5check.sh gnome-music.spec
Getting http://ftp.gnome.org/pub/GNOME/sources/gnome-music/3.9/gnome-music-3.9.90.tar.xz to /tmp/review/gnome-music-3.9.90.tar.xz
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 1305k  100 1305k    0     0  41626      0  0:00:32  0:00:32 --:--:-- 27715
7aca2f5922a12dd341f50414354f984f  /tmp/review/gnome-music-3.9.90.tar.xz
7aca2f5922a12dd341f50414354f984f /home/asinha/rpmbuild/SOURCES/gnome-music-3.9.90.tar.xz
removed ‘/tmp/review/gnome-music-3.9.90.tar.xz’
removed directory: ‘/tmp/review’
[asinha@ankur  SPECS]$
 [+] Build succeeds on at least one primary arch
 [-] Build succeeds on all primary arches or has ExcludeArch + bugs filed
 [+] BuildRequires correct, justified where necessary
 [?] Locales handled with %find_lang, not %_datadir/locale/*
find_lang has a --with-gnome flag. Worth checking if that should be used here.
https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#Handling_Locale_Files

 [-] %post, %postun call ldconfig if package contains shared .so files
 [?] No bundled libs
Internal library, so it should be fine. The library cannot be used by all apps
since they currently use their own versions temporarily.

 [-] Relocatability is justified
 [+] Package owns all directories it creates
 [+] Package requires others for directories it uses but does not own
 [+] No duplication in %files unless necessary for license files
 [+] File permissions are sane
 [+] Package contains permissible code or content
 [-] Large docs go in -doc subpackage
 [+] %doc files not required at runtime
 [-] Static libs go in -static package/virtual Provides
 [-] Development files go in -devel package
 [-] -devel packages Require base with fully-versioned dependency, %_isa
 [+] No .la files
 [?] GUI app uses .desktop file, installs it with desktop-file-install
Desktop file included but not installed using desktop-file-install or validate
https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#desktop-file-install_usage

 [+] File list does not conflict with other packages' without justification
 [+] File names are valid UTF-8

** Optional review guidelines: **
 [?] Query upstream about including license files
Would be good to include the other license files also

 [-] Translations of description, summary
 [+] Builds in mock
 [+] Builds on all arches
 [-] Functions as described (e.g. no crashes)
NOT TESTED

 [+] Scriptlets are sane
 [-] Subpackages require base with fully-versioned dependency if sensible
 [-] .pc file subpackage placement is sensible
 [+] No file deps outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin
 [-] Include man pages if available

Naming guidelines:
 [+] Package names use only a-zA-Z0-9-._+ subject to restrictions on -._+
 [+] Package names are sane
 [+] No naming conflicts
 [+] Spec file name matches base package name
 [+] Version is sane
 [+] Version does not contain ~
 [+] Release is sane
 [+] %dist tag
 [+] Case used only when necessary
 [+] Renaming handled correctly

Packaging guidelines:
 [+] Useful without external bits
 [+] No kmods
 [-] Pre-built binaries, libs removed in %prep
 [+] Sources contain only redistributable code or content
 [+] Spec format is sane
 [+] Package obeys FHS, except libexecdir, /run, /usr/target
 [+] No files in /bin, /sbin, /lib* on >= F17
 [-] Programs run before FS mounting use /run instead of /var/run
 [-] Binaries in /bin, /sbin do not depend on files in /usr on < F17
 [-] No files under /srv, /opt, /usr/local
 [+] Changelog in prescribed format
 [+] No Packager, Vendor, Copyright, PreReq tags
 [+] Summary does not end in a period
 [-] Correct BuildRoot tag on < EL6
 [-] Correct %clean section on < EL6
Do you plan to package this for EL?

 [+] Requires correct, justified where necessary
Comments with the Requires would be good to have.

 [+] Summary, description do not use trademarks incorrectly
 [+] All relevant documentation is packaged, appropriately marked with %doc
 [+] Doc files do not drag in extra dependencies (e.g. due to +x)
 [+] Code compilable with gcc is compiled with gcc
 [+] Build honors applicable compiler flags or justifies otherwise
Looks like, but a non verbose build would be more helpful

 [-] PIE used for long-running/root daemons, setuid/filecap programs
 [+] Useful -debuginfo package or disabled and justified
 [-] Package with .pc files Requires pkgconfig on < EL6
 [+] No static executables
 [+] Rpath absent or only used for internal libs
 [-] Config files marked with %config(noreplace) or justified %config
 [-] No config files under /usr
 [-] Third party package manager configs acceptable, in %_docdir
 [?] .desktop files are sane
Needs to be checked.

 [+] Spec uses macros consistently
 [+] Spec uses macros instead of hard-coded names where appropriate
 [+] Spec uses macros for executables only when configurability is needed
 [-] %makeinstall used only when alternatives don't work
 [-] Macros in Summary, description are expandable at srpm build time
 [+] Spec uses %{SOURCE#} instead of $RPM_SOURCE_DIR and %sourcedir
 [+] No software collections (scl)
 [-] Macro files named /etc/rpm/macros.%name
 [+] Build uses only python/perl/shell+coreutils/lua/BuildRequired langs
 [-] %global, not %define
 [?] Package translating with gettext BuildRequires it
Is this required?

 [-] Package translating with Linguist BuildRequires qt-devel
 [+] File ops preserve timestamps
 [+] Parallel make
 [+] No Requires(pre,post) notation
 [-] User, group creation handled correctly (See Packaging:UsersAndGroups)
 [-] Web apps go in /usr/share/%name, not /var/www
 [-] Conflicts are justified
 [+] One project per package
 [+] No bundled fonts
 [-] Patches have appropriate commentary
 [-] Available test suites executed in %check
 [-] tmpfiles.d used for /run, /run/lock on >= F15


 ** Python guidelines: **
 [+] Runtime Requires correct
Look OK

 [-] Python macros declared on < EL6
 [+] All .py files packaged with .pyc, .pyo counterparts
 [+] Includes .egg-info files/directories when generated
 [+] Provides/Requires properly filtered
 [-] Code that invokes gtk.gdk.get_pixels_array() Requires numpy


A few issues. Some are blockers, but they aren't too complicated to fix.
Comment 9 Ankur Sinha (FranciscoD) 2013-08-21 05:33:31 EDT
* a verbose build would be more helpful.
Comment 10 Mathieu Bridon 2013-08-21 23:07:26 EDT
Thanks for the feedback Ankur.

Here is a new package, fixing the issues you mentioned.

Note that I didn't add the --with-gnome option to the %find_lang macro because the package doesn't contain GNOME help files, and as a result the build fails with that option.

Also, due to popular request, I made the build verbose. One thing I hadn't thought about but you mentioned on IRC is that it makes the reviewer's job much easier when it comes to ensuring that the build flags are respected.

Spec URL: http://bochecha.fedorapeople.org/packages/gnome-music.spec
SRPM URL: http://bochecha.fedorapeople.org/packages/gnome-music-3.9.90-2.fc21.src.rpm
Comment 11 Ankur Sinha (FranciscoD) 2013-08-22 00:02:32 EDT
(In reply to Mathieu Bridon from comment #10)
> Thanks for the feedback Ankur.
> 
> Here is a new package, fixing the issues you mentioned.
> 
> Note that I didn't add the --with-gnome option to the %find_lang macro
> because the package doesn't contain GNOME help files, and as a result the
> build fails with that option.

I wasn't sure of this either. If it doesn't provide GNOME help files, the --with-gnome flag will fail. No worries. 

> 
> Also, due to popular request, I made the build verbose. One thing I hadn't
> thought about but you mentioned on IRC is that it makes the reviewer's job
> much easier when it comes to ensuring that the build flags are respected.

Thanks. The flags are indeed being respected. 

> 
> Spec URL: http://bochecha.fedorapeople.org/packages/gnome-music.spec
> SRPM URL:
> http://bochecha.fedorapeople.org/packages/gnome-music-3.9.90-2.fc21.src.rpm


Looks good to me. New rpmlint output:
[asinha@ankur  SRPMS]$ rpmlint ./gnome-music-3.9.90-2.fc21.src.rpm ../SPECS/gnome-music.spec /var/lib/mock/fedora-rawhide-x86_64/result/*.rpm
gnome-music.src:40: W: unversioned-explicit-provides bundled(libgd)
../SPECS/gnome-music.spec:40: W: unversioned-explicit-provides bundled(libgd)
gnome-music.src:40: W: unversioned-explicit-provides bundled(libgd)
gnome-music.x86_64: W: no-manual-page-for-binary gnome-music
4 packages and 1 specfiles checked; 0 errors, 4 warnings.

XXXXX APPROVED XXXX

Thanks,
Warm regards,
Ankur
Comment 12 Mathieu Bridon 2013-08-22 00:41:17 EDT
Thanks Ankur!

New Package SCM Request
=======================
Package Name: gnome-music
Short Description: Music player and management application for GNOME
Owners: bochecha
Branches: f20 devel
Comment 13 Jon Ciesla 2013-08-22 08:32:01 EDT
Git done (by process-git-requests).
Comment 14 Mathieu Bridon 2013-08-22 22:09:11 EDT
Thanks for the Git process Jon!

Package built in Rawhide and F20, closing.

Note You need to log in before you can comment on or make changes to this bug.