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 <[EMAIL PROTECTED]> 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. _______________________________________________ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
