Bug 698362 - Review Request: writetype - Light word processor
Summary: Review Request: writetype - Light word processor
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Elad Alfassa
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-04-20 17:53 UTC by PRABIN KUMAR DATTA
Modified: 2011-07-08 16:04 UTC (History)
5 users (show)

Fixed In Version: writetype-1.2.130-6.el6
Clone Of:
Environment:
Last Closed: 2011-06-29 21:52:31 UTC
Type: ---
Embargoed:
elad: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description PRABIN KUMAR DATTA 2011-04-20 17:53:17 UTC
Spec URL: http://prabindatta.fedorapeople.org/SPECS/writetype.spec
SRPM URL: http://prabindatta.fedorapeople.org/SRPMS/writetype-1.2.130-1.fc14.src.rpm

Description: 
WriteType is a free  (and open source) program that helps 
younger students experience success in writing. It is designed 
especially for schools to transform technology from a barrier 
into an opportunity for success.

Koji Build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=3014460

Rpmlint Output:-
For RPM file:
$ rpmlint -i RPMS/noarch/writetype-1.2.130-1.fc14.noarch.rpm 
writetype.noarch: W: no-manual-page-for-binary writetype
Each executable in standard binary directories should have a man page.
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

For SRPM file:
$ rpmlint -i SRPMS/writetype-1.2.130-1.fc14.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

For SPEC file:
$ rpmlint -i SPECS/writetype.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

Comment 2 Ankur Sinha (FranciscoD) 2011-04-20 19:00:52 UTC
Some notes:

1. The patch needs a comment describing its requirement.
2. You no longer need the buildroot tag, and the clean section.
3. It's shebang, not sheband ;)
4. In the files section, at the egginfo part, use py?.?.egg-info, it'll make updates easier. 
5. Do check if the python-setuptools is also a runtime requirement once. 
6. I don't see a LICENSE file in the files section. Please request upstream to include one.
7. You haven't handled the desktop file. Please refer to http://fedoraproject.org/wiki/PackagingGuidelines#Desktop_files for details
8. Isn't there an icon (Since there is a desktop file?)

The spec looks okay at a glance, but a detailed review might reveal more issues.

Regards,
Ankur

Comment 3 PRABIN KUMAR DATTA 2011-04-20 22:03:15 UTC
Updated:-
Spec URL: http://prabindatta.fedorapeople.org/SPECS/writetype.spec
SRPM URL: http://prabindatta.fedorapeople.org/SRPMS/writetype-1.2.130-2.fc14.src.rpm

Also, I have sent a request upstream to add a LICENSE file. 

@Ankur
Thank You, for the package review and also for the time you have spent for this.

Comment 4 Ankur Sinha (FranciscoD) 2011-04-21 05:36:54 UTC
Prabin,

You're most welcome. I wanted to point out that submitting packages isn't enough for sponsorship. You need to do unofficial reviews of as many packages as you can, and mention them in your reviews as well. The idea is that a sponsor needs to measure your understanding of the packaging guidelines before approving you.

I think this page details it out:

http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored

Thanks,
Ankur

Comment 5 PRABIN KUMAR DATTA 2011-04-23 03:23:52 UTC
Updated (Added upstream License):-
Spec URL: http://prabindatta.fedorapeople.org/SPECS/writetype.spec
SRPM URL: http://prabindatta.fedorapeople.org/SRPMS/writetype-1.2.130-3.fc14.src.rpm

Comment 6 Elad Alfassa 2011-06-18 17:38:20 UTC
+ Package meets naming and packaging guidelines
+ Spec file matches base package name.
+ Spec has consistant macro usage.
+ Meets Packaging Guidelines.
+ License
? License field in spec matches
+ License file included in package
+ Spec in American English
+ Spec is legible.
- Package needs ExcludeArch
? BuildRequires correct
- Spec handles locales/find_lang
- Package is relocatable and has a reason to be.
? Package has %defattr and permissions on files is good.
- Package has a correct %clean section.
+ Package has correct buildroot
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
+ Package is code or permissible content.
- Doc subpackage needed/used.
+ Packages %doc files don't affect runtime.

- Headers/static libs in -devel subpackage.
- Spec has needed ldconfig in post and postun
- .pc files in -devel subpackage/requires pkgconfig
- .so files in -devel subpackage.
- -devel package Requires: %{name} = %{version}-%{release}
- .la files are removed.

+ Package is a GUI app and has a .desktop file

? Package compiles and builds on at least one arch.
+ Package has no duplicate files in %files.
- Package doesn't own any directories other packages own.
- Package owns all the directories it creates.
+ No rpmlint output.
- final provides and requires are sane:
(include output of for i in *rpm; do echo $i; rpm -qp --provides $i; echo =; rpm -qp --requires $i; echo; done
manually indented after checking each line.  I also remove the rpmlib junk and anything provided by glibc.)

SHOULD Items:

? Should build in mock.
- Should build on all supported archs
- Should function as described.
- Should have sane scriptlets.
- Should have subpackages require base package with fully versioned depend.
- Should have dist tag
- Should package latest version
- check for outstanding bugs on package. (For core merge reviews)

Issues:

1. The license is GPLv3+ and not GPLv3. (notice the line "or (at your option) any later version.")
2. Strange premissions on the desktop file. Why?
3. Doesn't build in mock, missing build requirement on desktop-file-utils



-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 7 PRABIN KUMAR DATTA 2011-06-18 20:25:13 UTC
Updated package:-
Spec URL: http://prabindatta.fedorapeople.org/SPECS/writetype.spec
SRPM URL:
http://prabindatta.fedorapeople.org/SRPMS/writetype-1.2.130-5.fc15.src.rpm

> 1. The license is GPLv3+ and not GPLv3. (notice the line "or (at your option)
> any later version.")
Updated.

> 2. Strange premissions on the desktop file. Why?
After extraction from tarball file permission for  writetype.desktop is "755".
Now, in Makefile file under install section, code to install desktop file is:
"cp writetype.desktop $(ICONDIR)/writetype.desktop"
That why, the file permission which should have to 644 is 755.

> 3. Doesn't build in mock, missing build requirement on desktop-file-utils
Updated. But, here I like to mention one thing that pyttsx is required for this package which in under review request[1]. Thou, pyttsx is not mandatory since enchant and festival are already there performing similar function.

-
[1]. https://bugzilla.redhat.com/show_bug.cgi?id=702998

Comment 8 Elad Alfassa 2011-06-19 06:29:51 UTC
+ Package meets naming and packaging guidelines
+ Spec file matches base package name.
+ Spec has consistant macro usage.
+ Meets Packaging Guidelines.
+ License
+ License field in spec matches
+ License file included in package
+ Spec in American English
+ Spec is legible.
- Package needs ExcludeArch
+ BuildRequires correct
- Spec handles locales/find_lang
- Package is relocatable and has a reason to be.
+ Package has %defattr and permissions on files is good.
- Package has a correct %clean section.
+ Package has correct buildroot
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
+ Package is code or permissible content.
- Doc subpackage needed/used.
+ Packages %doc files don't affect runtime.

- Headers/static libs in -devel subpackage.
- Spec has needed ldconfig in post and postun
- .pc files in -devel subpackage/requires pkgconfig
- .so files in -devel subpackage.
- -devel package Requires: %{name} = %{version}-%{release}
- .la files are removed.

+ Package is a GUI app and has a .desktop file

+ Package compiles and builds on at least one arch.
+ Package has no duplicate files in %files.
- Package doesn't own any directories other packages own.
- Package owns all the directories it creates.
? No rpmlint output.
writetype.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/writetype/espeakInterface.py 0644L /usr/bin/python
writetype.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/writetype/listWidget.py 0644L /usr/bin/python
writetype.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/writetype/ttsInterface.py 0644L /usr/bin/python
writetype.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/writetype/festivalInterface.py 0644L /usr/bin/python
writetype.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/writetype/pyttsxInterface.py 0644L /usr/bin/python
writetype.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/writetype/main.py 0644L /usr/bin/python
writetype.noarch: W: no-manual-page-for-binary writetype
2 packages and 0 specfiles checked; 6 errors, 1 warnings.

The non-executable-script error is because of the (un-needed) shabang in the library files. I don't think it's a major issue, you can make a patch to remove the shabang from these files if you want.
the no-manual-page error can be ignored.

+ final provides and requires are sane:
writetype-1.2.130-5.fc16.noarch.rpm
mimehandler(application/x-writetype)  
writetype = 1.2.130-5.fc16
=
/bin/bash  
PyQt4  
enchant  
festival  
python(abi) = 2.7
pyttsx  
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(FileDigests) <= 4.6.0-1
rpmlib(PartialHardlinkSets) <= 4.0.4-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rpmlib(PayloadIsXz) <= 5.2-1

writetype-1.2.130-5.fc16.src.rpm
=
python2-devel  
python-setuptools  
desktop-file-utils  
rpmlib(FileDigests) <= 4.6.0-1
rpmlib(CompressedFileNames) <= 3.0.4-1


SHOULD Items:

+ Should build in mock.
- Should build on all supported archs
- Should function as described.
- Should have sane scriptlets.
- Should have subpackages require base package with fully versioned depend.
+ Should have dist tag
+ Should package latest version

Seems fine, APPROVED.    	





-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 9 PRABIN KUMAR DATTA 2011-06-19 12:45:37 UTC
> I don't think it's a major issue, you can make a patch to remove
> the shabang from these files if you want.

Updated package:-
Spec URL: http://prabindatta.fedorapeople.org/SPECS/writetype.spec
SRPM URL:
http://prabindatta.fedorapeople.org/SRPMS/writetype-1.2.130-6.fc15.src.rpm

> Seems fine, APPROVED.  
Thank You!

Comment 10 PRABIN KUMAR DATTA 2011-06-19 12:48:58 UTC
New Package SCM Request
=======================
Package Name: writetype
Short Description: Light word processor
Owners: prabindatta
Branches: f14 f15 f16 el6
InitialCC:

Comment 11 Gwyn Ciesla 2011-06-19 22:18:19 UTC
Git done (by process-git-requests).

Comment 12 Fedora Update System 2011-06-20 13:08:23 UTC
writetype-1.2.130-6.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/writetype-1.2.130-6.fc14

Comment 13 Fedora Update System 2011-06-20 13:18:08 UTC
writetype-1.2.130-6.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/writetype-1.2.130-6.fc15

Comment 14 Fedora Update System 2011-06-20 13:27:32 UTC
writetype-1.2.130-6.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/writetype-1.2.130-6.el6

Comment 15 Fedora Update System 2011-06-21 17:14:25 UTC
writetype-1.2.130-6.el6 has been pushed to the Fedora EPEL 6 testing repository.

Comment 16 Fedora Update System 2011-06-29 21:52:25 UTC
writetype-1.2.130-6.fc15 has been pushed to the Fedora 15 stable repository.

Comment 17 Fedora Update System 2011-06-29 21:52:38 UTC
writetype-1.2.130-6.fc14 has been pushed to the Fedora 14 stable repository.

Comment 18 Fedora Update System 2011-07-08 16:04:42 UTC
writetype-1.2.130-6.el6 has been pushed to the Fedora EPEL 6 stable repository.


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