Bug 460538 - Review Request: ircd-ratbox - Ircd-ratbox is an advanced, stable and fast ircd
Review Request: ircd-ratbox - Ircd-ratbox is an advanced, stable and fast ircd
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Lubomir Rintel
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 460632
  Show dependency treegraph
 
Reported: 2008-08-28 09:57 EDT by Marek Mahut
Modified: 2008-08-31 11:39 EDT (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-08-31 11:39:07 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
lkundrak: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Marek Mahut 2008-08-28 09:57:19 EDT
Spec URL: http://mmahut.fedorapeople.org/reviews/ircd-ratbox/ircd-ratbox.spec
SRPM URL: http://mmahut.fedorapeople.org/reviews/ircd-ratbox/ircd-ratbox-2.2.8-1.fc8.src.rpm
Koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=790314
Description: ircd-ratbox is an advanced, stable, fast ircd. It is an evolution where
ircd-hybrid left off around version 7-rc1. It supports the TS3 and TS5
protocols, and is used on EFnet and other IRC networks.
Comment 1 Marek Mahut 2008-08-28 09:58:03 EDT
Lubomir, please take a look in your free time at this package.
Comment 2 Lubomir Rintel 2008-08-28 16:00:05 EDT
Dude this just plainly sucks.
Comment 3 Lubomir Rintel 2008-08-28 16:00:50 EDT
No, seriously:

1.) You use Patch tag with an index and %patch macro without one:

Patch0:         ircd-ratbox-2.2.8-offbyone.patch
...
%patch -p1 -b .offbyone

Either replace "Patch9" with "Patch" or "%patch" with "%patch0"

2.) sysconfig/ircd has a redundant setting:

PIDFILE=/var/run/ircd.pid

The init script assigns a default value.
You may want to remove this from sysconfid/ircd

3.) Useless comment in init script:

# Note: pidfile is assumed to be created
# by ircd (config: server.pid-file).
# If not, uncomment 'pidof' line.

Really, there's no "'pidof' line"

4.) Patch status

You added a fairly long and sophisticated patch. You should send it upstream
accompany it with status comment.

5.) Useless comment in SPEC file:

#make %{?_smp_mflags} -C contrib

Get rid of that. Or build contrib stuff and eventually make a subpackage?

6.) Hardcoded path

--with-logdir=/var/log/ircd
...
mkdir -p $RPM_BUILD_ROOT/var/log ...

Replace with %{_localstatedir} (or %{_var}?)

7.) Own directories you create

Add %dir %{_sysconfdir}/ircd

8.) Add explaining comments to non-obvious commands:

mkdir -p ... $RPM_BUILD_ROOT/usr/local/ircd/

Why do you create a directory in /usr/local?

mv $RPM_BUILD_ROOT%{_datadir}/ircd-old/modules $RPM_BUILD_ROOT%{_datadir}/ircd/modules
rm -fr $RPM_BUILD_ROOT%{_datadir}/ircd-old

Why do you shuffle ircd-old around? What does it contain?

sed 's/-Werror//g' -i configure

Why do you strip this utterly useful compiler flag away?
What's the status of upstreaming this fix?
Comment 4 Marek Mahut 2008-08-28 16:34:10 EDT
(In reply to comment #2)
> Dude this just plainly sucks.
I told you it's not really ready yet ;)

(In reply to comment #3)
> No, seriously:
> 
> 1.) You use Patch tag with an index and %patch macro without one:
> 
> Patch0:         ircd-ratbox-2.2.8-offbyone.patch
> ...
> %patch -p1 -b .offbyone
>
> Either replace "Patch9" with "Patch" or "%patch" with "%patch0"

Typo - interesting that it works :)
 
> 2.) sysconfig/ircd has a redundant setting:
> 
> PIDFILE=/var/run/ircd.pid
> 
> The init script assigns a default value.
> You may want to remove this from sysconfid/ircd

Right, done.

> 3.) Useless comment in init script:
> 
> # Note: pidfile is assumed to be created
> # by ircd (config: server.pid-file).
> # If not, uncomment 'pidof' line.
> 
> Really, there's no "'pidof' line"

I agree, I think the author meant "remove" by "uncomment".

> 4.) Patch status
> 
> You added a fairly long and sophisticated patch. You should send it upstream
> accompany it with status comment.

The patch will be send upstream if it's approved :)

> 5.) Useless comment in SPEC file:
> 
> #make %{?_smp_mflags} -C contrib
> 
> Get rid of that. Or build contrib stuff and eventually make a subpackage?

No need for contrib.

> 6.) Hardcoded path
> 
> --with-logdir=/var/log/ircd
> ...
> mkdir -p $RPM_BUILD_ROOT/var/log ...
> 
> Replace with %{_localstatedir} (or %{_var}?)

%{_localstatedir} is /usr/var, but I can make it _var :)

> 7.) Own directories you create
> 
> Add %dir %{_sysconfdir}/ircd

Done.

> 8.) Add explaining comments to non-obvious commands:
> 
> mkdir -p ... $RPM_BUILD_ROOT/usr/local/ircd/
> 
> Why do you create a directory in /usr/local?

Left that from debugging, latest SRPM uploaded.

> mv $RPM_BUILD_ROOT%{_datadir}/ircd-old/modules
> $RPM_BUILD_ROOT%{_datadir}/ircd/modules
> rm -fr $RPM_BUILD_ROOT%{_datadir}/ircd-old
> 
> Why do you shuffle ircd-old around? What does it contain?

Nothing, upstream Makefile just create this directory to take a backup of previous data in ircd, which is useless if you're building it in mock.

> sed 's/-Werror//g' -i configure
> 
> Why do you strip this utterly useful compiler flag away?
> What's the status of upstreaming this fix?

We don't really need to stop at every warning, causes unwanted things, you know :) However, as I've already told you, other distributions with older compiler are using it frequently (openSUSE). I'll mention it in my mail to upstream once this package will be approved. 

New SRPM is available, thank you.
Comment 5 Lubomir Rintel 2008-08-29 02:28:08 EDT
Thanks, Well done!

APPROVED
Comment 6 Marek Mahut 2008-08-29 02:36:11 EDT
Thanks to you.

New Package CVS Request
=======================
Package Name: ircd-ratbox
Short Description: Ircd-ratbox is an advanced, stable and fast ircd
Owners: mmahut
Branches: F-9, EL-5
Comment 7 Kevin Fenzi 2008-08-30 16:47:17 EDT
cvs done.
Comment 8 Marek Mahut 2008-08-31 11:39:07 EDT
Thank you!

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