[Bug 1829] Review request: deadbeef - A music player with cue sheet support

[RPM Fusion Bugzilla]

http://bugzilla.rpmfusion.org/show_bug.cgi?id=1829


Richard <hobbes1069 at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |hobbes1069 at gmail.com




--- Comment #21 from Richard <hobbes1069 at gmail.com>  2011-10-06 20:14:49 ---
Some comments on the spec file:

1. BuildRoot:, "rm -rf $RPM_BUILD_ROOT" can be removed from %install, %clean
can be removed entirely, and "%defattr(-,root,root)" can be removed from %files
unless your building for EL5.

2. In you devel subpackage the Require: should probably be arch dependent:

Requires: %{name} = %{version}-%{release}

to

Requires: %{name}%{?_isa} = %{version}-%{release}

3. Modifying the desktop file in %prep is odd. desktop-file-install can modify
desktop files on the fly, so if it's capable of removing the Unity parts then I
would use it instead. Just research it's options.

If desktop-file-install is not capable of making the needed edits and you still
need to resort to sed then at least move it to just above your
desktop-file-install command. 

Also, currently you're not modifying anything in the desktop file by the time
you get to desktop-file-install so you could use desktop-file-validate instead.

See:
http://fedoraproject.org/wiki/Packaging:Guidelines#desktop-file-install_usage

4. Ok, you definitely took Michael's comments to heart, but perhaps went a
little too far :)

I would only add comments in %files for unusual situations. You can drop #doc,
# Files, and # Dirs.

5. %{_defaultdocdir} can just be %{_docdir}

6. In the case of the extra doc directory there's no hard rule for this that I
know of but it's my opinion that %exclude use in %files should be limited to
excluding files from one package so they can be included in a sub-package.

For instance, you might run into a situation where it would be difficult or
overly verbose to work around files that you really want to go into a -devel or
-doc subpackage. Here %exclude could be used to exclude files or directors in
the main package so they can be included in -doc or -devel.

If you're not going to include files in a package at all then it's better to
just remove them in %install like you're already doing for .la files.

For the static libraries (.a) I would just remove them if there's nothing that
actually needs them. If you need them then they should go in a -static
subpackage not in -devel.


-- 
Configure bugmail: http://bugzilla.rpmfusion.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.
You are the assignee for the bug.