Bug 566411 (umit)

Builds successfully in mock on Fedora 11 with i386/x86_64 architectures.

Builds successfully in mock on Fedora 13 with i386/x86_64 architectures.

Taking for review.

The stated "License: GPLv2" is wrong. Some of the files appear to be licensed under GPLv2+, and others are LGPLv2+. The license tag should thus read:
License:        GPLv2+ and LGPLv2+

As per https://fedoraproject.org/wiki/Packaging/SourceURL#Sourceforge.net , you should use the following for source URL:
http://downloads.sourceforge.net/umit/umit-%{version}%{prerelease}.tar.gz

The package uses python gtk2 bindings and needs to require pygtk2 to make sure it is installed at runtime.

You've split html documentation into -doc subpackage. I think it would be better to have this in the main package. Help->Help is supposed to open the html pages, but if -doc subpackage is not installed, the user gets an error dialog instead. It'd be better user experience if everything worked out of the box.
Also, anything marked with %doc must not affect the runtime of the application (http://fedoraproject.org/wiki/Packaging/Guidelines#Documentation), so please don't mark the html documentation with %doc.


Various minor issues
--------------------

%python_sitearch macro defined at the top of the file isn't used anywhere.

> %patch0 -p1 -b .setup.py
The patch -b option is for making backups of the original files. Patch0 is a patch to remove two lines from setup.py file. When the patch command runs, it thus first creates a backup file called setup.py.setup.py which doesn't make much sense. I'd suggest to use something like "%patch0 -p1 -b .check-buildroot", so that the backup file would get named as "setup.py.check-buildroot".

I would suggest to not use %{__command} macros in new spec files, as they are
really not needed and only make the spec file harder to read. Instead of
%{__chmod}, %{__rm}, %{__sed}, %{__ln_s}, and %{__install} I'd use just plain chmod, rm, sed, ln -s, and install.
However, %{__python} might be useful and should remain as macro. Also see how %__ macros are used in the official Fedora python packaging guidelines: http://fedoraproject.org/wiki/Packaging:Python
But this is mostly just personal preference, and up to you if you want to
change that.

Hi
Thank you for starting the review.

Spec diff:
--- umit.spec.orig	2010-02-18 12:29:17.000000000 +0200
+++ umit.spec	2010-03-13 20:44:54.432759235 +0200
@@ -1,19 +1,18 @@
 %if ! (0%{?fedora} > 12 || 0%{?rhel} > 5)
 %{!?python_sitelib: %global python_sitelib %(%{__python} -c "from distutils.sysconfig import get_python_lib; print get_python_lib()")}
-%{!?python_sitearch: %global python_sitearch %(%{__python} -c "from distutils.sysconfig import get_python_lib; print get_python_lib(1)")}
 %endif

 %global prerelease RC

 Name:           umit
 Version:        1.0
-Release:        0.1.%{prerelease}%{?dist}
+Release:        0.2.%{prerelease}%{?dist}
 Summary:        Nmap frontend

 Group:          Applications/Internet
-License:        GPLv2
+License:        GPLv2+ and LGPLv2+
 URL:            http://umit.sourceforge.net/
-Source0:        http://downloads.sourceforge.net/project/umit/umit/%{version}%{prerelease}/umit-%{version}%{prerelease}.tar.gz
+Source0:        http://downloads.sourceforge.net/umit/umit-%{version}%{prerelease}.tar.gz
 # http://trac.umitproject.org/ticket/378
 Source1:        umit_48x48.png
 # http://trac.umitproject.org/ticket/378
@@ -29,6 +28,7 @@
 BuildRequires:  python-sphinx

 Requires:       nmap
+Requires:       pygtk2


 %description
@@ -48,19 +48,9 @@
     * Use Umit interface through the Web


-%package doc
-Summary:        Documentation for %{name}
-Group:          Documentation
-Requires:       %{name} = %{version}-%{release}
-
-
-%description doc
-This package contains documentation files for %{name}.
-
-
 %prep
 %setup -q -n %{name}-%{version}%{prerelease}
-%patch0 -p1 -b .setup.py
+%patch0 -p1


 %build
@@ -68,30 +58,30 @@


 %install
-%{__rm} -rf %{buildroot}
+rm -rf %{buildroot}
 %{__python} setup.py install --root %{buildroot}

 # Remove useless files
-%{__rm} %{buildroot}%{_bindir}/uninstall_umit
+rm %{buildroot}%{_bindir}/uninstall_umit

 # Fix permissions
-find %{buildroot} -type f -exec %{__chmod} 644 {} \;
-%{__chmod} 755 %{buildroot}%{_bindir}/*
+find %{buildroot} -type f -exec chmod 644 {} \;
+chmod 755 %{buildroot}%{_bindir}/*

-# Remove interpreter from site-packages
+# Remove a interpreter from the site-packages
 find %{buildroot}%{python_sitelib} -type f -iname "*py" -exec \
-    %{__sed} -i 's/#!\/usr\/bin\/env python//' {} \;
+    sed -i 's/#!\/usr\/bin\/env python//' {} \;

-# Fix file end-of-line encoding
-%{__sed} -i 's/\r//' %{buildroot}%{_docdir}/%{name}/html/_sources/plugins.txt
+# Fix the file end-of-line encoding
+sed -i 's/\r//' %{buildroot}%{_docdir}/%{name}/html/_sources/plugins.txt

-# Install icons
-%{__install} -pm 0644 %{SOURCE1} %{buildroot}%{_datadir}/pixmaps/%{name}
-%{__install} -d %{buildroot}%{_datadir}/icons/hicolor/48x48/apps
-%{__ln_s} ../../../../pixmaps/%{name}/%{name}_48x48.png %{buildroot}%{_datadir}/icons/hicolor/48x48/apps/%{name}.png
+# Install the icons
+install -pm 0644 %{SOURCE1} %{buildroot}%{_datadir}/pixmaps/%{name}
+install -d %{buildroot}%{_datadir}/icons/hicolor/48x48/apps
+ln -s ../../../../pixmaps/%{name}/%{name}_48x48.png %{buildroot}%{_datadir}/icons/hicolor/48x48/apps/%{name}.png

-# Install desktop file
-%{__install} -d %{buildroot}%{_datadir}/applications
+# Install the desktop file
+install -d %{buildroot}%{_datadir}/applications
 desktop-file-install \
     --dir=%{buildroot}%{_datadir}/applications \
     %{SOURCE2}
@@ -100,7 +90,7 @@


 %clean
-%{__rm} -rf %{buildroot}
+rm -rf %{buildroot}


 %post
@@ -121,6 +111,7 @@
 %files -f %{name}.lang
 %defattr(-,root,root,-)
 %doc COPYING COPYING_HIGWIDGETS README
+%doc %{_docdir}/%{name}
 %{_bindir}/%{name}
 %{_bindir}/umit_scheduler.py
 %{_datadir}/applications/%{name}.desktop
@@ -133,12 +124,15 @@
 %{python_sitelib}/%{name}-*.egg-info


-%files doc
-%defattr(-,root,root,-)
-%doc %{_docdir}/%{name}
-
-
 %changelog
+* Sat Mar 13 2010 Nikolay Ulyanitsky <lystor AT lystor.org.ua> - 1.0-0.2.RC
+- Add the pygtk2 to the Requires
+- Fix the license
+- Fix the Source0
+- Remove the unused macro python_sitearch
+- Remove the -doc subpackage
+- Replace generally useful macros by regular commands
+
 * Fri Feb 05 2010 Nikolay Ulyanitsky <lystor AT lystor.org.ua> - 1.0-0.1.RC
 - Initial package build


Spec URL: http://repo.lystor.org.ua/fedora/12/SPECS/umit.spec
SRPM URL: http://repo.lystor.org.ua/fedora/12/SRPMS/umit-1.0-0.2.RC.fc12.src.rpm

The package review process needs reviewers!  If you haven't done any package
reviews recently, please consider doing one.

Fedora review umit-1.0-0.2.RC.fc12.src.rpm 2010-03-13

+ OK
! needs attention

rpmlint output:
umit.src: W: spelling-error Summary(en_US) Nmap -> Map, N map, Nap
umit.src: W: spelling-error Summary(en_US) frontend -> fronted, front end, front-end
umit.noarch: W: spelling-error Summary(en_US) Nmap -> Map, N map, Nap
umit.noarch: W: spelling-error Summary(en_US) frontend -> fronted, front end, front-end
2 packages and 1 specfiles checked; 0 errors, 4 warnings.

! rpmlint warning about "frontend" is correct and it should be changed to "front-end".
The other spelling error is just harmless noise.

+ The package is named according to the Package Naming Guidelines.
+ Spec file name matches the base package name
+ The package follows the Packaging Guidelines
+ The package is licensed with a Fedora approved license and meets the Licensing Guidelines.
+ The license field in the spec file matches the actual license
+ The package contains the license files (COPYING, COPYING_HIGWIDGETS)
+ Spec file is written in American English
+ Spec file is legible
+ Upstream sources match sources in the srpm. md5sum:
  e096ac3795017ba87be4ed569c520be8  umit-1.0RC.tar.gz
  e096ac3795017ba87be4ed569c520be8  Download/umit-1.0RC.tar.gz
+ The package builds in koji
n/a ExcludeArch bugs filed
+ BuildRequires look sane
+ The spec file handles locales properly
+ Packages does not bundle copies of system libraries
n/a Does not use Prefix: /usr
+ Package owns all directories it creates
+ No duplicate files in %files
+ Permissions are properly set and %files has %defattr
+ %clean contains rm -rf %{buildroot}
+ Consistent use of macros
+ Package contains code or permissible content
n/a Large documentation files should go in -doc subpackage
! Files marked %doc should not affect package
%{_docdir}/%{name} shouldn't be marked as %doc because the content affects package runtime (an error is displayed clicking Help->Help if the files don't exist).

n/a Header files should be in -devel
n/a Static libraries should be in -static
n/a Library files that end in .so must go in a -devel package
n/a -devel must require the fully versioned base
n/a Packages should not contain libtool .la files
+ Packages containing GUI apps must include %{name}.desktop file
+ Packages must not own files or directories owned by other packages
+ Filenames must be valid UTF-8


Please fix frontend -> front-end and remove the %doc before importing it into CVS. Those are minor issues and otherwise the package looks good.

APPROVED

> Please fix frontend -> front-end and remove the %doc before importing it into
> CVS. Those are minor issues and otherwise the package looks good.

Done

Spec URL: http://repo.lystor.org.ua/fedora/12/SPECS/umit.spec
SRPM URL: http://repo.lystor.org.ua/fedora/12/SRPMS/umit-1.0-0.3.RC.fc12.src.rpm

New Package CVS Request
=======================
Package Name: umit
Short Description: Nmap front-end
Owners: lystor
Branches: F-11 F-12 F-13
InitialCC:

CVS done (by process-cvs-requests.py).

umit-1.0-0.3.RC.fc13 has been submitted as an update for Fedora 13.
http://admin.fedoraproject.org/updates/umit-1.0-0.3.RC.fc13

umit-1.0-0.3.RC.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/umit-1.0-0.3.RC.fc12

umit-1.0-0.3.RC.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/umit-1.0-0.3.RC.fc11

umit-1.0-0.3.RC.fc11 has been pushed to the Fedora 11 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update umit'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/umit-1.0-0.3.RC.fc11

umit-1.0-0.3.RC.fc13 has been pushed to the Fedora 13 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update umit'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/umit-1.0-0.3.RC.fc13

umit-1.0-0.3.RC.fc12 has been pushed to the Fedora 12 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update umit'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/umit-1.0-0.3.RC.fc12

umit-1.0-0.3.RC.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

umit-1.0-0.3.RC.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.

umit-1.0-0.3.RC.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.