Bug 688056 - Review Request: sir - A simple application for resizing images
Summary: Review Request: sir - A simple application for resizing images
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Arun S A G
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-03-16 07:28 UTC by Praveen Kumar
Modified: 2012-08-01 03:08 UTC (History)
4 users (show)

Fixed In Version: sir-2.1.1-3.fc14
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-04-15 21:28:51 UTC
Type: ---
Embargoed:
sagarun: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)

Description Praveen Kumar 2011-03-16 07:28:32 UTC
Spec URL: http://kumarpraveen.fedorapeople.org/sir/sir.spec
SRPM URL: http://kumarpraveen.fedorapeople.org/sir/sir-2.1.1-1.fc14.src.rpm
Description: SIR (Simple Image Resizer) is a simple application for resizing images,inspired by GTPY - ImageResizer But it use C++/QT and QImage class to convert the images.it also can rotate your images.

Comment 1 Rahul Sundaram 2011-03-18 14:09:09 UTC
Use macros in source0

INSTALL shouldn't be packaged

License tag must be GPLv2+.  Don't just read the license.  Look at the source file headers.  

rpmlint warnings to be fixed:

sir.spec:21: W: macro-in-comment %{name}  (rpmlint on spec file)

sir.x86_64: W: unstripped-binary-or-object /usr/bin/sir
sir.x86_64: E: zero-length /usr/share/doc/sir-2.1.1/TODO
sir.x86_64: W: no-manual-page-for-binary sir

"#remove binary
rm %{name}"

Why is this necessary?

"%{_datadir}/applications/*.desktop"

better not to use a catch all for a single desktop file.

Comment 2 Praveen Kumar 2011-03-18 16:51:33 UTC
(In reply to comment #1)
> Use macros in source0
> 
> INSTALL shouldn't be packaged
> 
> License tag must be GPLv2+.  Don't just read the license.  Look at the source
> file headers.  
Fixed 
> 
> rpmlint warnings to be fixed:
> 
> sir.spec:21: W: macro-in-comment %{name}  (rpmlint on spec file)
> 
> sir.x86_64: W: unstripped-binary-or-object /usr/bin/sir
Can't understand what's unstripped-binary-or-object mean, please explain.
> sir.x86_64: E: zero-length /usr/share/doc/sir-2.1.1/TODO
TODO is removed from doc section
> sir.x86_64: W: no-manual-page-for-binary sir
> 
> "#remove binary
> rm %{name}"
> 
> Why is this necessary?
Source already contain a binary, before build section we have to remove all binary IIRC.
> 
> "%{_datadir}/applications/*.desktop"
> 
> better not to use a catch all for a single desktop file.
Fixed

Here is updated spec and srpm urls 
Spec URL: http://kumarpraveen.fedorapeople.org/sir/sir.spec
SRPM URL: http://kumarpraveen.fedorapeople.org/sir/sir-2.1.1-2.fc14.src.rpm

rpmlint Output :
$ rpmlint sir.spec 
sir.spec: W: invalid-url Source0: http://sir.googlecode.com/files/sir_2.1.1.tar.gz HTTP Error 404: Not Found
0 packages and 1 specfiles checked; 0 errors, 1 warnings.
(may be due to proxy its not able to resolve url)

Comment 3 Rahul Sundaram 2011-03-18 17:10:29 UTC
"Can't understand what's unstripped-binary-or-object mean, please explain."

Debuginfo package is not being generated.  Confirm with mock and check your Makefile/configure scripts to see why.  

"Source already contain a binary, before build section we have to remove all
binary IIRC."

Inform upstream of this.  Merely not including it %files would omit it out of the package.  

Also rpmlint should be run on spec file, srpm and binary rpm

Comment 4 Praveen Kumar 2011-03-18 19:54:59 UTC
(In reply to comment #3)
> Debuginfo package is not being generated.  Confirm with mock and check your
> Makefile/configure scripts to see why.  
In my system debuginfo package is generated.
Please take a look http://kumarpraveen.fedorapeople.org/sir/sir-debuginfo-2.1.1-2.fc14.i686.rpm

> Inform upstream of this.  Merely not including it %files would omit it out of
> the package.  
I informed to upstream for removing the binaries. 

> Also rpmlint should be run on spec file, srpm and binary rpm

$rpmlint sir.spec ../SRPMS/sir-2.1.1-2.fc14.src.rpm ../RPMS/i686/sir-2.1.1-2.fc14.i686.rpm 
sir.spec: W: invalid-url Source0: http://sir.googlecode.com/files/sir_2.1.1.tar.gz HTTP Error 404: Not Found
sir.src: W: invalid-url Source0: http://sir.googlecode.com/files/sir_2.1.1.tar.gz HTTP Error 404: Not Found
sir.i686: W: no-manual-page-for-binary sir
2 packages and 1 specfiles checked; 0 errors, 3 warnings.

Comment 5 Rahul Sundaram 2011-03-18 20:33:47 UTC
Done a scratch build

http://koji.fedoraproject.org/koji/taskinfo?taskID=2924176

No debuginfo issue.  So ignore that.  

Minor nit pick

"But used C++/QT"

Should be " but uses"  in the description.    I will let Arun SAG do the full review and approve since he has taken it.

Comment 6 Arun S A G 2011-03-18 20:56:47 UTC
Praveen,

Why wouldn't you use 'make install' instead of manually copying binaries and data to their corresponding directories?

Also you seems to be ignoring the following files, which actually installs when using the default "make install"

images/sir-128x128.png
images/sir-32x32.png
images/sir-64x64.png
images/sir.ico
sir_service.desktop

Comment 7 Arun S A G 2011-03-18 20:57:22 UTC
Thanks rahul, for the informal review :-). I will do the full review.

Comment 8 Praveen Kumar 2011-03-19 05:08:02 UTC
(In reply to comment #6)
> Praveen,
> 
> Why wouldn't you use 'make install' instead of manually copying binaries and
> data to their corresponding directories?
> 
> Also you seems to be ignoring the following files, which actually installs when
> using the default "make install"
> 
> images/sir-128x128.png
> images/sir-32x32.png
> images/sir-64x64.png
> images/sir.ico
> sir_service.desktop

This MakeFile is generated using qmake and when I am using "make install DESTDIR=/path/to/install" then it is not creating directories(bin,pixmaps,share ..etc) on given path, but a GNU Makefile do. If above files are important then i can add manually.

Comment 9 Arun S A G 2011-03-19 06:32:27 UTC
(In reply to comment #8)
> 
> This MakeFile is generated using qmake and when I am using "make install
> DESTDIR=/path/to/install" then it is not creating directories(bin,pixmaps,share
> ..etc) on given path, but a GNU Makefile do. If above files are important then
> i can add manually.

A quick look at the makefile shows that, it is using INSTALL_ROOT as prefix. Remove everything in install section and try using the following lines

"make install INSTALL_ROOT=$RPM_BUILD_ROOT
desktop-file-install --dir=$RPM_BUILD_ROOT%{_datadir}/applications %{name}.desktop 

I do not know whether these files are important for the application, the application is working fine without them; But it desirable to include them as the original make file is installing them.

sir_service.desktop seems to adding this application to "Open with" dialog box.

Comment 10 Arun S A G 2011-03-19 06:35:12 UTC
Forgot!

Add the following too,

desktop-file-install desktop-file-install --dir=$RPM_BUILD_ROOT {_datadir}/applications %{name}_service.desktop

Comment 11 Praveen Kumar 2011-03-19 07:34:27 UTC
Thanks for suggestion, Fixed all Issues, here is updated spec and srpm
SPEC URL: http://kumarpraveen.fedorapeople.org/sir/sir.spec
SRPM URL: http://kumarpraveen.fedorapeople.org/sir/sir-2.1.1-3.fc14.src.rpm

rpmlint output:
$ rpmlint sir.spec ../SRPMS/sir-2.1.1-3.fc14.src.rpm ../RPMS/i686/sir-2.1.1-3.fc14.i686.rpm 
sir.spec: W: invalid-url Source0: http://sir.googlecode.com/files/sir_2.1.1.tar.gz HTTP Error 404: Not Found
sir.src: W: invalid-url Source0: http://sir.googlecode.com/files/sir_2.1.1.tar.gz HTTP Error 404: Not Found
sir.i686: W: no-manual-page-for-binary sir
2 packages and 1 specfiles checked; 0 errors, 3 warnings.

Comment 12 Arun S A G 2011-04-06 16:50:25 UTC
[+] OK
[X] NOT OKAY
[?] Investigate the issue before committing the package
[-] NA


[+] Package meets naming and packaging guidelines
[+] Spec file matches base package name.
[+] Spec has consistent macro usage.
[+] Meets Packaging Guidelines.
[+] License
[+] License field in spec matches
[+] License file included in package
[+] Spec in American English
[+] Spec is legible.
[+] Sources match upstream 
[zer0c00l@gnubox SPECS]$ md5sum ~/Downloads/sir_2.1.1.tar.gz 
bbb0526a8b828379449468066d166e04  /home/zer0c00l/Downloads/sir_2.1.1.tar.gz
[zer0c00l@gnubox SPECS]$ 


-- done
[-] 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
[+] 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.
http://koji.fedoraproject.org/koji/taskinfo?taskID=2979037

[+] Package has no duplicate files in %files.
[+] Package doesn't own any directories other packages own.
[+] Package owns all the directories it creates.

[?] rpmlint output 
[zer0c00l@gnubox sir]$ rpmlint ~/rpmbuild/RPMS/i686/sir-2.1.1-3.fc14.i686.rpm 
sir.i686: W: no-manual-page-for-binary sir
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

[zer0c00l@gnubox sir]$ rpmlint ~/rpmbuild/RPMS/i686/sir-debuginfo-2.1.1-3.fc14.i686.rpm 
sir-debuginfo.i686: W: hidden-file-or-dir /usr/src/debug/sir/.moc
sir-debuginfo.i686: W: hidden-file-or-dir /usr/src/debug/sir/.moc
sir-debuginfo.i686: W: hidden-file-or-dir /usr/src/debug/sir/.ui
sir-debuginfo.i686: W: hidden-file-or-dir /usr/src/debug/sir/.ui
1 packages and 0 specfiles checked; 0 errors, 4 warnings.

Check the hidden-file-or-dir warning, ask the upstream about the purpose of their existence before committing the package.

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)


XXX APPROVED XXX

Comment 13 Praveen Kumar 2011-04-07 03:16:16 UTC
New Package SCM Request
=======================
Package Name: sir 
Short Description: A simple application for resizing images
Owners: kumarpraveen
Branches: f13 f14 f15

Comment 14 Jason Tibbitts 2011-04-07 14:56:43 UTC
Git done (by process-git-requests).

Comment 15 Fedora Update System 2011-04-07 16:04:05 UTC
sir-2.1.1-3.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/sir-2.1.1-3.fc15

Comment 16 Fedora Update System 2011-04-07 16:05:33 UTC
sir-2.1.1-3.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/sir-2.1.1-3.fc14

Comment 17 Fedora Update System 2011-04-07 16:06:32 UTC
sir-2.1.1-3.fc13 has been submitted as an update for Fedora 13.
https://admin.fedoraproject.org/updates/sir-2.1.1-3.fc13

Comment 18 Fedora Update System 2011-04-08 23:20:15 UTC
sir-2.1.1-3.fc14 has been pushed to the Fedora 14 testing repository.

Comment 19 Fedora Update System 2011-04-15 21:28:46 UTC
sir-2.1.1-3.fc15 has been pushed to the Fedora 15 stable repository.

Comment 20 Fedora Update System 2011-04-16 20:52:28 UTC
sir-2.1.1-3.fc13 has been pushed to the Fedora 13 stable repository.

Comment 21 Fedora Update System 2011-04-16 20:59:18 UTC
sir-2.1.1-3.fc14 has been pushed to the Fedora 14 stable repository.

Comment 22 Fedora Update System 2012-08-01 03:08:37 UTC
sir-2.4-1.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/sir-2.4-1.fc17


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