[Bug 460538] Review Request: ircd-ratbox - Ircd-ratbox is an advanced, stable and fast ircd

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=460538





--- Comment #4 from Marek Mahut <mmahut at redhat.com>  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.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.