Bug 218258
Summary: | Review Request: audacious-docklet - a docklet plugin for Audacious | ||||||
---|---|---|---|---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Yu Fan <yufanyufan> | ||||
Component: | Package Review | Assignee: | Christoph Wickert <christoph.wickert> | ||||
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> | ||||
Severity: | medium | Docs Contact: | |||||
Priority: | medium | ||||||
Version: | rawhide | CC: | christoph.wickert | ||||
Target Milestone: | --- | Flags: | kevin:
fedora-cvs+
|
||||
Target Release: | --- | ||||||
Hardware: | All | ||||||
OS: | Linux | ||||||
Whiteboard: | |||||||
Fixed In Version: | Doc Type: | Bug Fix | |||||
Doc Text: | Story Points: | --- | |||||
Clone Of: | Environment: | ||||||
Last Closed: | 2006-12-25 00:55:07 UTC | Type: | --- | ||||
Regression: | --- | Mount Type: | --- | ||||
Documentation: | --- | CRM: | |||||
Verified Versions: | Category: | --- | |||||
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||
Cloudforms Team: | --- | Target Upstream Version: | |||||
Embargoed: | |||||||
Bug Depends On: | |||||||
Bug Blocks: | 163779 | ||||||
Attachments: |
|
Description
Yu Fan
2006-12-04 07:57:14 UTC
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. |