[Bug 231861] Merge Review: cyrus-imapd - high-performance mail server (IMAP, POP3, ...)

[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=231861





--- Comment #18 from Mads Kiilerich <mads at kiilerich.com>  2009-11-26 13:25:41 EDT ---
>> Shouldn't most of the renamings and other file hackings in %build be in %prep?
>>
> 
> fixed

How about "Fix permissions on perl programs"? And the removal of cvs files?

>> Why do %pre stop the service but pretend it is running?
>>
> 
> I guess it's for keeping condrestart working. I don't know why there is not
> just condrestart itself, but I thought it's because there were some race
> conditions. After searching through cvs history and filled bugs, I was not able
> to find any complain that condrestart alone does not work. removed

Right. It might be the same as bug 518753#c12 - perhaps it would be better to
add a sleep in restart?

>> Is the chattr in %post OK? It might be a good idea, but is it OK that a package
>> does it automatically? Shouldn't it be in README.RPM instead? Anyway, why not
>> just
>> chattr -R +S %{_var}/lib/imap/{user,quota} %{_var}/spool/imap
>>
> 
> afaik cyrus-imapd expect this behaviour and I don't think this makes any
> problems. Simplified

Yes, but that behaviour isn't observable by cyrus anyway and "only" serves to
"guarantee" the RFC requirement that mails must have reached the discs before
being accepted - and that can't be guaranteed that way anyway. 10 years ago it
was pushed as best practice by Brad Knowles & co, but I don't think it applies
in modern times. AFAICS neither postfix nor sendmail nor dovecot does the same.
I suggest asking some FS guru at RH.

>> Shouldn't the certificate creation be moved to service start where it is more
>> transparent what goes on, just like /etc/rc.d/init.d/sshd does?
> 
> well, would it make any real difference? For example dovecot does the same
> think. Yes, I know it's bad example since I'm its owner, but I didn't add that
> part in specs and don't know any other package that creates certificates after
> installation (except dovecot, cyrus-imapd and openssh).

Isn't it more like database initialization - which AFAIK is done in startup
script? 

I think the killer argument is that for live-images and appliances it is very
bad that the certificates is created at rpm-install-time.

>> Why is the user and group (and csync) created by the utils package %pre which
>> doesn't use them? Move to main package?
>>
> 
> well, just a little complicated here. cyrus-imapd requires utils and some of
> them are executed as cyrus user. But yes, user creation should be in main
> package. Moved

I noticed the "TODO: merge all sub-packages" - I assume that will solve it
anyway? (Even though IIRC some the tools are imap client tools and can be used
remotely.)


A couple of other nits:

Inconsistent use of xargs and -exec for file removal - shouldn't one of them be
used consistently?

"Generate db config file" isn't exactly legible ...

"[ -x /usr/lib/rpm/brp-compress ]" - isn't that a mandatory build requirement?

Bashisms in cyrus-imapd.init which is /bin/sh - that is very common, but I
guess it is bad?


I assume you will give a notice here when the attempt of getting patches
upstreamed has been concluded somehow.

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