[Bug 578480] Review Request: spectrum - XMPP transport/gateway

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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=578480

--- Comment #18 from Michal Schmidt <mschmidt@xxxxxxxxxx> 2010-05-31 10:06:20 EDT ---
(In reply to comment #17)
> RHEL-5 http://koji.fedoraproject.org/koji/taskinfo?taskID=2208854    

In the spec file:
> %{!?_initddir: %define _initddir %{_initrddir}}

I see you're firmly set on having the spec file compatible with RHEL-5 by using
a lot of conditionals, thus making the spec slightly more ugly than necessary
in the F-* branches, and at the same time you want to use the clean modern name
"_initddir" which in the end only makes you use more tricks like that. If you
want to be compatible, just stick with the old "_initrddir" - it's still used
in many Fedora spec files

> License: GPLv2+ and GPLv3+

This would benefit from a comment describing which parts are under which
licence.

> Patch0: spectrum-without-gettext.patch
> Patch1: spectrum-0.2-cmake.patch

All patches should have a comment (in the spec or in the patch files
themselves) describing their purpose and upstream status.

> %if 0%{?rhel} <= 5
> ExcludeArch: ppc
> %endif

First of all: Why is ppc excluded? Is it a workaround for a bug or an inherent
incompatibility of the software with the arch?
And second: The condition will be true on any Fedora. Though ppc is not a
primary arch in Fedora anymore, secondary arch efforts should be still
possible.

> %if 0%{?rhel} <= 5 && 0%{?fedora} < 7
> Users of RHEL-5 Server need to have RHEL Optional Productivity Apps channel
> switched on in order to get libpurple.
> %endif

The text is very specific to RHEL-5. Wouldn't then "%if 0{?el5}" do what you
want to achieve?
The same condition is used a few lines later again.

> cat <<EOS >spectrum-logrotate
> /var/log/spectrum/*.log {
>     rotate 5
>     size 1M
>     missingok
>     notifempty
> }
> EOS

I usually prefer adding such files as another Source. Whatever.

> # To work around these issues in 'make install':
> # - spectrum.cfg contains secret and should not be world readable...
> # - it wont install $DESTDIR/etc/spectrum/spectrum.cfg if /etc/spectrum/spectrum.cfg exists. (would not be included in rpm)
> # - installs spectrumctl withoud read permissions, so it cannot be copied to rpm.

Are these problems known to upstream?
BTW, "withoud" is a typo.

> %config(noreplace) %{_sysconfdir}/spectrum/spectrum.cfg
> %config(noreplace) %{_sysconfdir}/logrotate.d/spectrum

You should own the /etc/spectrum directory.
You should Require logrotate because you do not own /etc/logrotate.d (and
should not own it).

-- 
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.
_______________________________________________
package-review mailing list
package-review@xxxxxxxxxxxxxxxxxxxxxxx
https://admin.fedoraproject.org/mailman/listinfo/package-review


[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]