Bug 241654 - Review Request: ipcalculator - A utility for computing broadcast, network, mask, and host ranges
Summary: Review Request: ipcalculator - A utility for computing broadcast, network, ma...
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Marek Mahut
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-05-29 11:58 UTC by Jakub Hrozek
Modified: 2008-02-12 12:15 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-02-12 12:15:19 UTC
Type: ---
Embargoed:
mmahut: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Jakub Hrozek 2007-05-29 11:58:55 UTC
Spec URL: http://www.volny.cz/jhrozek/ipcalc/ipcalc.spec
SRPM URL: http://www.volny.cz/jhrozek/ipcalc/ipcalc-0.41-1.fc7.src.rpm
Description: 
ipcalc takes an IP address and netmask and calculates the resulting
broadcast, network, Cisco wildcard mask, and host range. By giving a second
netmask, you can design subnets and supernets. It is also intended to be
a teaching tool and presents the subnetting results as easy-to-understand
binary values.

Comment 1 manuel wolfshant 2007-05-29 12:13:56 UTC
Fedora already includes an ipcalc utility (in initscripts). Trying to install 
another application with the very same name (and location, /bin/ipcalc) will
lead to confusions and conflicts.


Comment 2 Jakub Hrozek 2007-05-29 12:38:32 UTC
Ouch, you are right, I was not aware of the initscripts ipcalc..

One minor thing, this ipcalc installs its binary into /usr/bin/ipcalc, 
not /bin/ipcalc, so these two would not slap on one another, but one would 
have to invoke the binary with full path, which is quite ugly..

Would renaming of the package/binary be allowed? If not, I'm OK with 
withdrawing this package.


Comment 3 manuel wolfshant 2007-05-29 12:40:22 UTC
Sure, renaming is fine

Comment 5 Philippe Valembois 2007-05-29 19:34:25 UTC
Package doesn't follow guidelines : 
-http://fedoraproject.org/wiki/Packaging/Guidelines#head-5d1681fa7cf3714ad490fbf7c095a0cfe16da27f


Comment 6 Jakub Hrozek 2007-06-02 10:34:58 UTC
* Sat Jun 1 2007 Jakub Hrozek <jhrozek> - 0.41-3
-move the cgi script outside /var/www

http://www.volny.cz/jhrozek/ipcalc/ipcalculator.spec
http://www.volny.cz/jhrozek/ipcalc/ipcalculator-0.41-3.fc8.src.rpm

Comment 7 Patrice Dumas 2007-06-02 11:07:38 UTC
The sed substitution may also be along:

sed -i 's:/usr/local/bin/ipcalc/:%{_bindir}/ipcalculator:' \
   $RPM_BUILD_ROOT%{_datadir}/%{name}/ipcalculator.cgi


Regarding the package name, this deviation is accepted in the 
guidelines since it adds a best judgement clause. And in that 
case indeed there could be confusion with the other ipcalc.

You should own the
%{_datadir}/%{name}/
directory.

You should also patch the program to use ipcalculator instead of 
ipcalc in help message. Unless I am wrong it could also be achieved
by

sed -e 's/ipcalc /%{name} /'

I know it is a small package, but you may want to split the cgi 
part and the main script anyway since they correspond with very 
different uses in my opinion. Also, especially in case you have 
a -cgi subpackage, you may consider shipping a config file for 
apache, as doc or in /etc/httpd/conf.d such that the cgi works
more easily out of the box. Having a config file for httpd already
installed has advantages (works out of the box) and inconvenients 
(may create security issues if it isn't required to change something 
manually by the local admin). All that is optional and left to your
best judgement.

Comment 8 manuel wolfshant 2007-06-02 21:37:28 UTC
Something similar to the below lines should do for
/etc/httpd/conf.d/ipcalculator.conf (beware: not tested)


ScriptAlias /ipcalculator/ /usr/share/ipcalculator/
<Directory /usr/share/ipcalculator/>
        DirectoryIndex ipcalculator.cgi
        Options ExecCGI
        order deny,allow
        deny from all
        allow from 127.0.0.1
</Directory>



Comment 9 Jakub Hrozek 2007-06-02 22:10:09 UTC
Patrice, Manuel,
Thanks for the valuable suggestions. I'll fix the issues pointed out in 
comment #7, also the split seems like a good idea, thanks for proposing the 
httpd config file.

But it might take a few days as I'm currently swamped with other things (a.k.a 
it's exam time on universities :) ), please don't take it as I'm abandoning 
the review and/or ignoring your suggestions.

Comment 10 Jakub Hrozek 2007-06-16 19:48:53 UTC
* Sat Jun 16 2007 Jakub Hrozek <jhrozek> - 0.41-4
- split the CGI wrapper into its own -cgi subpackage
- nicer sed substitution in the CGI module
- own the /usr/share/ipcalculator directory
- add a config file for httpd (written by Manuel Wolfshant)
- mention ipcalculator, not ipcalc in usage and help messages

http://hrozkovi.homelinux.org/ipcalculator/ipcalculator.spec
http://hrozkovi.homelinux.org/ipcalculator/ipcalculator-0.41-4.fc8.src.rpm

One question - is it necessary to have some docs in the -cgi subpackage even
though it requires the ipcalculator package which contains the necessary docs?
(rpmlint gripes about that)

Comment 11 Patrice Dumas 2007-06-16 23:48:59 UTC
(In reply to comment #10)

> One question - is it necessary to have some docs in the -cgi subpackage even
> though it requires the ipcalculator package which contains the necessary docs?
> (rpmlint gripes about that)

You don't have to worry about that rpmlint warning. In general rpmlint
errors and warnings may be ignored when they are pointless, as it is
the case here.

Comment 12 Patrice Dumas 2007-11-28 18:47:59 UTC
I cannot access the srpm... I can't remember why I didn't
finished that review...

Comment 13 Jakub Hrozek 2007-11-28 19:02:29 UTC
srpm re-uploaded.
Thanks for your time taking a look at my package!

Comment 14 Patrice Dumas 2007-11-28 22:13:15 UTC
The perl(CGI) requires seems to be only for -cgi, and autodetected.

Similarly the perl requires is autodetected.

Shouldn't the cgi subpackage depend on httpd?

GPL isn't a valid license anymore, it is GPL+, GPLv2, GPLv2+...

The source archive timestamp isn't kept:
-rw-rw-r-- 1 dumas dumas 21599 jui 27  2006 ipcalc-0.41.tar.gz
-rw-rw-r-- 1 dumas dumas 21599 mai 29  2007 ../SOURCES/ipcalc-0.41.tar.gz

You can use wget -N, spectool -g and the corresponding curl option.

There are issues with the package as I tested. First the images
are considered as scripts with the conf file Manuel proposed. I
propose the following:

Alias /ipcalculator/ /usr/share/ipcalculator/
<Directory /usr/share/ipcalculator/>
    DirectoryIndex ipcalculator.cgi
    Options ExecCGI
    AddHandler cgi-script cgi
    order deny,allow
    deny from all
    allow from 127.0.0.1
</Directory>

The images should be owned by the -cgi subpackage.

There is a missing image files, bg.gif.  ipcalc.gif should be 
renamed ipcal03.gif.

<a href="ipcalc.png">Screenshot</a> (ipcalc works also at the prompt)<br>
should be changed to
<a href="http://jodies.de/ipcalc.png">Screenshot</a> (ipcalc works also at the
prompt)<br>
Similar with
<a href="ipcalc-archive">Archive</a><br>
<a href="ipcalc-faq/win32.html">How to run this under windows</a><br>


Suggestion:
use %{version} in 
Source0:        http://jodies.de/ipcalc-archive/ipcalc-%{version}.tar.gz

Comment 15 Mamoru TASAKA 2007-12-16 04:59:34 UTC
Any progress on here?

Comment 16 Jakub Hrozek 2007-12-16 14:40:55 UTC
Thanks for poking me, Mamoru! I totally forgot about this one..

http://hrozkovi.cz/ipcalculator.spec
http://hrozkovi.cz/ipcalculator-0.41-5.fc7.src.rpm

The bg.gif image is still not included, as I'd like to use the one at the
upstream page, so I queried the author about including it. 

Comment 17 Patrice Dumas 2007-12-16 18:17:21 UTC
The sed is wrong:
sed -i 's:/usr/local/bin/ipcalc/:%{_bindir}/ipcalculator:' ipcalc.cgi
should be
sed -i 's:/usr/local/bin/ipcalc:%{_bindir}/ipcalculator:' ipcalc.cgi

Comment 18 Jakub Hrozek 2007-12-26 20:00:55 UTC
http://hrozkovi.cz/ipcalculator.spec
http://hrozkovi.cz/ipcalculator-0.41-6.fc7.src.rpm

* Wed Dec 26 2007 Jakub Hrozek <jhrozek> - 0.41-6
- fix a typo in the sed substitution
- add the background image from the project page

Comment 19 Marek Mahut 2008-02-10 18:21:04 UTC
Jakub,

All ok for me. 

APPROVED.

Comment 20 Patrice Dumas 2008-02-10 18:47:16 UTC
Looks good to me too.

Comment 21 Jakub Hrozek 2008-02-11 21:56:01 UTC
New Package CVS Request
=======================
Package Name: ipcalculator
Short Description: A utility for computing broadcast, network, mask, and host ranges
Owners: jhrozek
Branches: F-7 F-8
InitialCC: none
Cvsextras Commits: yes

Comment 22 Kevin Fenzi 2008-02-12 02:35:41 UTC
cvs done.

Comment 23 Jakub Hrozek 2008-02-12 12:15:19 UTC
Built for rawhide:
http://koji.fedoraproject.org/koji/taskinfo?taskID=418486

Patrice, Manuel, Marek - thanks for the work done on this review!


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