Bug 525151 - Review Request: CLconverter - A simple command line tool for converting units
Summary: Review Request: CLconverter - A simple command line tool for converting units
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-09-23 14:45 UTC by Mario Basic
Modified: 2010-05-03 18:26 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-05-03 18:26:12 UTC


Attachments (Terms of Use)

Description Mario Basic 2009-09-23 14:45:30 UTC
Spec URL: http://www.information-hq.org/data/projects/CLconverter/Spec/CLconverter-0.4.9.spec
SRPM URL: http://www.information-hq.org/data/projects/CLconverter/RPMs/CLconverter-0.4.9-2.fc11.i586.rpm
Description: It's a simple command-line tool for converting metric to imperial units and vice-versa.Now it supports cm,mm,dm,m,km,inch,mile and yard conversions. This is my first package and I need a sponsor.

Comment 2 Jason Tibbitts 2009-09-23 20:40:16 UTC
Spec URL above is 404; please fix.

Comment 4 Ralf Corsepius 2009-09-24 07:25:41 UTC
(In reply to comment #3)
> SRPM URL:
> http://www.information-hq.org/data/projects/CLconverter/RPMs/CLconverter-0.4.7-3.fc11.i586.rpm  

You posted an URL to a binary RPM. Something, which is pretty much pointless in a package review. You are supposed to post an SRPM URL.

Comment 5 Mario Basic 2009-09-24 07:49:05 UTC
SRPM URL: http://www.information-hq.org/data/projects/CLconverter/Srpm/CLconverter-0.4.7-3.fc11.src.rpm
Thanks, I've got it mixed up.

Comment 6 Ralf Corsepius 2009-09-24 10:06:28 UTC
(In reply to comment #5)
> SRPM URL:
Thanks.

As you seem to be new to Fedora packaging, let's start with some basic remarks on your package:

1) Fedora *.spec's are supposed to be named <package>.spec.
You are using <package>-<version>.spec
Please change this.

2) Your *.spec contains hard-coded /usr/bin.
Please use %{_bindir} instead.

3) The compilation being applied by your spec doesn't apply the CFLAGS, all packages in Fedora are supposed to use.
One way to achieve this with your package is to explicitly set CFLAGS:
make CFLAGS="${RPM_OPT_FLAGS}"

4) You are stripping the application in %install. Fedora applications need to be built and installed unstripped.
Please remove the "-s" from install.

5) The source files inside of your tarball carry bogus permissions (they are executable). As you seem to be upstream of the tarball, I'd recommend "upstream" to fix this in the upstream tarball.
If this should not be applicable, the permissions should be fixed in %build.

Hint: Are you familiar with "rpmlint"? If not, please make yourself familiar with it - Though this tool is far from being perfect and occasionally produces bogus warning, it helps avoiding the worst packaging mistakes.

Comment 7 Mario Basic 2009-09-24 12:27:08 UTC
Thanks a lot for this, really helped. I used it as a checklist.
1.done
2.done
3.I think I've done this because I had to write CFLAGS="${RPM_OPT_FLAGS}" to work. When I wrote make CFLAGS="${RPM_OPT_FLAGS}" It would report an error.
4.done
5.done

I used rpmlint to check the spec file and package, and now it reports 0 error and warnings. I used rpmlint CLconverter CLconverter.spec to check it.


SPEC URL:
http://www.information-hq.org/data/projects/CLconverter/Spec/CLconverter.spec
SRPM URL:
http://www.information-hq.org/data/projects/CLconverter/Srpm/CLconverter-0.4.7-8.fc11.src.rpm

Comment 8 Ralf Corsepius 2009-09-24 14:47:52 UTC
(In reply to comment #7)
>
> 3.I think I've done this because I had to write CFLAGS="${RPM_OPT_FLAGS}" to
> work. When I wrote make CFLAGS="${RPM_OPT_FLAGS}" It would report an error.

make CFLAGS="${RPM_OPT_FLAGS}" CLconverter
would have been it.

> 5.done
You seem to have rebuilt the tarball without incrementing the version.
A reasonable upstream increments the version each time it releases a new tarball, because people are checking tarballs for changes and will yell at you when a tarball is being replaced without incrementing the version.

BTW1: Did you consider to add a Makefile to your package? It would help packagers of various distros and OSes, and assist them to avoid having to figure out how your package is supposed to be built.

BTW2: Do you have another package on review? Based on this package review alone, I am not yet sufficiently convinced about your packaging expertise to sponsor you.

Comment 9 Martin Gieseking 2009-09-25 06:29:20 UTC
A few additional remarks from me: :)

- the license tag must be GPLv3+ (because of the addition "or
    (at your option) any later version" in the source headers)

- the given URL doesn't seem to be up-to-date (404)

- append the CFLAGS assignment to the make statement:
  make CLconverter CFLAGS="${RPM_OPT_FLAGS}" 

- in %files, replace %defattr(-,root,root) by %defattr(-,root,root,-)

- you should add a changelog entry with a short summary about the changes for each revision, so that the file history can be reproduced

- change the file permissions of the tarball to 0644  


$ rpmlint /var/lib/mock/fedora-11-x86_64/result/CLconverter-*
CLconverter.src: W: strange-permission CLconverter-0.4.7.tar.gz 0755
CLconverter-debuginfo.x86_64: E: debuginfo-without-sources
3 packages and 0 specfiles checked; 1 errors, 1 warnings.

Comment 10 Mario Basic 2009-09-25 08:46:58 UTC
(In reply to comment #8)
> 
> make CFLAGS="${RPM_OPT_FLAGS}" CLconverter
> would have been it.
> 
> > 5.done
> You seem to have rebuilt the tarball without incrementing the version.
> A reasonable upstream increments the version each time it releases a new
> tarball, because people are checking tarballs for changes and will yell at you
> when a tarball is being replaced without incrementing the version.
> 
> BTW1: Did you consider to add a Makefile to your package? It would help
> packagers of various distros and OSes, and assist them to avoid having to
> figure out how your package is supposed to be built.
> 
> BTW2: Do you have another package on review? Based on this package review
> alone, I am not yet sufficiently convinced about your packaging expertise to
> sponsor you.  

I've changed the spec file to use make CFLAGS="${RPM_OPT_FLAGS}" CLconverter.
Totally forgotten about changing the version.
Will look into how to create a Makefile. I have done it once before but forgotten.
No, this one only, yet. 

(In reply to comment #9)
> A few additional remarks from me: :)
> 
> - the license tag must be GPLv3+ (because of the addition "or
>     (at your option) any later version" in the source headers)
> 
> - the given URL doesn't seem to be up-to-date (404)
> 
> - append the CFLAGS assignment to the make statement:
>   make CLconverter CFLAGS="${RPM_OPT_FLAGS}" 
> 
> - in %files, replace %defattr(-,root,root) by %defattr(-,root,root,-)
> 
> - you should add a changelog entry with a short summary about the changes for
> each revision, so that the file history can be reproduced
> 
> - change the file permissions of the tarball to 0644  
> 
> 
> $ rpmlint /var/lib/mock/fedora-11-x86_64/result/CLconverter-*
> CLconverter.src: W: strange-permission CLconverter-0.4.7.tar.gz 0755
> CLconverter-debuginfo.x86_64: E: debuginfo-without-sources
> 3 packages and 0 specfiles checked; 1 errors, 1 warnings.  

I've changed the license tag.
URL to the new files is below.
I've used the suggestion from comment #8 to set the CFLAGS.
I have replaced %defattr(-,root,root) to %defattr(-,root,root,-).

SPEC URL: http://www.information-hq.org/download/projects/CLconverter/Spec/CLconverter.spec
SRPM URL: http://www.information-hq.org/download/projects/CLconverter/Srpm/CLconverter-0.4.8-1.fc11.src.rpm

Comment 11 Jan Klepek 2009-09-26 10:11:30 UTC
1] keep timestamp when performing install (add -p switch to install command)
https://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps

2] Source0 doesn't exists
$ wget http://www.information-hq.org/data/projects/CLconverter/Source/CLconverter-0.4.8.tar.gz
--2009-09-26 12:01:54--  http://www.information-hq.org/data/projects/CLconverter/Source/CLconverter-0.4.8.tar.gz
Resolving www.information-hq.org... 69.174.114.214
Connecting to www.information-hq.org|69.174.114.214|:80... connected.
HTTP request sent, awaiting response... 404 Not Found
2009-09-26 12:01:54 ERROR 404: Not Found.

3] wrong format of Source0:
http://fedoraproject.org/wiki/PackageMaintainers/CreatingPackageHowTo#Spec_file_pieces_explained

4] Url tag is reffering to 404 page

Comment 12 Ralf Corsepius 2009-09-26 12:09:09 UTC
(In reply to comment #11)
> 1] keep timestamp when performing install (add -p switch to install command)
> https://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps
Even if preserving timestamps was useful (Which it isn't the case), there are no timestamps which are useful to be preserved in this package.

> 3] wrong format of Source0:
> http://fedoraproject.org/wiki/PackageMaintainers/CreatingPackageHowTo#Spec_file_pieces_explained
Except that the URL is invalid, I don't understand your point is.

Jan, your comments were not useful.

Comment 13 Mario Basic 2009-09-26 14:36:22 UTC
I was reorganizing my website so there was a little mess with the URLs.
I have fixed the broken URLs, so I've repacked the package and these are the new links:

SPEC URL: http://www.information-hq.org/download/projects/CLconverter/Spec/CLconverter.spec

SPRM URL: http://www.information-hq.org/download/projects/CLconverter/Srpm/CLconverter-0.4.8-2.fc11.src.rpm  

I think I got all previous remarks covered. If you have some more, please tell me so I can work on fixing them. Thanks

Comment 14 Mohammed Imran 2010-05-03 11:16:17 UTC
URL's are not working


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