Bug 199919

Summary: Review Request: Asuka
Product: [Fedora] Fedora Reporter: Tim Niemueller <tim>
Component: Package ReviewAssignee: Hans de Goede <hdegoede>
Status: CLOSED NOTABUG QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: panemade
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2006-11-26 18:38:33 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 201449    
Attachments:
Description Flags
My version of SPEC file for asuka packaging none

Description Tim Niemueller 2006-07-24 11:15:30 UTC
Spec URL: http://www.niemueller.de/projects/extrpms/packages/fedora-extras-5/asuka.spec
SRPM URL: http://www.niemueller.de/projects/extrpms/packages/fedora-extras-5/asuka-1.2.1-1.i386.rpm
Description: UnderNet IRCd with QuakeNet Asuka Patches

Comment 2 Parag AN(पराग) 2006-07-25 03:51:57 UTC
No SRPM found on given SRPM URL

Comment 3 Tim Niemueller 2006-07-25 08:24:47 UTC
Please try again. It's there now.

Comment 4 Parag AN(पराग) 2006-07-25 09:12:30 UTC
rpmlint gave errors on SRPM as 

W: asuka strange-permission asuka-init 0755
A file that you listed to include in your package has strange
permissions. Usually, a file should have 0644 permissions.
==> I dont know why rpmlint reported warning here. This is correct

W: asuka hardcoded-packager-tag Tim
The Packager tag is hardcoded in your spec file. It should be removed, so
as to use rebuilder's own defaults.
==> No Packager tag is used in Fedoras SPECs. So remove it.

W: asuka redundant-prefix-tag
The Prefix tag is uselessly defined as %{_prefix} in your spec file. It
should be removed, as it is redundant with rpm defaults.
==>Don't define again %{_prefix} as its standard macro. 
Remove Prefix:    %{_prefix} line from SPEC


E: asuka use-of-RPM_SOURCE_DIR
You use $RPM_SOURCE_DIR or %{_sourcedir} in your spec file. If you have to
use a directory for building, use $RPM_BUILD_ROOT instead.
==> Don't use macro $RPM_SOURCE_DIR instead use $RPM_BUILD_ROOT

* Also, Change line from 
  BuildRoot: %{_tmppath}/%{name}-root 
   to   
  BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

* Provide Source URL to download source tarball.

* You don't need to have line
  mkdir -p $RPM_BUILD_ROOT

* You need to add dist tag to release

* try to use macros
You would like to see http://fedoraproject.org/wiki/Packaging/Guideline for more
information on packaging.

Comment 5 Paul Howarth 2006-07-25 09:22:33 UTC
(In reply to comment #4)
> rpmlint gave errors on SRPM as 
> 
> W: asuka strange-permission asuka-init 0755
> A file that you listed to include in your package has strange
> permissions. Usually, a file should have 0644 permissions.
> ==> I dont know why rpmlint reported warning here. This is correct

Not really. The initscript is just a regular source file and does not need to be
executable in the SRPM. It gets installed with the right permissions anyway
during the %install part of the build.

In addition to all of the other comments in Comment #4, I'd also suggest:
 * using "|| :" at the end of each command in the scriptlets (see the "Tips for
new recipes" at the bottom of http://fedoraproject.org/wiki/ScriptletSnippets)
 * If there is any chance of the package leaving behind files owned by the
userid created in this package (irc), don't remove the user and group when the
package is erased.


Comment 6 Parag AN(पराग) 2006-07-25 10:05:20 UTC
Created attachment 132969 [details]
My version of SPEC file for asuka packaging

Only use it for doing your own modification to asuka SPEC

Comment 7 Parag AN(पराग) 2006-07-25 10:15:39 UTC
*Remember above SPEC contains REAME in %doc tag
*I dont know how to install README to REDME.mkpasswd with %doc under %files

*Again Binary RPM reported some more rpmlint errors as
W: asuka wrong-file-end-of-line-encoding /usr/share/doc/asuka-1.2.1/snomask.html
This file has wrong end-of-line encoding, usually caused by creation or
modification on a non-Unix system. It could prevent it from being displayed
correctly in some circumstances.
==> remove non-unix characters by saving file under Linux

W: asuka wrong-file-end-of-line-encoding /usr/share/doc/asuka-1.2.1/p10.txt
This file has wrong end-of-line encoding, usually caused by creation or
modification on a non-Unix system. It could prevent it from being displayed
correctly in some circumstances.
==> remove non-unix characters by saving file under Linux

E: asuka incoherent-logrotate-file /etc/logrotate.d/ircd
Your logrotate file should be named /etc/logrotate.d/<package name>.
==>rpmlint suggests to use same logrotate filename as package name

W: asuka non-conffile-in-etc /etc/logrotate.d/ircd
A non-executable file in your package is being installed in /etc, but is not
a configuration file. All non-executable files in /etc should be configuration
files. Mark the file as %config in the spec file.
==>you included 
%config %{_sysconfdir}/ircd
chnage it to
%config(noreplace) %{_sysconfdir}/ircd

W: asuka dangerous-command-in-%post chown
W: asuka dangerous-command-in-%postun userdel
==> Maybe Paul would like to comment here

E: asuka no-chkconfig-line /etc/rc.d/init.d/ircd
The init script doesn't contain a chkconfig line to specify the runlevels
at which to start and stop it.
==>don't know why rpmlint gave error besides chkconfig is included in asuka-init

W: asuka incoherent-init-script-name ircd
The init script name should be the same as the package name in lower case.
==>as per rpmlint suggestions init-script-name must be same as package name in
lower case.


Comment 8 Hans de Goede 2006-07-27 11:21:16 UTC
Not a full review yet, first a few must fix items:
* You're using a hardcoded UID, very very BAD! Instead try this in a %pre script
groupadd -r irc || :
useradd -r -g irc -d /etc/ircd -n -c "IRC Daemon" irc || :
* Since this program will leave log files around after erase do not remove the 
 user and group on uninstall otherwise we get files owned by a non existing user 
 which is bad security wise
* "irc" as username and /etc/ircd as dir are way to generic and likely to
 collide with other packages, try asuka as username and /etc/asuka as dir.
* The same goes for the init and logrotate script names
* You're manually creating a %doc, thats not nescesarry add this to %prep:
cp tools/README doc/README.mkpasswd
 and add "%doc doc/*" to %files then the dir will get created and the files
 copied there by RPM.

Please post a new version with this fixed and taking the comments above into
account, then I'll do a full review.


Comment 9 Tim Niemueller 2006-08-08 00:25:27 UTC
A new package is available at
http://www.niemueller.de/projects/extrpms/packages/fedora-extras-5/asuka-1.2.1-2.src.rpm
(spec file at known place). I have incorporated most of the suggested changes.
One problem remains to be discussed:
E: asuka incoherent-logrotate-file /etc/logrotate.d/ircd
W: asuka incoherent-init-script-name ircd

As we see these both point to the same: name incoherency. As you can see the
binary is called ircd. So it is expected to be _the_ ircd on a system. The
reason for these names of the files is that the package contains the undernet
irc daemon with a specific Asuka patchset from Quakenet (which assumes
exclusiveness on the machine).

I saw that there is an ircd-hybrid in extras. It contains the very same files
(everything just named ircd). So  we have two options:
* it may be an option to rename the package to ircd-asuka and rename the other
files (and the user) appropriately. This should make it easy to distuinguish the
packages and install them side-by-side.
* Another way is to make the packages conflict. This makes sense since in
general you do not want two ircds on your system that you can confuse...

So this decision has to be made now. I'd vote for the latter way since this is
closer to what a asuka-user is known.

Comment 10 Hans de Goede 2006-08-11 05:22:20 UTC
(In reply to comment #9)
> One problem remains to be discussed:
> E: asuka incoherent-logrotate-file /etc/logrotate.d/ircd
> W: asuka incoherent-init-script-name ircd
> 
> As we see these both point to the same: name incoherency. As you can see the
> binary is called ircd. So it is expected to be _the_ ircd on a system. The
> reason for these names of the files is that the package contains the undernet
> irc daemon with a specific Asuka patchset from Quakenet (which assumes
> exclusiveness on the machine).
> 
> I saw that there is an ircd-hybrid in extras. It contains the very same files
> (everything just named ircd). So  we have two options:
> * it may be an option to rename the package to ircd-asuka and rename the other
> files (and the user) appropriately. This should make it easy to distuinguish the
> packages and install them side-by-side.
> * Another way is to make the packages conflict. This makes sense since in
> general you do not want two ircds on your system that you can confuse...
> 
> So this decision has to be made now. I'd vote for the latter way since this is
> closer to what a asuka-user is known.

I agree that this should be decided upon before this package gets included into
FE. But I'm not sure if I'm the one who should make this descission. I think it
would be best to discuss this on the fedora-extras-list.

I personally would prefer renaming the binary and init script instead of using a
Conflicts, a user should be able to install both if he wants to toy around. If
he then tries to start them both at the same port the latest one will fail,
which is kinda what he is asking for when doing things like that.

Also I believe that there is an unwritten policy that Conflicts should never be
used within FE.


Comment 11 Paul Howarth 2006-08-11 06:30:14 UTC
I think it might be worth getting together with the other ircd maintainer(s) in
Extras and working out how to use the "alternatives" system for ircd, much like
is done in Core for MTAs like sendmail, postfix etc.

Comment 12 Hans de Goede 2006-09-28 14:39:22 UTC
ping?


Comment 13 Hans de Goede 2006-11-19 07:14:16 UTC
ping again? If I still get no response within one week I'm closing this as a
dead review.