Bug 497525

Thanks for this submission, I was thinking about packaging this just yesterday.

IMO this package should be named gnome-applet-bublemon to follow the fedora naming conventions, see
http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Addon_Packages_.28General.29
You can add a provides for bubblemon-gnome if you like.

The other bubblemon package from bug # 165483 should be renamed to bubblemon-dockapp because it violates the naming guidlines, but that's different issue.

License: GPLv2 is not correct, this is GPLv2+, because if you look at the sourcecode you will find GPLv2 "or any later version". See 
https://fedoraproject.org/wiki/Packaging/LicensingGuidelines#.22or_later_version.22_licenses

BuildRequires: gettext is not really necessary, will be pulled in by intltool automatically.

Remove "AutoReqProv: yes" & comment, it's a default that should not be mentioned explicitly. Also it wont work in this case:
  $ rpm -qp --requires bubblemon-gnome-2.0.13-2.fc10.i386.rpm | grep panel
  libpanel-applet-2.so.0
  $ rpm -q --whatprovides libpanel-applet-2.so.0
  gnome-panel-libs-2.24.3-2.fc10.i386
But gnome-panel-libs is not enough to actually run the applet, add "Requires: gnome-panel".

Drop ABOUT-NLS again, generic info, not needed when installed from rpm.

%{_mandir}/hu/man1/bubblemon-gnome2.1.gz and
%{_mandir}/sv/man1/bubblemon-gnome2.1.gz are not utf-8, use iconv to convert them, see 
https://fedoraproject.org/wiki/PackageMaintainers/PackagingTricks#Convert_encoding_to_UTF-8

The rest looks good.

Preserve timestamps during install:

make install DESTDIR=%{buildroot} INSTALL="install -p"

See: https://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps

Thank you for your thorough commenting. I have two points where I need some additional clarification.

1) I've included gettext since it does not appear to be really pulled in by intltool, see below. Additionally it is suggested by the packaging guidelines: "If the package includes translations, add BuildRequires: gettext. If you don't, your package could fail to generate translation files in the buildroot." (https://fedoraproject.org/wiki/Packaging/Guidelines#Handling_Locale_Files).

$ rpm -q gettext
gettext-0.17-8.fc10.i386
$ rpm -q --whatrequires gettext
gettext-devel-0.17-8.fc10.i386
[edwin@localhost ~]$ rpm -q intltool
intltool-0.40.5-1.fc10.i386
[edwin@localhost ~]$ rpm -q --requires intltool
/bin/sh  
/usr/bin/perl  
automake  
gettext-devel  
patch  
perl(Cwd)  
perl(File::Basename)  
perl(File::Copy)  
perl(File::Find)  
perl(Getopt::Long)  
perl(Text::Wrap)  
perl(XML::Parser)  
perl(strict)  
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rpmlib(VersionedDependencies) <= 3.0.3-1

-> Do you still feel that gettext is not required? In that case I would like to suggest a change in the Packaging Guidlines Wiki.

2) The conversion to UTF-8 has been taken care of by the upstream maintainer during my creation of the rpm spec file since I've found that warning too. Since it was a 'warning' with a a 'consider' considering the two translated manpages, I figured this would solve itself in the next upstream release.

W: file-not-utf8 /usr/share/man/sv/man1/bubblemon-gnome2.1.gz
The character encoding of this file is not UTF-8. Consider converting it in
the specfile's %prep section for example using iconv(1).

W: file-not-utf8 /usr/share/man/hu/man1/bubblemon-gnome2.1.gz
The character encoding of this file is not UTF-8.  Consider converting it in
the specfile's %prep section for example using iconv(1).

-> Do you think the conversion is a must for this release or a nice to have? It doesn't really bother to include in this release, but I thought it unnecessary since I will be removing it the next release anyway.

(In reply to comment #3)

> [edwin@localhost ~]$ rpm -q --requires intltool
> /bin/sh  
> /usr/bin/perl  
> automake  
> gettext-devel  

And gettext-devel requires gettext. On the other hand it does not harm to mention it, I recall in Fedora > 7 intlttol did not pull in gettext-devel. It's up to you, that's why I said "not *really*".

> -> Do you think the conversion is a must for this release or a nice to have?

It's a nice to have, I wouldn't call it a blocker. I will do the final review tomorrow, stay tuned.

(In reply to comment #4)

I've uploaded the changed files for your final review. (The previous locations were URL's of a different mirror.)

Spec URL: http://savannah.inetbridge.net/bubblemon/fedora-stage/gnome-applet-bubblemon.spec
SRPM URL: http://savannah.inetbridge.net/bubblemon/fedora-stage/gnome-applet-bubblemon-2.0.13-3.fc10.src.rpm

In these files all your remarks are incorporated with the exception of:
- gettext (where I decided to follow the suggestions in the Packaging Guidelines), and
- the conversion of the two manpages (languages sv and hu) since they are already included by the upstream maintainer in the upcoming next release and these files have a small target audience which should have the used character encoding anyway.

(In reply to comment #2)
> Preserve timestamps during install:
> 
> make install DESTDIR=%{buildroot} INSTALL="install -p"
> 
> See: https://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps  

This is a valid comment. I'm only wondering why the generated template spec file does not include the part INSTALL="install -p".

Isn't this something which should be changed in rpmdev-newspec, which I used to generate the 'offending' line? Future newcomers might be making this same mistake again by following the generated spec.

(In reply to comment #5)
> - gettext (where I decided to follow the suggestions in the Packaging
> Guidelines), and

Ok, didn't remember that

> - the conversion of the two manpages (languages sv and hu) since they are
> already included by the upstream maintainer in the upcoming next release and
> these files have a small target audience which should have the used character
> encoding anyway.

Ok, I guess we should trust the translators here. Hope it gets fixed upstream though.

(In reply to comment #6)
> This is a valid comment. I'm only wondering why the generated template spec
> file does not include the part INSTALL="install -p".

Some Makefiles already use the -p option by default, but not this one. Others don't understand the INSTALL parameter and will fail.


REVIEW FOR gnome-applet-bubblemon-2.0.13-3.fc10.src.rpm

OK - MUST: rpmlint must be run on every package. The output should be posted in the review.
OK - MUST: The package is named according to the Package Naming Guidelines.
OK - MUST: The spec file name matches the base package %{name}, in the format %{name}.spec.
OK - MUST: The package meets the Packaging Guidelines.
OK - MUST: The package is licensed with a Fedora approved license and meets the Licensing Guidelines: GPLv2+
OK - MUST: The License field in the package spec file matches the actual license.
OK - MUST: The license file from the source package is included in %doc.
OK - MUST: The spec file is in American English.
OK - MUST: The spec file for the package is legible.
OK - MUST: The sources used to build the package match the upstream source by MD5 0f0e72376c112126f0b0d1487ac7c57c
OK - MUST: The package successfully compiles and builds into binary rpms on all archs http://koji.fedoraproject.org/koji/taskinfo?taskID=1321531
N/A - MUST: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch.
OK - MUST: All build dependencies are listed in BuildRequires.
OK - MUST: The spec file handles locales properly with the %find_lang macro.
N/A - MUST: Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun.
N/A - MUST: If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the rationalization for relocation of that specific package.
OK - MUST: The package owns all directories that it creates. (None)
OK - MUST: The package does not contain any duplicate files in the %files listing.
OK - MUST: Permissions on files are set properly. Every %files section includes a %defattr(...) line.
OK - MUST: The package has a %clean section, which contains rm -rf %{buildroot}.
OK - MUST: The package consistently uses macros, as described in the macros section of Packaging Guidelines.
OK - MUST: The package contains code, or permissable content.
N/A - MUST: Large documentation files should go in a -doc subpackage.
OK - MUST: Files included as %doc do not affect the runtime of the application.
N/A - MUST: Header files must be in a -devel package.
N/A - MUST: Static libraries must be in a -static package.
N/A - MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'.
N/A - MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package.
N/A - MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release}
OK - MUST: The package does not contain any .la libtool archives.
N/A - MUST: The package contains a Gnome panel plugin, whcih needs no desktop-file-install
OK - MUST: The packages does not own files or directories already owned by other packages.
OK - MUST: At the beginning of %install, the package runs rm -rf %{buildroot}.
OK - MUST: All filenames in rpm packages are valid UTF-8.


SHOULD Items:
N/A - SHOULD: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it.
N/A - SHOULD: The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available.
FAIL - SHOULD: The the package builds in mock on F-10, but not F-11 and F-12
OK - SHOULD: The package should compile and build into binary rpms on all supported architectures.
OK - SHOULD: The package functions as described.
N/A - SHOULD: If scriptlets are used, those scriptlets must be sane. This is vague, and left up to the reviewers judgement to determine sanity.
N/A - SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency.
N/A - SHOULD: The placement of pkgconfig(.pc) files depends on their usecase, and this is usually for development purposes, so should be placed in a -devel pkg.
N/A - SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself.


Issues
- mock build for F-11 and F-12 fails, see 
http://koji.fedoraproject.org/koji/taskinfo?taskID=1329436
http://koji.fedoraproject.org/koji/taskinfo?taskID=1329461

- SourceURL not found, see http://fedoraproject.org/wiki/Packaging/SourceURL#Referencing_Source
We need a downloadable URL in the Source0 tag, use spectool to verify.
- Minor: When renaming the package you forgot the comment at the head of the spec

Regarding your sponsorship: Do you have any other packages or have you participated in other reviews?

(In reply to comment #7)
> > - the conversion of the two manpages (languages sv and hu) since they are
> > already included by the upstream maintainer in the upcoming next release and
> > these files have a small target audience which should have the used character
> > encoding anyway.
> 
> Ok, I guess we should trust the translators here. Hope it gets fixed upstream
> though.

It currently is, the upstream maintainer assured me of this fact.
 
> (In reply to comment #6)
> > This is a valid comment. I'm only wondering why the generated template spec
> > file does not include the part INSTALL="install -p".
> 
> Some Makefiles already use the -p option by default, but not this one. Others
> don't understand the INSTALL parameter and will fail.

I'll see if I can get that fixed upstream as well. Perhaps it's just a question of moving to a more recent automake/autoconf.
 
> REVIEW FOR gnome-applet-bubblemon-2.0.13-3.fc10.src.rpm
> Issues
> - mock build for F-11 and F-12 fails, see 
> http://koji.fedoraproject.org/koji/taskinfo?taskID=1329436
> http://koji.fedoraproject.org/koji/taskinfo?taskID=1329461

The build.log is pretty clear and fortunately identical in F11 and F12. Strange that it does not happen under F10 or earlier (I used F9 for an earlier version, and F10 for this submission). I will check myself as well as with the upstream maintainer what happened.
 
> - SourceURL not found, see
> http://fedoraproject.org/wiki/Packaging/SourceURL#Referencing_Source
> We need a downloadable URL in the Source0 tag, use spectool to verify.

Sorry, that's my mistake. The download page on upstream's project page embeds the source directory which is on another server. It should read:
http://savannah.inetbridge.net/bubblemon/bubblemon-%{version}.tar.gz

> - Minor: When renaming the package you forgot the comment at the head of the
> spec

I see, consider it corrected.


Because of the upcoming release of F11, I suggest not to incorporate this package until the build issues for F11 and F12 have been resolved. I assume you agree with me on this.

I will incorporate the above issues and revert as soon as possible. That will probably be with a new upstream release to fix the build issues.

(In reply to comment #7)
> Regarding your sponsorship: Do you have any other packages or have you
> participated in other reviews?  

Just yesterday I have submitted another very small package for review (see bug 498218), which has not had a reviewer yet.

I have some plans for submitting yet another package (log4cpp, http://sourceforge.net/project/showfiles.php?group_id=15190), after playing around with it a bit more. From the concept it is an interesting package, however does not seem to be very actively maintained at the moment judging from the bug, patch and feature trackers on sourceforge. I'm therefore not sure if this would make a good candidate at the moment.

I have not yet participated in any reviews yet. My main activities on Bugzilla so far have been bug reporting (sometimes signalling dupes when I stumbled upon them) and commenting on other bugs which were reported by others before me.

I have been actively using RedHat Linux around 1996-2002, and returned to Fedora again about a year ago. I therefore do not possess much recent experience of Fedora packaging and its guidelines, but I am familiar with rpm.

Your sponsorship would be very much appreciated.

(In reply to comment #8)
> Because of the upcoming release of F11, I suggest not to incorporate this
> package until the build issues for F11 and F12 have been resolved. I assume you
> agree with me on this.

Ok, there are some weired things going on with libtool, your package probably is not the only one that fails here.

> I will incorporate the above issues and revert as soon as possible. That will
> probably be with a new upstream release to fix the build issues.  

Ok, feel free when you are ready.

The build errors under F11 and F12 have been fixed. The pkgconfig data of one of the Gnome packages has changed which resulted in a missing include directive, which was not generated by configure. All dependencies have been checked, but it turned out this was the only one.

Additionally, the man pages which were not in UTF-8 have been converted to UTF-8 upstream.

Current status:
- The package is released upstream with a new release number (2.0.14).
- All earlier review suggestions have been included (except for the gettext suggestion where the package follows the recommendations from the Packaging Guidelines).
- rpmlint reports no errors, no warnings.
- The package builds cleanly on F10, F11 and F12 via a Koji scratch build.

Spec URL: http://savannah.inetbridge.net/bubblemon/fedora-stage/gnome-applet-bubblemon.spec
SRPM URL: http://savannah.inetbridge.net/bubblemon/fedora-stage/gnome-applet-bubblemon-2.0.14-1.fc10.src.rpm

I would like to request a final review.

Thanks Edwin, here we go:

The new package

fixes all outstanding issues:
OK - rpmlint is silent
OK - builds in mock on rawhide as well as F-11
OK - all files are UTF-8

and therefore the package is APPROVED.

One last thing: I suggest to also require gnome-system-monitor, since it is in the right click menu. But you can change this after import into CVS. I have just sponsored you.

New Package CVS Request
=======================
Package Name: gnome-applet-bubblemon
Short Description: Bubbling Load Monitoring Applet for the GNOME Panel
Owners: edwintb
Branches: F-11
InitialCC:

cvs done.

Removing NEEDSPONSOR.

(In reply to comment #12)
> One last thing: I suggest to also require gnome-system-monitor, since it is in
> the right click menu. But you can change this after import into CVS. I have
> just sponsored you.  

Christoph, I'm not entirely sure where you found that, but on my system the right-click menu has only the following options:
- About
- Remove From Panel
- Move
- Lock To Panel

The package is currently imported into CVS (F-11 and devel) but not yet built pending this question.

Sorry, my bad, that was another applet. You can start your builds now.

The package is built successfully for devel and F-11 and has been included in comps.xml.

Closing this ticket.