First time submitted package?

This is my first time, and I'm looking for a sponsor.

Adding the FE-NEEDSPONSOR tracker.

Some initial comments on you specfile:

- please don't use %define rel and ver.

- don't repeat the name of the package in Summary, just use "A docklet plugin
for Audacious".

- the release tag is wrong, should be "1%{?dist}", which will result in 
audacious-docklet-0.1.1-1.fc6.src.rpm. Please read
http://fedoraproject.org/wiki/Packaging/NamingGuidelines#head-5ea39bbc33cf351b41b51325ac3527eff4c58dac
and 
http://fedoraproject.org/wiki/Packaging/NamingGuidelines#head-beca3bf84972f19a384cc2e5091ed47c2b3cebc7

- BuildRoot should be
"%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)", see 
http://fedoraproject.org/wiki/Packaging/Guidelines#head-f196e7b2477c2f5dd97ef64e8eacddfb517f1aa1

- no BuildRequires are certainly not correct

- %description should be more elaborate while it's only the summary again. You
should describe the features of the program a little.

- remove the link to the website from %description, rpm has it's own tag "URL"
for that, which is missing in your specfile.

- linking to http://nedudu.hu/?page_id=11 is a bad idea, for it's not a
permanent link. In a feew weeks this will be on page 12 and so on. The permalink
for this entry is http://nedudu.hu/index.php?entry=entry060828-105151, but I
suggest you use http://nedudu.hu/static.php?page=audacious instead.

So you would insert
"URL: http://nedudu.hu/static.php?page=audacious" somewhere, e. g. below License:

- simplyfy the %clean section to "rm -rf $RPM_BUILD_ROOT"

- you should clean the built-root at the beginning of %install, too:
  %install
  rm -rf $RPM_BUILD_ROOT
  make DESTDIR=$RPM_BUILD_ROOT install

- %defattr should be (-,root,root,-)

- remove INSTALL and ABOUT-NLS from %doc, not needed if the programm is
installed via rpm

- the %files is not ok, simply using %{_datadir} will result in directories
which are owned by multiple packages.

- locales need to be handled with %find_lang, see
http://fedoraproject.org/wiki/Packaging/Guidelines#head-8c605ebf8330f6d505f384e671986fa99a8f72ee

Thank you so much for your carefully review. This is my first time...

I update the spec file as required.
Spec URL: <http://yufanyufan.googlepages.com/audacious-docklet.spec>
SRPM URL: <http://yufanyufan.googlepages.com/audacious-docklet-0.1.1.src.rpm>
Description: <A plugin that allow you to minimize and control Audaciou as a
system tray icon.>

However, After I change Release tag to "1%{?dist}", there's no "fc6" in the
src.rpm. Is that correct?
  
Thank you again!

(In reply to comment #4)
> Thank you so much for your carefully review. This is my first time...

No problem, you are welcome.

> I update the spec file as required.
> Spec URL: <http://yufanyufan.googlepages.com/audacious-docklet.spec>
> SRPM URL: <http://yufanyufan.googlepages.com/audacious-docklet-0.1.1.src.rpm>

I'm getting a 404 error on these.

> However, After I change Release tag to "1%{?dist}", there's no "fc6" in the
> src.rpm. Is that correct?

Yes, but the buildsys will resolve it to fc5/6/7. You could add something like

 %distname fc
 %distversion %(rpm -qf --qf='%{VERSION}' /etc/redhat-release)
 %dist    .%{distname}%{distversion}

to your ~/.rpmmacros to also have the disttag for your local builds.

fixed URL:
Spec URL: <http://yufanyufan.googlepages.com/audacious-docklet.spec>
SRPM URL: <http://yufanyufan.googlepages.com/audacious-docklet-0.1.1-1.src.rpm>

I hope everything goes right this time!
 

Some more things I'd like you to fix:

- BuildRequires: are incorrect. Instead of glib-devel you mean glib2-devel.
audacious-devel already requires glib2-devel and gtk2-devel, so there's no need
to list them (twice).
On the other hand you need gettext for %find_lang (see
http://fedoraproject.org/wiki/Packaging/Guidelines#head-8c605ebf8330f6d505f384e671986fa99a8f72ee)
and maybe perl(XML::Parser) for generating the locales.

- better change Requires: audacious to audacious-plugins, because that packgage
owns /usr/lib/audacious/(General).

- there are two typos in %description, also I think that you don't need to
mention the minimize feature, "control Audacious" is IMO enought. How about:
"A plugin that allows you to control Audacious from the system tray."? If your
description is longer, don't forget to add a line warp after 79 characters.

- add "--disable-static" to %configure, because we don't want no static libs,
see
http://fedoraproject.org/wiki/Packaging/Guidelines#head-2302ec1e1f44202c9cc4bcce24cb711266557ad7libdocklet.a


- we also don't want no libtool archives, so remove libdocklet.la after install:
 %find_lang %{name}
 rm $RPM_BUILD_ROOT/%{plugin_dir}/General/libdocklet.la

- audacious-docklet-0.1.1.tar.bz2 inside of the srpm has permissions of 555,
please change it to 644.

Once again, all fixed. Sorry for my mistakes.

Spec URL: <http://yufanyufan.googlepages.com/audacious-docklet.spec>
SRPM URL: <http://yufanyufan.googlepages.com/audacious-docklet-0.1.1-1.src.rpm>

Please check it out! Thank you!

(In reply to comment #8)
> Sorry for my mistakes.

Once again: There's no need for an excuse. You are new to Fedora Extras, but you
are doing fine. :)

REVIEW for 
887e3df8edb6a71276a6c1abb3be1bcb  audacious-docklet-0.1.1-1.src.rpm

MUST Items:
FAIL - rpmlint is not happy with the package:
  rpmlint audacious-docklet-0.1.1-1.fc6.i386.rpm 
  W: audacious-docklet no-version-in-last-changelog
  E: audacious-docklet zero-length /usr/share/doc/audacious-docklet-0.1.1/README

To fix these:
- Add the version to the changelog entry:
  * Thu Dec 14 2006 Yu Fan <yufanyufan at gmail dot com> - 0.1.1-1
- Remove README from doc as long as it's empty.

OK - package meets naming guidelines
OK - spec file meets naming guidelines
OK - package meets package guidelines
OK - license open-source compatible (GPL)
OK - license in spec file matches actual license
OK - license included in %doc
OK - spec file in American English
OK - spec file is legible

FAIL - source in SRPM doesn't match upstream source, md5 is
  7503981a0a0ee229e5bdbe18553810db while upstream is
  9b51ac5fd179ede8d0d75be12f920ed2
Please download a new source tarball for the SRPM.

OK - package builds on i386
OK - all build dependencies listed in BuildRequires
OK - none of the exceptions of packaging guidelines in BuildRequires
OK - locales handled correctly with %find_lang
OK - no shared libs to worry about
OK - package is not relocatable
OK - package owns all directories that it creates
OK - no duplicate files in %files section
OK - permissions and %defattr correct 
OK - clean section with "rm -rf $RPM_BUILD_ROOT" present
OK - macro usage consistent
OK - code, not content
OK - no large docs
OK - docs don't affect runtime
OK - no headers, static libs or pkgconfig files, no -devel package needed
OK - no libtool archives
OK - plugin, no need for a *.desktop file.
OK - package doesn't own files/directories owned by other packages

SHOULD items:
OK - package builds in mock (Core 6 and devel on i386)
OK - package functions as described (but see note below)
OK - package uses disttag

NEEDSWORK

Please fix the issues I mentioned before I can approve this package.

Also I found audacious ui freezing when I choose "Preferences" from the docklet
right after the start. This only happens when the prefes window is showing
"Appearence" and if it has net been opened before in the audacious session. If I
opened the prefs before from the audaciuos main window or if I selected
something else but "Appearance", everything works fine.

Do you see the same behaviour? If so:
- Please notify upstream. Not sure if this is an audacious or a docklet bug.
- Do not build this package for branches other than devel. I'd like to see this
fixed before the package enters FE 5/6.

All the issues you mentioned above are fixed, except the md5 problem.
The md5 of up stream source
<http://nedudu.hu/downloads/audacious-docklet-0.1.1.tar.bz2> is 
7503981a0a0ee229e5bdbe18553810db, which match exactly the one inside src.rpm.

The freezing preferece window bug does exist. A review of the source code
suggests that it is a bug inside audacious, but not the plugin. To be a quick
fix, I create a patch that removed the preference menu entry and related stuff
from the docklet plugin. I will try to notify the upstreams.

The SPEC and src.rpm
Spec URL: <http://yufanyufan.googlepages.com/audacious-docklet.spec>
SRPM URL: <http://yufanyufan.googlepages.com/audacious-docklet-0.1.1-1.src.rpm>

(In reply to comment #10)
> All the issues you mentioned above are fixed, except the md5 problem.

Sorry, my fault.

> To be a quick
> fix, I create a patch that removed the preference menu entry and related stuff
> from the docklet plugin.

Sounds good and the patch looks sane. 

As all outstanding issues are fixed now, this package is APPROVED for all
versions of Fedora Extras (audaciuos is only in > 6).

You are now at step 9 of the new contributor's process, see
http://fedoraproject.org/wiki/Extras/Contributors#head-bb3314e7b80fd98f037edd46f6d1efafbb611752

I'm also willing to sponsor you, but before I do I'd like you to get more
familiar with Fedora Extras, especially with 
http://fedoraproject.org/wiki/Extras/HowToGetSponsored and
http://fedoraproject.org/wiki/Extras/UsingCvsFaq

Are you planing to release more packages?

Thank you for you supporting all the way!
It's a great honor to be a contributor.
By now, I indeed has another package to release. Hopefully, all I
learned here will save me a lot of time in the future.
Please come by and see by the time!

I rename audacious-docklet to audacious-plugins-docklet as proposed.
What should I do with cvs and build-sys now?

new spec file is:
%define plugin_dir %(pkg-config audacious --variable=plugin_dir)

Name: audacious-plugins-docklet
Version: 0.1.1
Release: 3%{?dist}
Summary: A docklet plugin for Audacious

License: GPL
Group: Applications/Multimedia
URL: http://nedudu.hu/static.php?page=audacious
Source0: http://nedudu.hu/downloads/audacious-docklet-0.1.1.tar.bz2
Patch0: audacious-plugins-docklet-0.1.1-prefwindow.patch
BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)


BuildRequires: audacious-devel, gettext, perl(XML::Parser)
Requires: audacious-plugins

%description
A plugin that allows you to control Audacious from the system tray.

%prep
%setup -q -n audacious-docklet-%{version}
%patch0 -p1 -b .prefwindow

%build
%configure --disable-static
make %{?_smp_mflags}

%install
rm -rf $RPM_BUILD_ROOT
make DESTDIR=$RPM_BUILD_ROOT install
%find_lang audacious-docklet
rm -f $RPM_BUILD_ROOT/%{plugin_dir}/General/libdocklet.la

%clean
rm -rf $RPM_BUILD_ROOT

%files -f audacious-docklet.lang
%defattr(-,root,root,-)
%doc AUTHORS COPYING NEWS ChangeLog
%{plugin_dir}/General/libdocklet.so

%Changelog
* Thu Jun 7 2007 Yu Fan <yufanyufan> 0.1-3
- Rename audacious-docklet to audacious-plugins-docklet.

* Tue Apr 10 2007 Kevin Fenzi <kevin> - 0.1.1-2
- Rebuild for new audacious version. 

* Thu Dec 14 2006 Yu Fan <yufanyufan> - 0.1.1-1
- Modify spec file to conform fedora extras packaging and naming convention

* Mon Apr  9 2006 Jiang Tao <jiangtao9999>
- Modify spec file from audacious-mac.spec to audacious-docklet plugin.

(In reply to comment #13)
> I rename audacious-docklet to audacious-plugins-docklet as proposed.
> What should I do with cvs and build-sys now?
> 
> new spec file is:

The spec looks good to me, but you should have used an attachment for this.
There is a typo in the last changelog entry. Version needs to be 0.1.1-3 instead
of 0.1-3.

To rename the module in cvs you need to follow the cvs admin procedure as
described in 
http://fedoraproject.org/wiki/PackageMaintainers/CVSAdminProcedure

Package Change Request
======================
Updated Package Name: audacious-plugins-docklet
Updated Fedora CC: cwickert

Yu: Can you add an cvs request to do the rename? Then we can add cwickert at the
same time. What branches do you want to rename? Do you want to keep cvs history
or start over new?

I will set cvs to - now and you can reset it to ? when you are ready. 

(In reply to comment #15)
> What branches do you want to rename?

No branches but the whole module. For the reasons please see the
audacious-plugin-itouch review at
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=218256

BTW: IMO it should be audacious-plugin-docklet (singular instead of plural),
just like Michael said in bug #222648 comment #3

@Yu: I just realized your spec is missing Obsoletes: and Provides: as described in
http://fedoraproject.org/wiki/Packaging/NamingGuidelines#head-3cfc1ea19d28975faad9d56f70a6ae55661d3c3d
Without these tags people won't be able to upgrade without removing the old
package first.

Created attachment 156534 [details]
new spec file for audacious-plugins-docklet

Adding Obsoletes: audacious-docklet < 0.1.1-2 s  statement

Package Change Request
======================
Package Name: audacious-plugins-docklet
New Branches: FC6 F7
Updated Description: Rename audacious-docklet to audacious-plugins-docklet as
discussed in this thread.

module has been renamed and should be usable within 18 minutes of this post. 
Please let me know if you run into any trouble.

Audacious upstream hereby requests that you do not include this plugin because:

(a) we ship our own statusicon plugin in 1.3+
(b) this plugin is known to cause state corruption in GTK and Xlib
(c) this plugin is known to use our (Audacious) APIs improperly

Unfortunately I have to say that I agree with William. Since audacious now has
an own docklet, there is no need to duplicate this functionality. So I suggest
to drop this package and remove it from the repos. This also means that
audacious needs to obsolete audacious-docklet.

Yu: Is this ok for you? This doesn't mean that you "loose" your only package,
you still have audacious-itouch and I'm pretty sure you can contribute other
packages, too.

William: Are there any issuses from upstream's point of view with
audacious-itouch  we should know about?

It's fine with me as long as the audacious built-in docklet is working.

The audacious-itouch is replaced by audacious-itouch-control.
http://sourceforge.net/projects/itouch-control

(In reply to comment #22)
> The audacious-itouch is replaced by audacious-itouch-control.
> http://sourceforge.net/projects/itouch-control

Then please update bug #218256 and I will do the final review.

(In reply to comment #19)
> module has been renamed and should be usable within 18 minutes of this post. 
> Please let me know if you run into any trouble.

Warren, I can see the module but I cannot check it out:
cvs co audacious-plugins-docklet
cvs [checkout aborted]: there is no repository /cvs/pkgs/rpms/audacious-docklet

Apparently CVS still thinks the module is named audacious-docklet.

cvs fixed I think. It wasn't entirely changed in the modules file. 
Try now.