[Bug 199029] Review Request: jokosher

Tue Mar 13 04:36:47 UTC 2007

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 peter at thecodergeek.com  2007-03-13 00:36 EST -------
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. :]

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.)

== Unofficial Review of jokosher 0.9-1.20070308svn == 
-----
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).

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.

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. :]

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.

BAD: Your package does not have an appropriate %clean section. It should contain
simply "rm -rf $RPM_BUILD_ROOT". See 
-----
GOOD :The spec file is named "%{name}.spec" in accordance with Naming Guidelines.

GOOD: BuildRoot is "%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u}
-n)", following the packaging guidelines. 

GOOD: Included documentation (%doc) is OK; and it seems to run well without
these files installed.

GOOD: Package includes an appropriate .desktop file since it is a graphical
application.

GOOD: Macros are used instead of hardcoded file names, and usage of these macros
(including $RPM_BUILD_ROOT) is consistent throughout the spec file.

GOOD: Locale files are handled correctly, using %find_lang appropriately.

GOOD: Package is not relocatable.

GOOD: Package includes appropriate code and content.

GOOD: Package does not own any system files/directories or any files/directories
that conflict with another package.   

GOOD: Package license (GPL) is OK; and a copy of it is included in the package
as documentation (%doc COPYING). The License field in the spec file properly
reflects this.

GOOD: Spec file is legible and in American English.

GOOD: No duplicates are listed in the %files section; and its %defattr line is good.

GOOD: When installed, the application runs well with no apparent segfaults or
major bugs.

GOOD: Appropriate scriplets for MIME-type, Scrollkeeper, and .desktop files are
used.

GOOD: The %files section has a proper %defattr(-,root,root,-) line.
-----
N/A: No non-ASCII characters are needed, so encoding is OK.
N/A: This is a noarch package, so compiler flags are not needed. 
N/A: No static libraries or RPATH exclusions are needed.
N/A: Package includes no configuration files or data, so %config markings are
not needed.
N/A: Package uses Python setuptools for building; so using `make %{?_smp_flags}`
is not needed.
N/A: Package is not a web application.
N/A: Package is noarch; thus no ExcludeArch/ExclusiveArch tweaking is required.
N/A: Package installs no shared libraries; thus %post/%postun scriplets of
/sbin/ldconfig are not needed.
N/A: No large (neither in size nor in quantity) documentation is included, thus
no -doc subpackage is needed.
N/A: No headers, no pkgconfig files, and no static or unsuffixed shared
libraries are included. Thus, no -devel subpackage is needed.
N/A: Package contains no libtool archives (*.la files) 
N/A: Package contains no %description or Summary translations.
-----
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.

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.

-- 
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.