Bug 504178 - Review Request: gstreamer-plugins-fc - Future Composer input plugin for GStreamer
Review Request: gstreamer-plugins-fc - Future Composer input plugin for GStre...
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Ding-Yi Chen
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-06-04 11:47 EDT by Michael Schwendt
Modified: 2010-01-09 10:13 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-01-09 10:13:13 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
dchen: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Michael Schwendt 2009-06-04 11:47:04 EDT
Spec URL: http://mschwendt.fedorapeople.org/gstreamer-plugins-fc.spec
SRPM URL: http://mschwendt.fedorapeople.org/gstreamer-plugins-fc-0.1-1.fc10.src.rpm
Description: This is an input plugin for GStreamer which can play back Future Composer music files from AMIGA. Song-length detection and seek are implemented, too.

[...]

Example files can be found here - for example ;-)
http://xmms-fc.sourceforge.net/favfcmods2.tgz
Comment 1 Jochen Schmitt 2009-06-04 14:35:34 EDT
Some first comments:

Good:
+ Basename of SPEC file matches with package name
+ Package name fullfill naming guidelines
+ URL tag contains proper project homepage
+ Package contains most recent release of the software


Bad:
- Please use %glogal instead of %define
- Could not download source tar ball via spectool -g
Comment 2 Michael Schwendt 2009-06-04 14:50:25 EDT
> - Please use %glogal instead of %define

Why?

> - Could not download source tar ball via spectool -g  

Works for me. How does it fail for you?
Comment 3 Jason Tibbitts 2009-11-12 21:05:57 EST
I'm just looking over some old review tickets.

The answer to "Why?" is 
http://fedoraproject.org/wiki/Packaging:Guidelines#.25global_preferred_over_.25define
I don't think it particularly matters in this case, but it's in the guidelines and if for some reason you really want to use %define instead of %global you should expect reviewers to ask you to justify it.

I didn't have any problem downloading the source, but the tarball I got doesn't match the one in this package.  The contents seem to be the same; perhaps they just recompressed the file.
Comment 4 Michael Schwendt 2009-11-13 05:47:44 EST
Okay. All that was wrong was the tarball file extension:

$ file gstreamer-plugin-fc-0.1.tar.bz2
gstreamer-plugin-fc-0.1.tar.bz2: gzip compressed data, from Unix, last modified: Tue Oct 14 13:36:25 2008

$ md5sum gstreamer-plugin-fc-0.1.tar.bz2 
69af63d11dd3eae161969cc9ede10d4f  gstreamer-plugin-fc-0.1.tar.bz2
$ md5sum gstreamer-plugin-fc-0.1.tar.gz
69af63d11dd3eae161969cc9ede10d4f  gstreamer-plugin-fc-0.1.tar.gz

I've replaced the tarball with the .bz2 release.

[...]

Usage of  %define  in this very short .spec file is fine and doesn't cause any problems. No nested macro expansion is needed.

[...]

Spec URL: http://mschwendt.fedorapeople.org/gstreamer-plugins-fc.spec
SRPM URL: http://mschwendt.fedorapeople.org/gstreamer-plugins-fc-0.1-2.fc11.src.rpm
Comment 5 Ding-Yi Chen 2010-01-05 00:51:32 EST
I've tested this package.
It seems working with totem, and built sucessfully in koji for F-12, F-11 and rawhide.

However, just one concern:
Indeed most of plugins have prefix gstreamer-plugin'S' and includes a few plugins. This package on the other hand, only contain sone plugin, 
you may just use the upstream name: gstreamer-plugin-fc.
Well, this is up to your justification.

What say you?
Comment 6 Michael Schwendt 2010-01-06 15:10:40 EST
Well, either one would be fine, IMO.

With audacious-plugin-fc and audacious-plugin-xmp, some packagers complained about not putting them into the "audacious-plugins-" namespace. :)

Hence I appended the "s" to match all other gstreamer-plugins- packages. There's nothing in the guidelines about this. There has been only one good rationale for appending the "s" even if it's just a single plugin: Users who are accustomed to queries like "yum list gstreamer-plugins\*" would not see the packages without "s".

Btw, some of the audacious-plugins-foo packages contain only a single plugin, too.
Comment 7 Ding-Yi Chen 2010-01-07 19:07:23 EST
MUST:
+  rpmlint output is acceptable.
+  Package meets naming and packaging guidelines.
+  Package meets licensing guidelines, and match the source license.
+  Source files match upstream.
+  specfile is properly named, is cleanly written
+  Spec file is written in American English.
+  Spec file is legible.
+  dist tag is present.
+  BuildRoot is proper.
+  BuildRequires are proper.
+  Requires are proper.
+  %install starts with rm -rf %{buildroot} (or $RPM_BUILD_ROOT)
+  %clean contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT)
+  %doc files present.
+  %doc files do not interfere runtime application.
+  Macros are consistently used.
+  Package builds in koji.
+  Package contains code or permissible content.
+  Package installed properly.
+  No system library is bundled.
+  Not relocatable, unless proper justification is presented.
+  %files section must include a %defattr(...) line, and file permissions are correct.
+  No duplication in %files
+  File names are in valid UTF-8.
+  Own all directory it creates.
+  Files or directories are not owned by other packages.
+  .so goes in a -devel package if .so.X exists.
+  No .la libtool archives exists.

SHOULD:
+  License text are in separate files.
+  Translations for supported non-English languages if available.
+  Package build in mock.
+  Package can build in all supported architectures.
+  Package runs properly.
+  Scriptlets are sane.
+  Subpackages Requires: %{name} = %{version}-%{release}
+  No direct files dependencie, unless they are in either /etc, /bin, /sbin, /usr/bin, or /usr/sbin

APPROVED
Comment 8 Michael Schwendt 2010-01-08 18:06:36 EST
New Package CVS Request
=======================
Package Name: gstreamer-plugins-fc
Short Description: Future Composer input plugin for GStreamer
Owners: mschwendt
Branches: F-11 F-12
InitialCC:
Comment 9 Kevin Fenzi 2010-01-08 23:31:19 EST
cvs done.
Comment 10 Michael Schwendt 2010-01-09 10:13:13 EST
Thanks for the review!

Built for Rawhide
and F-12, F-11:
https://admin.fedoraproject.org/updates/gstreamer-plugins-fc

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