Bug 432246 - Review Request: pic2aa - Pic2AA is tool for converting jpeg/png to AA (Ascii Art) images
Summary: Review Request: pic2aa - Pic2AA is tool for converting jpeg/png to AA (Ascii ...
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Parag AN(पराग)
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-02-10 12:27 UTC by Krzysztof Kurzawski
Modified: 2008-02-16 07:53 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-02-16 07:53:54 UTC
Type: ---
Embargoed:
panemade: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Krzysztof Kurzawski 2008-02-10 12:27:05 UTC
Spec URL: http://kurzawa.nonlogic.org/rpm/pic2aa/pic2aa.spec
SRPM URL: http://kurzawa.nonlogic.org/rpm/pic2aa/pic2aa-0.2.1-1.fc8.src.rpm
Description: Pic2AA is tool providing converting jpeg/png (and any other image formats 
supported by Qt library) to AA (Ascii Art) images, using AA-Lib library. 
It can show preview of converted image and save image into text file.

Comment 1 Benjamin Lewis 2008-02-10 18:52:19 UTC
I'm not sponsored so I can't approve this, but from what I can see it meets
every requirement. The only changes I would make are that the word 'providing'
should be replaced with 'for' in the summary and description, and 'preview'
should be 'previews', also in the description.

Source file match upstream: YES
Package meets versioning and naming guidelines: YES
Specfile is properly named, is cleanly written and uses macros consistently: YES
Dist tag is present: YES
Build root is correct: YES
License field matches the actual license: YES
License is open source-compatible: YES
Latest version is being packaged: YES
BuildRequires are proper: YES
Compiler flags are appropriate: YES
%clean is present: YES
Package installs properly
debuginfo package looks complete: YES
rpmlint is silent: YES
Final provides and requires are sane: YES
%check is present and all tests pass: YES
No shared libraries are added to the regular linker search paths: YES
Owns the directories it creates: YES
Doesn't own any directories it shouldn't: YES
No duplicates in %files: YES
File permissions are appropriate: YES
No scriptlets present: YES
Code, not content: YES
Documentation is small, so no -docs subpackage is necessary: YES
No headers: YES
No pkgconfig files: YES
No libtool .la droppings: YES

Comment 2 Benjamin Lewis 2008-02-10 19:15:30 UTC
Further to the above:

Forgot to mention that rpmlint is complaining about:

pic2aa.i386: W: file-not-utf8 /usr/share/doc/pic2aa-0.2.1/COPYING
pic2aa.i386: W: file-not-utf8 /usr/share/doc/pic2aa-0.2.1/AUTHORS
 
on the binary RPM. Also, after a conversation in #fedora-devel, I think that
specspo handles translations, so I'm not sure if these should be in the spec.

Comment 3 Rahul Sundaram 2008-02-10 19:29:34 UTC
You can use iconv to convert it into UTF-8

Example:

%prep

iconv --from=ISO-8859-1 --to=UTF-8 file.txt > file.txt.utf8
mv file.txt.utf8 file.txt

Comment 4 Krzysztof Kurzawski 2008-02-10 21:20:36 UTC
Fixed
Spec URL: http://kurzawa.nonlogic.org/rpm/pic2aa/pic2aa.spec
SRPM URL: http://kurzawa.nonlogic.org/rpm/pic2aa/pic2aa-0.2.1-2.fc8.src.rpm

Thanks for review and thanks for help!

Comment 5 Parag AN(पराग) 2008-02-11 08:13:41 UTC
Benjamin,
   Thanks for having a look at this package and your review. But, I see that 
"Fedora user tc1415, aka Benjamin Tomos Lewis <ben.lewis.uk> has requested
membership in the cvsextras group and needs a sponsor."
So, its good to review other's packages but you fist get sponsorship and then
start changing fedora-review flags. removing fedora-review+ and taking this for
review.



Comment 6 Parag AN(पराग) 2008-02-11 09:04:53 UTC
1)I see that timestamp of upstream tarball is preserved.
upstream tarball ->
-rw-r--r-- 1 parag parag 156379 2005-02-15 17:42 pic2aa-0.2.1.tar.gz
tarball in SRPM ->
-rw-rw-r-- 1 parag parag 156379 2008-02-10 15:10 pic2aa-0.2.1.tar.gz

2) Change comment in desktop file to what new summary is now.

3)So you took screenshot posted on upstream web site as icon. Not sure if there
will be any licensing issue for using that screenshot as icon for desktop.
  But looks ok as upstream is not providing any desktop file and icon file and
to use that screenshot as icon file.

4) Can you add comment in PL language also in .desktop file?



Comment 8 Parag AN(पराग) 2008-02-13 07:07:17 UTC
Review:
+ package builds in mock (rawhide i386).
koji build => http://koji.fedoraproject.org/koji/taskinfo?taskID=422141
+ rpmlint is silent for SRPM and for RPM.
+ source files match upstream.
8c0431aa6521f6abc55459cb9d6e70f8  pic2aa-0.2.1.tar.gz
d3b0b6f745e6137bd6fb54164e284dcf  shot1.png
+ package meets naming and packaging guidelines.
+ specfile is properly named, is cleanly written
+ Spec file is written in American English.
+ Spec file is legible.
+ dist tag is present.
+ build root is correct.
+ license is open source-compatible.
+ License text is included in package.
+ %doc files present.
+ BuildRequires are proper.
+ Compiler flags are honored correctly.
+ defattr usage is correct.
+ %clean is present.
+ package installed properly.
+ Macro use appears rather consistent.
+ Package contains code.
+ no static libraries.
+ no .pc file present.
+ no -devel subpackage exists.
+ no .la files.
+ no translations are available.
+ Does owns the directories it creates.
+ no duplicates in %files.
+ file permissions are appropriate.
+ no scriptlets are used.
+ Desktop file installed correctly.
+ GUI app.

 Should:
   Issue 1 from comment #1 still exists. Fix that.

APPROVED.

Comment 9 Krzysztof Kurzawski 2008-02-13 15:56:44 UTC
Could you explain me what should I fix?

Comment 10 Krzysztof Kurzawski 2008-02-13 15:58:32 UTC
New Package CVS Request
=======================
Package Name: pic2aa
Short Description: Pic2AA is tool for converting jpeg/png to AA (Ascii Art) images
Owners: kurzawa
Branches: F-7 F-8
InitialCC:
Cvsextras Commits: yes

Comment 11 Kevin Fenzi 2008-02-13 17:35:29 UTC
cvs done.

Comment 12 Parag AN(पराग) 2008-02-14 03:30:39 UTC
(In reply to comment #9)
> Could you explain me what should I fix?
  download source tarball using wget and just package it in SRPM. 


Comment 13 Krzysztof Kurzawski 2008-02-16 07:53:54 UTC
Imported and built. Thanks again for review!


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