Bug 688056 - Review Request: sir - A simple application for resizing images
Review Request: sir - A simple application for resizing images
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Arun S A G
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-16 03:28 EDT by Praveen Kumar
Modified: 2012-07-31 23:08 EDT (History)
4 users (show)

See Also:
Fixed In Version: sir-2.1.1-3.fc14
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2011-04-15 17:28:51 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
sagarun: fedora‑review+
tibbs: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Praveen Kumar 2011-03-16 03:28:32 EDT
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 10:09:09 EDT
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 12:51:33 EDT
(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 13:10:29 EDT
"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 15:54:59 EDT
(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 16:33:47 EDT
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 16:56:47 EDT
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 16:57:22 EDT
Thanks rahul, for the informal review :-). I will do the full review.
Comment 8 Praveen Kumar 2011-03-19 01:08:02 EDT
(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 02:32:27 EDT
(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 02:35:12 EDT
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 03:34:27 EDT
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 12:50:25 EDT
[+] 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-06 23:16:16 EDT
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 10:56:43 EDT
Git done (by process-git-requests).
Comment 15 Fedora Update System 2011-04-07 12:04:05 EDT
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 12:05:33 EDT
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 12:06:32 EDT
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 19:20:15 EDT
sir-2.1.1-3.fc14 has been pushed to the Fedora 14 testing repository.
Comment 19 Fedora Update System 2011-04-15 17:28:46 EDT
sir-2.1.1-3.fc15 has been pushed to the Fedora 15 stable repository.
Comment 20 Fedora Update System 2011-04-16 16:52:28 EDT
sir-2.1.1-3.fc13 has been pushed to the Fedora 13 stable repository.
Comment 21 Fedora Update System 2011-04-16 16:59:18 EDT
sir-2.1.1-3.fc14 has been pushed to the Fedora 14 stable repository.
Comment 22 Fedora Update System 2012-07-31 23:08:37 EDT
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.