Bug 506833 - Review Request: bisho - Moblin web services settings
Summary: Review Request: bisho - Moblin web services settings
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Christoph Wickert
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 506476 506486 506780
Blocks: FedoraMoblin20
TreeView+ depends on / blocked
 
Reported: 2009-06-18 19:58 UTC by Peter Robinson
Modified: 2009-08-11 13:35 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-08-11 13:35:24 UTC
Type: ---
Embargoed:
christoph.wickert: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Peter Robinson 2009-06-18 19:58:22 UTC
SPEC: http://pbrobinson.fedorapeople.org/bisho.spec
SRPM: http://pbrobinson.fedorapeople.org/bisho-0.10.2-1.fc11.src.rpm

Moblin web services settings

Comment 1 Jeff Garzik 2009-07-19 09:59:06 UTC
The description "Moblin web services settings" seems to imply to me a dataset of some sort, not an application.

A better description, IMO, would indicate that this is an application.

The specfile otherwise seems fine, and rpmlint doesn't seem to spew.

Comment 2 Peter Robinson 2009-07-24 20:35:11 UTC
I've updated the description as its a moblin application to configure the web services accounts. A new upstream release as well.

SPEC: as above
SRPM: http://pbrobinson.fedorapeople.org/bisho-0.10.7-1.fc11.src.rpm

Comment 3 Christoph Wickert 2009-08-06 22:34:30 UTC
The spec at http://pbrobinson.fedorapeople.org/bisho.spec does not match the one in the package. mux-devel is missing.

mux-devel is also missing from the rawhide repo. Looking at http://koji.fedoraproject.org/koji/packageinfo?packageID=8679 it looks a little messed up: Why is the package locked? And why did you build it for F9???

Anyway, starting the
REVIEW for 6cfb7ae0d1ea8fcd504ff89f50a32079  bisho-0.10.2-1.fc11.src.rpm


TBD - MUST: rpmlint must be run on every package. The output should be posted in the review.
OK - MUST: Named according to the Package Naming Guidelines
OK - MUST: Spec file name matches the base package %{name}
OK - MUST: package meets the Packaging Guidelines
OK - MUST: Fedora approved license and meets the Licensing Guidelines: GPLv2+
FAIL - MUST: License field in spec file doesn't match the actual license: It's GPLv2+, not GPLv2. Looking at the source you will find "... or any later version"
OK - MUST: License file included in %doc
OK - MUST: Spec is in American English
OK - MUST: Spec is legible
OK - MUST: Sources match the upstream source by MD5 f0b354455eabc021045920123198fcd9
OK - MUST: Successfully compiles and builds into binary rpms on x86_64
OK - MUST: No ExcludeArch
OK - MUST: All build dependencies are listed in BuildRequires, but you are listing a couple of redundant deps that get already pulled in by other packages: 
OK - MUST: Handles locales properly with %find_lang
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.
OK - MUST: Not designed to be relocatable
OK - MUST: Owns all directories that it creates
OK - MUST: No duplicate files in the %files listing
OK - MUST: Permissions on files set properly, includes %defattr(...)
OK - MUST: Package has a %clean section, which contains rm -rf %{buildroot}.
OK - MUST: Consistently uses macros
OK - MUST: Package contains code, or permissable content
OK - MUST: No large docs
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.
FAIL - MUST: The package contains a GUI application and includes a %{name}.desktop file, but that file is not properly installed with desktop-file-install in the %install section.
OK - MUST: 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 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.
TBD - SHOULD: The the package doesn't build in mock because mux is not available.
TBD - SHOULD: The package should compile and build into binary rpms on all supported architectures.
TBD - SHOULD: The package functions as described.
FAIL - SHOULD: Scriptlets are not sane. You are running gtk-update-icon-cache only in %post.
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.


Other items:
OK - Latest stable version packaged
OK - RPM_OPT_FLAGS honored


Issues:
- Fix the license tag
- Drop 'Requires(post): /bin/touch', explained in bug 507480
- Drop the redundant BuildRequires, it's no use listing them: glib2-devel, pkgconfig are pulled in be nearly every devel package, autoconf and automake are required by libtool.
- The comment "Require these because ..." is misleading. gnome-common is (likely) needed and gettext/intltool are needed because of the locales. So all that is actually required to run autogen.sh is libtool. Please change the comment to reflect this.
- Timestamps are nor preserved during %install, add "INSTALL='install -p'" to make install, see https://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps
- Update icon-cache scriptlet with latest version from https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#Icon_Cache
- What libtool archives are you trying to remove? There are none!
- AUTHORS and TODO are missing from %doc. Don't add NEWS and README (empty) or ChangeLog (not useful)
- Use desktop-file-install or desktop-file-validate for the desktop file, see
https://fedoraproject.org/wiki/Packaging/Guidelines#desktop-file-install_usage


Please fix the issues and contact rel-eng in order to get mux into the repo. Once this is done, I will finish this review.

Comment 4 Peter Robinson 2009-08-06 22:49:06 UTC
(In reply to comment #3)
> The spec at http://pbrobinson.fedorapeople.org/bisho.spec does not match the
> one in the package. mux-devel is missing.
> 
> mux-devel is also missing from the rawhide repo. Looking at
> http://koji.fedoraproject.org/koji/packageinfo?packageID=8679 it looks a little
> messed up: Why is the package locked? And why did you build it for F9???

It seems I uploaded a new package and didn't update the bug. I never built it for F-9. There's 2 builds listed, both for F-12. I think the F-9 tag is to do with the way the tag inheritance works. Anyway, mux is obsolete (yes, already, welcome to moblin) and has been merged into nbtk.

Matching srpm that goes with the spec file here:

SRPM: http://pbrobinson.fedorapeople.org/bisho-0.10.7-1.fc11.src.rpm

Comment 5 Peter Robinson 2009-08-06 23:12:34 UTC
I'd already fixed most of these locally yesterday based on the other reviews but hasn't uploaded it yet.

> Issues:
> - Fix the license tag

Fixed

> - Drop 'Requires(post): /bin/touch', explained in bug 507480

Done

> - Drop the redundant BuildRequires, it's no use listing them: glib2-devel,
> pkgconfig are pulled in be nearly every devel package, autoconf and automake
> are required by libtool.

I don't see what is the major issue, dependencies change over time and it doesn't add build time so it ends up being semantics. I have removed them. 

> - The comment "Require these because ..." is misleading. gnome-common is
> (likely) needed and gettext/intltool are needed because of the locales. So all
> that is actually required to run autogen.sh is libtool. Please change the
> comment to reflect this.

Well none of them would be needed at all if the package was a released package that had "make dist" run, hence the comment. Maybe I haven't documented it properly. gnome-common isn't pulled in automatically hence the reason it was added, nor are gettext/intltool I believe.

> - Timestamps are nor preserved during %install, add "INSTALL='install -p'" to
> make install, see
> https://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps
> - Update icon-cache scriptlet with latest version from
> https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#Icon_Cache

Done.

> - What libtool archives are you trying to remove? There are none!

Copied over from a template, removed.

> - AUTHORS and TODO are missing from %doc. Don't add NEWS and README (empty) or
> ChangeLog (not useful)

Added, they use to be empty :)

> - Use desktop-file-install or desktop-file-validate for the desktop file, see
> https://fedoraproject.org/wiki/Packaging/Guidelines#desktop-file-install_usage

Updated.

> Please fix the issues and contact rel-eng in order to get mux into the repo.
> Once this is done, I will finish this review.  

Not an issue, mux was in rawhide for about 2 weeks and then marked as a dead.package due to being merged into nbtk.

Also fixed up the autoconf.sh so it doesn't run configure twice.

SRPM: http://pbrobinson.fedorapeople.org/bisho-0.10.7-2.fc11.src.rpm
koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=1588011

Comment 6 Christoph Wickert 2009-08-06 23:41:53 UTC
(In reply to comment #4)
> It seems I uploaded a new package and didn't update the bug. 

Peter, you have posted two different packages with the same version: 0.10.7-1. The one from comment #2 had a md5sum of 6cfb7ae0d1ea8fcd504ff89f50a32079, the one from comment comment #4 had 438f7278066fca02589fe16d26b7a80d. Please don't do that. Whenever you update the package, increase the release to avoid confusion.

(In reply to comment #5)
> > - Drop the redundant BuildRequires, it's no use listing them: glib2-devel,
> > pkgconfig are pulled in be nearly every devel package, autoconf and automake
> > are required by libtool.
> 
> I don't see what is the major issue, dependencies change over time and it
> doesn't add build time so it ends up being semantics. I have removed them. 

Ask yourself: What is the benefit of listing them as long as they are not versioned? None, so remove them.

> > - The comment "Require these because ..." is misleading. gnome-common is
> > (likely) needed and gettext/intltool are needed because of the locales. So all
> > that is actually required to run autogen.sh is libtool. Please change the
> > comment to reflect this.
> 
> Well none of them would be needed at all if the package was a released package
> that had "make dist" run, hence the comment. 

intltool is needed to generate the locales, gettext is needed for find_lang.sh. And I'm pretty sure that gnome-common is also not needed, so IMO the comment is still misleading.

> > - AUTHORS and TODO are missing from %doc. Don't add NEWS and README (empty) or
> > ChangeLog (not useful)
>
> Added, they use to be empty :)

No they were not, even in 0.10.2 ;)

> Not an issue, mux was in rawhide for about 2 weeks and then marked as a
> dead.package due to being merged into nbtk.

Then please make notice of this change in the bug to save the reviewer a lot of time and headache.

> Also fixed up the autoconf.sh so it doesn't run configure twice.

Fine.

> SRPM: http://pbrobinson.fedorapeople.org/bisho-0.10.7-2.fc11.src.rpm
> koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=1588011  

OK, let me take a look at it. Stay tuned.

Comment 7 Peter Robinson 2009-08-07 00:20:14 UTC
(In reply to comment #6)
> (In reply to comment #4)
> > It seems I uploaded a new package and didn't update the bug. 
> 
> Peter, you have posted two different packages with the same version: 0.10.7-1.
> The one from comment #2 had a md5sum of 6cfb7ae0d1ea8fcd504ff89f50a32079, the
> one from comment comment #4 had 438f7278066fca02589fe16d26b7a80d. Please don't
> do that. Whenever you update the package, increase the release to avoid
> confusion.

The comment I made in comment #4 was the same one. I didn't upload a new one there. Check the dates here http://pbrobinson.fedorapeople.org/

> > > - The comment "Require these because ..." is misleading. gnome-common is
> > > (likely) needed and gettext/intltool are needed because of the locales. So all
> > > that is actually required to run autogen.sh is libtool. Please change the
> > > comment to reflect this.
> > 
> > Well none of them would be needed at all if the package was a released package
> > that had "make dist" run, hence the comment. 
> 
> intltool is needed to generate the locales, gettext is needed for find_lang.sh.
> And I'm pretty sure that gnome-common is also not needed, so IMO the comment is
> still misleading.

I've needed it for gtk based apps in the past, that's why I added it. I will investigate it tomorrow.
 
> > > - AUTHORS and TODO are missing from %doc. Don't add NEWS and README (empty) or
> > > ChangeLog (not useful)
> >
> > Added, they use to be empty :)
> 
> No they were not, even in 0.10.2 ;)

Maybe they were empty in one of the other many packages.

> > Not an issue, mux was in rawhide for about 2 weeks and then marked as a
> > dead.package due to being merged into nbtk.
> 
> Then please make notice of this change in the bug to save the reviewer a lot of
> time and headache.

The new package in comment #2 had it removed, sorry I assumed the latest package would be used.

> > Also fixed up the autoconf.sh so it doesn't run configure twice.
> 
> Fine.
> 
> > SRPM: http://pbrobinson.fedorapeople.org/bisho-0.10.7-2.fc11.src.rpm
> > koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=1588011  
> 
> OK, let me take a look at it. Stay tuned.  

Thanks.

Comment 8 Christoph Wickert 2009-08-07 00:53:03 UTC
(In reply to comment #7)
> 
> The comment I made in comment #4 was the same one. I didn't upload a new one
> there. Check the dates here http://pbrobinson.fedorapeople.org/

Sorry, my bad. I md5sum'ed and reviewed the wrong package (0.10.2) but the correct spec, so I'm not going to start all over again.

> I've needed it for gtk based apps in the past, that's why I added it. I will
> investigate it tomorrow.

gnome-common can be dropped, I tested it. intltool and gettext are needed for the locales, so the only thing left for autogen.sh is libtool. Just move the comment down by three lines and it's correct.

Ok, now for 0.10.7 and the remaining issues:
OK - MUST: $ rpmlint /var/lib/mock/fedora-rawhide-x86_64/result/bisho-*
3 packages and 0 specfiles checked; 0 errors, 0 warnings.
OK - MUST: License field in spec file matches the actual license
OK - MUST: %{name}.desktop properly validated with desktop-file-validate

OK - SHOULD: The package builds in mock
OK - SHOULD: The package should compile and build into binary rpms on all
supported architectures.
OK - SHOULD: The package functions as described. However I noticed a small annoyance: When hovering the close button in the top right corner, I get the gtk-broken icon, although I have moblin-icon-theme-installed. Can you confirm this? If so, I will look into that
OK - SHOULD: Scriptlets are sane.


Issues:
"INSTALL='install -p'" belongs to make install, not to make! Should be 
  make install DESTDIR=%{buildroot} INSTALL='install -p'

The summary still is "Moblin web services settings", you might want to change that to to make Jeff happy.

Please fix the remaining issues. None of these are real blockers, so the package is APPROVED.

Comment 9 Peter Robinson 2009-08-07 10:09:28 UTC
> > I've needed it for gtk based apps in the past, that's why I added it. I will
> > investigate it tomorrow.
> 
> gnome-common can be dropped, I tested it. intltool and gettext are needed for
> the locales, so the only thing left for autogen.sh is libtool. Just move the
> comment down by three lines and it's correct.

Done

> OK - SHOULD: The package functions as described. However I noticed a small
> annoyance: When hovering the close button in the top right corner, I get the
> gtk-broken icon, although I have moblin-icon-theme-installed. Can you confirm
> this? If so, I will look into that

That might be fixed in the upstream icon theme. I will add it to my check list for the weekend. I'm hoping to have a test spin to play with then.

> Issues:
> "INSTALL='install -p'" belongs to make install, not to make! Should be 
>   make install DESTDIR=%{buildroot} INSTALL='install -p'

Oops. That will teach me to make changes at 1am :) Fixed.

> The summary still is "Moblin web services settings", you might want to change
> that to to make Jeff happy.

How's "Moblin configuration tool for mojito social network aggregator"

> Please fix the remaining issues. None of these are real blockers, so the
> package is APPROVED.  

For reference changes are in spec and here:
http://pbrobinson.fedorapeople.org/bisho-0.10.7-3.fc11.src.rpm

Thanks :-)

Comment 10 Peter Robinson 2009-08-07 10:10:56 UTC
> > The summary still is "Moblin web services settings", you might want to change
> > that to to make Jeff happy.
> 
> How's "Moblin configuration tool for mojito social network aggregator"

Actually I dropped Moblin from the front.

Comment 11 Peter Robinson 2009-08-07 10:11:40 UTC
New Package CVS Request
=======================
Package Name: bisho
Short Description: Configuration tool for mojito social network aggregator
Owners: pbrobinson
Branches: F-11
InitialCC:

Comment 12 Christoph Wickert 2009-08-07 10:26:36 UTC
(In reply to comment #10)
> Actually I dropped Moblin from the front.  

Hmm, I think it's not bad to mention Moblin in the description. The rest looks good.

Comment 13 Peter Robinson 2009-08-07 10:35:52 UTC
New Package CVS Request
=======================
Package Name: bisho
Short Description: Moblin configuration tool for mojito social network aggregator
Owners: pbrobinson
Branches: F-11
InitialCC:

Comment 14 Kevin Fenzi 2009-08-07 20:07:03 UTC
cvs done.

Comment 15 Peter Robinson 2009-08-08 11:47:20 UTC
Built and on its way to rawhide

Comment 16 Peter Robinson 2009-08-11 13:35:24 UTC
Now in rawhide. As always Christoph thanks for the review :-)


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