Bug 234926 - Review Request: ngircd - Next Generation IRC Daemon
Summary: Review Request: ngircd - Next Generation IRC Daemon
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Nigel Jones
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
: 228955 (view as bug list)
Depends On: 234882
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-04-02 21:20 UTC by Andreas Thienemann
Modified: 2008-02-11 17:40 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-02-11 17:40:45 UTC
Type: ---
Embargoed:
dev: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)
update to the writebuf patch (1.01 KB, patch)
2007-04-26 13:14 UTC, Nigel Jones
no flags Details | Diff

Description Andreas Thienemann 2007-04-02 21:20:55 UTC
Spec URL: http://home.bawue.de/~ixs/ngircd/ngircd.spec
SRPM URL: http://home.bawue.de/~ixs/ngircd/ngircd-0.10.1-2.src.rpm
Description:
ngIRCd is a free open source daemon for Internet Relay Chat (IRC), 
developed under the GNU General Public License (GPL). It's written from 
scratch and is not based upon the original IRCd like many others.

Comment 1 Kevin Fenzi 2007-04-02 21:26:56 UTC
Is this possibly a duplicate of: 
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=228955
?


Comment 2 Andreas Thienemann 2007-04-02 21:40:23 UTC
Uhh, right. I missed that one.

Okay, it's a dupe. I'll talk with Sean about it. His package doesn't look as
polished though. 

Comment 3 Andreas Thienemann 2007-04-02 22:08:29 UTC
*** Bug 228955 has been marked as a duplicate of this bug. ***

Comment 4 Nigel Jones 2007-04-23 09:43:52 UTC
If no one else is interested in reviewing, I'll try to take a look tomorrow.

(I notice libident is still waiting approval, so you'd have to fix that up first).

Comment 5 Nigel Jones 2007-04-26 12:09:00 UTC
Noone else seems to want to take a look, so assigning to myself.  I'll have a
review based on the current status of libident's review tomorrow, but until
libident is accepted I can't give a definate approval yet.

Sound okay Andreas?

Comment 6 Andreas Thienemann 2007-04-26 12:25:05 UTC
If it is any consolidation, I'll just remove the libident requirement.

I honestly do not care.

Comment 7 Nigel Jones 2007-04-26 12:39:32 UTC
In the end it's up to you, but I feel (as a seasoned IRC user) that libident
would  be useful in a package such as ngircd.

One possible work around is: provide a .spec file for ngircd without the
libident requirement, once ngircd and libident is approved, readd the
dependency, (after lots of checking, assuming all the static linking issues can
be sorted).

Comment 8 Andreas Thienemann 2007-04-26 12:48:37 UTC
Okay, new package without libident at the usual location.

have fun. :>

Comment 9 Nigel Jones 2007-04-26 13:14:05 UTC
Created attachment 153508 [details]
update to the writebuf patch

Comment 10 Nigel Jones 2007-04-26 13:16:48 UTC
Mock Log:
Installing /builddir/build/SRPMS/ngircd-0.10.1-3.fc7.src.rpm
Building target platforms: x86_64
Building for target x86_64
Executing(%prep): /bin/sh -e /var/tmp/rpm-tmp.97021
+ umask 022
+ cd /builddir/build/BUILD
+ LANG=C
+ export LANG
+ unset DISPLAY
+ cd /builddir/build/BUILD
+ rm -rf ngircd-0.10.1
+ /bin/gzip -dc /builddir/build/SOURCES/ngircd-0.10.1.tar.gz
+ tar -xf -
+ STATUS=0
+ '[' 0 -ne 0 ']'
+ cd ngircd-0.10.1
++ /usr/bin/id -u
+ '[' 509 = 0 ']'
++ /usr/bin/id -u
+ '[' 509 = 0 ']'
+ /bin/chmod -Rf a+rX,u+w,g-w,o-w .
+ echo 'Patch #0 (ngircd-writebuf-stupidity.diff):'
Patch #0 (ngircd-writebuf-stupidity.diff):
+ patch -p1 -s
missing header for unified diff at line 13 of patch
The text leading up to this was:
--------------------------
|ngircds ancient WRITEBUFFER_LEN legacy crap actively sabotages
|server connections (where a lot of stuff has to be sent at once)
|
|Work around this stupidity by tolerating pending writes of up to 512kb.
|
|Index: conn.c
|===================================================================
|RCS file: /srv/cvs/ngircd/ngircd/src/ngircd/conn.c,v
|retrieving revision 1.198.2.2
|diff -u -r1.198.2.2 conn.c
|--- conn.c     17 Dec 2006 23:06:29 -0000      1.198.2.2
|+++ conn.c     3 Apr 2007 19:34:00 -0000
--------------------------
File to patch:
Skip this patch? [y]
1 out of 1 hunk ignored
error: Bad exit status from /var/tmp/rpm-tmp.97021 (%prep)


RPM build errors:
    Bad exit status from /var/tmp/rpm-tmp.97021 (%prep)

The patch file does not work because of the additional text in the header, I
have updated the patch so that it should apply correctly.  (See Comment #9)

Comment 11 Nigel Jones 2007-04-28 00:00:16 UTC
Okay, here is the state of things:

I've done a rough review at the moment, but it the spec file needs some changes
first:

1. The diff needs replacing (See Comment #9 and Comment #10)
2. The dependency on tcp_wrappers is correct for fc6, but not f7 (devel), the
development libraries got moved to tcp_wrappers-devel.  I'm not totally sure how
other reviewers have counteracted this in the past, but my suggestion is for
review change the builddep to: /usr/include/tcpd.h this will counteract the
problem, the problem here is it puts increased demands on yum, so when you
commit to CVS you will need to alter the BR's to specific packages (tcp_wrappers
for FC-5 & 6, tcp_wrappers-devel for F7/devel)
(I have confirmed that this workaround works under mock okay)

Once the two changes are made, I should be able to build it fine without
altering the src.rpm myself, and should be able to give it a review and test it.

Comment 12 Andreas Thienemann 2007-04-28 09:25:30 UTC
Thanks, I'll change the patch accordingly.

About the tcp_wrappers requirement, the correct way of solving this is left up
to the developer.
Adding a file-dependency is what I do not want to do.

I might either have a different specfile for the devel tree or I'd use
conditionals like "%{fedora}" < "7" or something similar.
I haven't decided yet, but I guess I'll go with the former.

Comment 13 Nigel Jones 2007-04-28 10:59:04 UTC
(In reply to comment #12)
> Thanks, I'll change the patch accordingly.
> 
> About the tcp_wrappers requirement, the correct way of solving this is left up
> to the developer.
I agree, I was just suggesting one way.
> Adding a file-dependency is what I do not want to do.
I don't like file based dependencies myself to tell the truth.
> 
The reason why I suggested a file dependency for review only, is because once
reviewed, the spec file in FC6 can BR on tcp_wrappers and devel can BR on
tcp_wrappers-devel

Just for the sake of been complete with suggestions, the another appropriate way is:
As mentioned with the dependency altered to -devel, it can be built in RAWHIDE
obviously your using a FC-6 machine when creating packages (like myself), so
another method to settle the problem is to change the spec file with rawhide
compatible dependencies, and when testing on FC-6 use --nodeps.  Although not
specified in the PackageReviewGuidelines I'm pretty sure that most reviews agree
that packages must be buildable on the current devel at the very least.

The first solution I've provided will allow you build on both FC-6 and devel
while under review, the second will make the package only buildable on devel
during review, or with --nodeps on FC-6

As an after thought, depending on changes to the spec file, I know that it will
build on rawhide, and can approve a FC-6 compat spec file.


> I might either have a different specfile for the devel tree or I'd use
> conditionals like "%{fedora}" < "7" or something similar.
> I haven't decided yet, but I guess I'll go with the former.
I'd be happy to review a devel spec file, and a FC-6 spec file as another option

I've leave the choice up to you, but just remember, any workarounds for review
will be changed by the time it enters cvs.

Comment 14 David Woodhouse 2007-04-28 11:42:43 UTC
Package uses obsolete networking functions. Please update it to use the current
networking API.

See the 'Example Server Code' section at
http://people.redhat.com/drepper/userapi-ipv6.html 


Comment 15 Nigel Jones 2007-05-07 10:41:34 UTC
(In reply to comment #12)
> I might either have a different specfile for the devel tree or I'd use
> conditionals like "%{fedora}" < "7" or something similar.
> I haven't decided yet, but I guess I'll go with the former.
This will actually be the best way for the review.

(In reply to comment #14)
> Package uses obsolete networking functions. Please update it to use the current
> networking API.
> 
> See the 'Example Server Code' section at
> http://people.redhat.com/drepper/userapi-ipv6.html 
> 
Quite important in todays 'IPv6' Fedora, I've actually discussed this issue with
David and I think it is something that should be concerntrated on.  I've noticed
that upstream have neither said yay or nay regarding IPv6, so it might be an
idea to create a patch for this, and send upstream.

Comment 16 Nigel Jones 2007-05-23 00:04:27 UTC
Ping?

Is there any update regarding the status of ngircd?

Comment 17 Nigel Jones 2007-06-17 03:09:24 UTC
(In reply to comment #16)
> Ping?
> 
> Is there any update regarding the status of ngircd?

Ping #2, I havn't heard anything for the last 6 weeks or so, regarding this bug,
can you respond please.  I'm not too keen on closing this as a dead review per
http://fedoraproject.org/wiki/Extras/Policy/StalledReviews, so I would
appreciate any form or response.

Comment 18 Nigel Jones 2007-07-28 05:08:21 UTC
Package name:             PASS (ngircd)
License:                  PASS (GPL)
Spec Legible:             PASS (en_US)
md5sum matches:           PASS
rpmlint clean:            NOTES
Builds correctly:         PASS (i386)
RPaths removed:           PASS
Spec has %clean:          PASS
Macro use consistant:     PASS
Contains code/content:    PASS
-doc subpackage:          NA
-devel subpackage:        NA
-static subpackage:       NA
pkgconfig depend:         NA
Contains %doc:            PASS
Library suffix:           NA
No .la files:             NA
Use desktop-file-install: NA
No duplicate ownerships:  PASS
rm -rf %{buildroot}:      PASS
RPM uses valid UTF-8:     PASS
%defattr is set:          PASS
No duplicate %files:      PASS
Not relocatable:          PASS
Calls ldconfig:           PASS
Supports Locales:         NA
BR's are correct:         NOTES

I believe the tcp_wrappers issue still exists, so does the patch issue (please
fix at some point)

E: ngircd non-standard-gid /var/run/ngircd ngircd
E: ngircd non-standard-dir-perm /var/run/ngircd 0775
E: ngircd non-standard-gid /etc/ngircd.motd ngircd
E: ngircd non-readable /etc/ngircd.motd 0660
E: ngircd non-standard-gid /etc/ngircd.conf ngircd
E: ngircd non-readable /etc/ngircd.conf 0660
E: ngircd use-tmp-in-%pre

non-standard/non-readable messages can be ignored, use-tmp-in-%pre appears to be
a bug.

The IPv6 stuff isn't actually policy like I was led to believe, sorry for
holding it up though.  (Although you might want to file a bug blocking against
bug 195271 if you want).

Approved

Comment 19 Andreas Thienemann 2007-11-20 10:20:19 UTC
Ohhkay, this completely fell under the table.

Thanks for the approval, I'm pushing a current package into CVS now.

Comment 20 Andreas Thienemann 2007-11-20 15:05:36 UTC
New Package CVS Request
=======================
Package Name: ngircd
Short Description: Next Generation IRC Daemon
Owners: ixs
Branches: F-7 F-8 EPEL-5 EPEL-4
Cvsextras Commits: no

Comment 21 Kevin Fenzi 2007-11-20 18:48:06 UTC
cvs done. 

Note that there was a security alert in the last few days that all versions
except the current one are vulnerable to. Make sure you import the latest
upstream version.

Comment 22 Andreas Thienemann 2007-11-20 19:00:49 UTC
(In reply to comment #21)
> cvs done. 

Thx.

> Note that there was a security alert in the last few days that all versions
> except the current one are vulnerable to. Make sure you import the latest
> upstream version.

I know, the main coder is nearly (sitting) next to me. :)

Comment 23 Lubomir Kundrak 2008-01-16 06:03:52 UTC
Andreas, note that there is yet another one: CVE-2008-0285 (Bugzilla bug
#428935). So be sure to fix also that one. Thanks!

Comment 24 Andreas Thienemann 2008-02-11 17:40:45 UTC
checked in and build 0.11.0, fixing CVE-2008-0285.

thx for the review.


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