[Bug 199029] Review Request: jokosher

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Review Request: jokosher


https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=199029





------- Additional Comments From snecklifter at gmail.com  2007-03-16 19:58 EST -------
Hello Peter,

(In reply to comment #69)
> I can't sponsor you, but here's a brief unofficial review of your spec/SRPM to
> help expedite getting this into Fedora because it's such an awesome little app. :]

I'm glad you agree - its been a long time coming :P

> Unfortunately, Mock is also quite broken at the moment, so I cannot post
> comments about its build-time structure or quality. (However, it seems to run
> okay through a standard rpmbuild invocation; and rpmlint is silent on that built
> RPM.)

Mock is working for me and builds fine with the following changes. rpmlint is quiet.

> BAD: The package does not follow the Naming Guidelines: The Release tag should
> be "0.%{X}.%{alphatag}" or equivalent as noted in the "Pre-Release packages"
> section of the naming guidelines (Packaging/NamingGuidelines on the wiki).

This should now be fixed as I have started bumping the %{X} section with each
new build.

> BAD: You have duplicate BuildRequires listed: python-devel and pycairo-devel are
> both dependencies of pygtk2-devel. Thus, simply listing pygtk2-devel as a
> BuildRequires should pull in the other two automagically; and you should remove
> those two from the BuildRequires list.

Fixed.

> BAD: %{_datadir}/omf/%{name}/, %{_datadir}/gnome/help/%{name}/, and
> %{_datadir}/gnome/help/%{name}/C/ should all be owned by the package. Otherwise
> they are orphaned when the package is removed. We don't like clutter in our
> filesystems. :]

Fixed.

> BAD: The package should invoke desktop-file-install to install the .desktop file
> in your %install section. See the "desktop-file-install usage" section on the
> wiki's Packaging/Guidelines page for more details.

Fixed.

> BAD: Your package does not have an appropriate %clean section. It should contain
> simply "rm -rf $RPM_BUILD_ROOT". See 

Fixed.

> MINOR(?): The source tarball does not match that of upstream (in this case, the
> upstream tarball is generated from the svn export as you explain in your spec
> comments):
>     $ md5sum SOURCES/jokosher-0.9*
>     24ae1fa6adad7893a58fcd32aaec1ff3  jokosher-0.9-srpm.tar.gz
>     26837769e637a9225fb26afe243488db  jokosher-0.9-upstream.tar.gz
> However, if I grab another SVN snapshot and tar it up, the md5sum changes yet
> again, so this seems to be a simple false positive; and is probably safe to
ignore.

Okay, let me know if this becomes a problem. Note I am doing an svn export as
opposed to a svn checkout to lose all the usual svn cruft.
 
> MINOR: It's just preference, but instead of listing the directory via %dir and
> globbing all of the files inside of it, it's probably simpler to simply list the
> directory itself as a parent. RPM will automagically make it grab all the files
> and directories in that recursively. E.g., instead of the current:
>     %dir %{python_sitelib}/Jokosher
>     %{python_sitelib}/Jokosher/*.py
>     %{python_sitelib}/Jokosher/*.pyo
>     %{python_sitelib}/Jokosher/*.pyc
> You could use something like:
>     %{python_sitelib}/Jokosher/
> Your %exclude line should still hold, so that's ok.

Fair point. Hopefully it now looks cleaner. 

http://www.iammetal.co.uk/jokosher/

Cheers
Chris

-- 
Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug, or are watching the QA contact.