Bug 231861 - Merge Review: cyrus-imapd - high-performance mail server (IMAP, POP3, ...)
Merge Review: cyrus-imapd - high-performance mail server (IMAP, POP3, ...)
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Michal Sekletar
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-03-12 13:01 EDT by Radek Vokal
Modified: 2013-03-19 15:50 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-03-19 15:50:47 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
msekleta: fedora‑review?


Attachments (Terms of Use)
Rpmlint results for cyrus-imapd x86_64, source and subpackages (4.70 KB, text/plain)
2007-06-28 14:49 EDT, Lubomir Kundrak
no flags Details
cyrus-imapd fedpkg lint log (4.28 KB, text/plain)
2011-08-18 09:37 EDT, Michal Sekletar
no flags Details

  None (edit)
Description Tomas Janousek 2007-03-12 13:01:36 EDT
http://cvs.fedora.redhat.com/viewcvs/devel/cyrus-imapd/?root=extras

Seems like this package got into Extras without a review. Asking for it then.
Comment 1 Lubomir Kundrak 2007-06-28 14:46:47 EDT
I'll take this for the review. The package is in Fedora already and
proved that it is in solid and usable state. Just a formal review and few
notes follow:

* specfile and the package properly named
* BSD license matches reality and is legally ok
* specfile is legible (to some extent :) and text is written in american english
* sources match upstream:
     ac03b02c1ae08d52f807b58c488b204f  cyrus-imapd-2.3.8.tar.gz
     8f7a26b0556369827bb5c8084a3e3ea1  cyrus_sharedbackup-0.1.tar.gz
* builds supported architectures
* dependencies list seems to be fine
* no locales
* no shared libraries
* not relocatable
* %files section is all ok, so is the ownership of directories
! The %clean section contains old construct
* couldn't find an instance of inconsistent use of macros
* package is program code
! relatively large amount of documentation is not split into a subpackage
* header files and static libraries are in -devel subpackage
! static libraries are present
* no pkgconfig stuff, nor anything else that would go into a -devel subpackage
* devel does not depend on base, but that's okay since it contains no shared
libraries, just headers (see below regarding the static libs).
* no libtool archives
* not a gui application
! the subtle problem with %clean also applies to %install
-----

1.) In %clean section, please remove the test from

  [ "%{buildroot}" != "/" ] && %{__rm} -rf %{buildroot}

it is not needed, as you set the build root here:

  BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}-root

The same applies for %install section.

2.) Please consider spliting of the html documentation into the -doc
subpackage, though it is largely dependent on your personal appeal.
Also, I think the html versions of the manual pages should not be packaged
as they are redundant -- please remove them.

3.) Please try to avoid static libraries. Does anything use these?

./usr/lib64/libcyrus.a
./usr/lib64/libcyrus_min.a

4.) From the rpmlint result (the whole dump is attached):

W: cyrus-imapd conffile-without-noreplace-flag /etc/cron.daily/cyrus-imapd
W: cyrus-imapd conffile-without-noreplace-flag /etc/logrotate.d/cyrus-imapd
W: cyrus-imapd conffile-without-noreplace-flag /etc/rc.d/init.d/cyrus-imapd

Tomas, please fix this -- those should not be marked as config files.
Though I believe other rpmlint warnings to be relatively harmless, please
at least skim through them. You might well want to silence some of those (as
some are trivially easy to fix.)

Also, sed might be better suited for what you do with perl in %post.
Comment 2 Lubomir Kundrak 2007-06-28 14:49:11 EDT
Created attachment 158151 [details]
Rpmlint results for cyrus-imapd x86_64, source and subpackages
Comment 3 Lubomir Kundrak 2007-10-03 17:03:44 EDT
Ping on this.
Comment 4 Tomas Janousek 2007-10-15 11:02:12 EDT
Package Change Request
======================
Package Name: cyrus-imapd
New Branches: F-8
Comment 5 Kevin Fenzi 2007-10-15 11:29:15 EDT
cvs done.
Comment 6 Tomas Janousek 2007-10-15 12:45:17 EDT
Lubo, Thanks for your comments and sorry for the delay. Me not like reviews, you
know.

I did a few changes as you suggested:
http://cvs.fedora.redhat.com/viewcvs/devel/cyrus-imapd/cyrus-imapd.spec?root=extras&r1=1.32&r2=1.33

And I have a few comments:
Ad 2). I'd like to leave the HTML docs in the main package, but you may try to
convince me. I don't think we should delete the HTML manpages though, because
they are linked from the HTML docs.

Ad 3). As you pointed out, there are no shared libs in the package. libcyrus is
intended to be linked statically and that's what is in the -devel subpackage.

Ad 4). I fixed these and about 1 other, will maybe fix others later.


There has been a change in the License field since, re-review this, please.

And, the most important thing:
This package has a whole lot of files in /usr/lib/cyrus-imapd, regardless of the
architecture. This used to be a reason for marking it multiarch and warning
about multiarch collisions. It's been added to a list of exceptions since then.
But the question remains -- should I leave it that way, or should I make some
effort to move those to /usr/libexec (or something similar)?
Comment 7 Dan Horák 2008-05-26 10:18:24 EDT
I am the new owner, so we can continue in the review.
Comment 10 Mads Kiilerich 2009-10-27 14:11:02 EDT
Is this package still ready for review? (And shouldn't it be a Merge Review?) How can it be that it has status Assigned but isn't assigned to anybody?

My first impression is that it looks like it haven't been dressed up for examn and could use some polishing before a final review.

Some brief comments:

It seems like most (all?) of the code now is licensed announcement-BSD-ish, so the License and the comments about it are a bit misleading.

The spec is quite complex and verbose and IMHO not easy to read.

The spec contains comments left over from the Invoca version.

_perlhack variable seems to be unused since 7.3 - Red Hat, not Fedora!

There are manu variables and configuration options. Are they necessary and used?

The %file specs are very explicit and verbose. Is that intentional and necessary? (And %{_contribdir} is listed twice.)

Rpmlint says
    6 packages and 1 specfiles checked; 20 errors, 61 warnings.
Some of the warnings might be invalid, but some of them definitely should be adressed before review.

The spec has 30 sources and 15 patches without any indication if they have been pushed upstream.
Comment 11 Michal Hlavinka 2009-10-29 11:49:54 EDT
(In reply to comment #10)
> Is this package still ready for review? 

somehow :o)

> (And shouldn't it be a Merge Review?)

definitely 

> How can it be that it has status Assigned but isn't assigned to anybody?

afaict, it seems it was assigned to someone whose account has been closed

> My first impression is that it looks like it haven't been dressed up for examn
> and could use some polishing before a final review.

sure, I've inherited this package and it required a *lot* of polishing. But after a few rounds it got lower and lower in my todo-list, especially because there was no reviewer.

> Some brief comments:
> 
> It seems like most (all?) of the code now is licensed announcement-BSD-ish, so
> the License and the comments about it are a bit misleading.

fixed

> 
> The spec is quite complex and verbose and IMHO not easy to read.

I agree

> The spec contains comments left over from the Invoca version.

fixed

> _perlhack variable seems to be unused since 7.3 - Red Hat, not Fedora!

yes, this was leftover, I've removed all perlhack ifdefs some time ago

removed

> 
> There are manu variables and configuration options. Are they necessary and
> used?

this is what I can't even guess actually. I'd definitely like to get rid of all those switches, but I don't want to break it for someone... Well, I've just though about removing them in new rawhide and wait if someone complains.

and since devel is future rawhide now, I've removed them

> 
> The %file specs are very explicit and verbose. Is that intentional and
> necessary? (And %{_contribdir} is listed twice.)

second _contribdir removed

it seems to me there is quite a lot of space for improvement since attributes do not need to be specified twice (install in %install and %files), I'll look at this.

> Rpmlint says
>     6 packages and 1 specfiles checked; 20 errors, 61 warnings.
> Some of the warnings might be invalid, but some of them definitely should be
> adressed before review.

I've lowered the number a little for now 

> The spec has 30 sources and 15 patches without any indication if they have been
> pushed upstream.  

Afaik they were rejected

some sources are additional modules/tools that upstream is not interested in

----------------------------------

This is first round and definitely not finished. Just to show you there is someone on the other end. I'll continue with this on monday

-----------------------------------

changes were only commited, not tagged yet, you find actual (not finished) spec in cvs
Comment 12 Mads Kiilerich 2009-10-29 12:30:49 EDT
>> The spec has 30 sources and 15 patches without any indication if they have been
>> pushed upstream.  
> 
> Afaik they were rejected

Probably. I worked a bit on packaging 10 (hmm ... scary!) years ago (http://www.rpmfind.net/linux/RPM/conectiva/atualizacoes/8/RPMS/cyrus-imapd-devel-static-2.0.17-1U80_1cl.i386.html), and back then upstream wasn't that "open". But the license change might be an indication that things have changed now?

> some sources are additional modules/tools that upstream is not interested in

That might be. But there are so many of them that it almost deserves a real home. Fedora CVS is not a good upstream. Perhaps upstream could be convinced to carry it in its contrib folder?
Comment 13 Jason Tibbitts 2009-10-29 13:00:54 EDT
I'm glad to see some progress with cyrus-imapd, or any merge review for that matter, but is anyone actually reviewing this?  fedora-review is set to '?' but the ticket isn't assigned to anyone.
Comment 14 Michal Hlavinka 2009-10-30 10:33:51 EDT
fixed, that flag was set by previous reviewer whose account has been closed
Comment 15 Michal Hlavinka 2009-11-25 06:36:08 EST
another round completed

> 6 packages and 1 specfiles checked; 20 errors, 61 warnings.

reduced to 17/17, but hitting the ground here, remaining W/E are because of cyrus user and some non-world readable directories


there is still some work to be done, but I think it's ready for some initial comments from any reviewer.

The most important remaining part is looking into deep dark past and trying to find out something about that ancient patches/additional sources. (I'm accepting crystal balls as gifts ;) I'll try (again) to send patches upstream when I know more about them.
Comment 16 Mads Kiilerich 2009-11-25 20:30:31 EST
Ok, another round of comments/questions:

MIT license? AFAICS the COPYRIGHT file is BSD. The CMU entry on http://fedoraproject.org/wiki/Licensing is internally inconsistent, and http://fedoraproject.org/wiki/Licensing/MIT#CMU_Style isn't the cyrus license - AFAIK that license has never applied to cyrus.

_cyrusconf seems like a bit of overkill.

Haven't all the versions in the requirements been irrelevant for ages?

Shouldn't most of the renamings and other file hackings in %build be in %prep?

Why this "if pkg-config openssl" thing? When and why does it apply?

pushd before "perl -pi" and use of "$(ls *x)" isn't needed - I would prefer the simple version

It seems like there is no need for cleanup of "*~" and "*.html.*".

Why do %pre stop the service but pretend it is running?

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

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?

Why is the user and group (and csync) created by the utils package %pre which doesn't use them? Move to main package?

Wouldn't it be better if csync were included in the setup packages /etc/services?

Is it relevant to include html versions of the man pages?

Much of README.RPM is outdated or irrelevant. I think "chkconfig cyrus-imapd on" should be mentioned. README.buildoptions doesn't exist. And postfix shouldn't be discriminated ;-)
Comment 17 Michal Hlavinka 2009-11-26 08:39:45 EST
(In reply to comment #16)
> Ok, another round of comments/questions:
> 
> MIT license? AFAICS the COPYRIGHT file is BSD. The CMU entry on
> http://fedoraproject.org/wiki/Licensing is internally inconsistent, and
> http://fedoraproject.org/wiki/Licensing/MIT#CMU_Style isn't the cyrus license -
> AFAIK that license has never applied to cyrus.

oops, you're right, it's BSD license

fixed

> _cyrusconf seems like a bit of overkill.

this was here to set config file version (prefork/no-prefork), but we did not change that for ages, removed

fixed

> 
> Haven't all the versions in the requirements been irrelevant for ages?
> 

verified and removed

fixed

> Shouldn't most of the renamings and other file hackings in %build be in %prep?
> 

fixed

> Why this "if pkg-config openssl" thing? When and why does it apply?
> 

(afaik) this was there because there were problems with makefiles, but it's not needed now, removed

fixed

> pushd before "perl -pi" and use of "$(ls *x)" isn't needed - I would prefer the
> simple version
> 

fixed

> It seems like there is no need for cleanup of "*~" and "*.html.*".
> 

fixed

> 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

fixed

> 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

fixed

> 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).

> 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

fixed

> Wouldn't it be better if csync were included in the setup packages
> /etc/services?
> 

I've asked setup maintainer to add csync in /etc/services

( http://git.fedorahosted.org/git/?p=setup.git;a=summary )

waiting for new setup release

> Is it relevant to include html versions of the man pages?

they are making no harm, but yes, they are not necessary, removed.

fixed

> 
> Much of README.RPM is outdated or irrelevant. I think "chkconfig cyrus-imapd
> on" should be mentioned. README.buildoptions doesn't exist. And postfix
> shouldn't be discriminated ;-)  

this file seems outdated and not too useful for Fedora, removed

fixed

--------
btw, there is "discussion" on mailing list where someone asked for including autocreate patch in upstream's repository. I've added there my "me too". Watching for upstream's decision.
Comment 18 Mads Kiilerich 2009-11-26 13:25:41 EST
>> 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.
Comment 19 Michal Sekletar 2011-08-18 09:37:39 EDT
Created attachment 518874 [details]
cyrus-imapd fedpkg lint log
Comment 20 Michal Sekletar 2011-08-18 09:39:11 EDT
Git commit
6fe53495447ec5cf6bb6bf697335c732f839f81e
YES source file match upstream
YES package meets naming and versioning guidelines
YES spec file is properly named, is cleanly written and uses macros consistently
YES dist tag is present
YES clean section and buildroot present
YES license field matches the actual license
YES license is included in package
YES latest version is being packaged
YES build BuildRequires are proper
YES compiler flags are appropriate
YES package builds in mock
YES debuginfo package seems ok
NO  rpmlint is silent
 - some spelling-error warnings could be ignored
 - %patch1 and %patch2 are commented out but still in preamble of spec file
 - invalid patch url (Patch3)
 - some warnings about missing manpages for utils - ignored
 - warnings about directory permissions - ignored
 - warnings about hardcoded library paths - might be solved by replacing 
   hard-coded paths with macros
 - private-shared-objects-provides warnings
 - rpmlint output was consulted with maintainer, added as attachment

YES final provides and requires look sane
N/A %check is present and all tests pass
YES no shared libraries are added to regular linker search paths
YES it owns directories it creates
YES it doesn't own directories it shouldn't
YES no duplicates in %files
YES scriptlets looks sane
YES code, not content
YES large documentation must go in a -doc subpackage
 - documentation is about 1MB in size, not big enough that it is worthed
   to make standalone doc package 
YES %docs are not necessary for the proper functioning of the package
YES no headers
YES no pkgconfig files
YES no libtool .la droppings
YES not a GUI app
Comment 21 Michal Hlavinka 2012-03-22 06:37:47 EDT
(In reply to comment #20)
>  - %patch1 and %patch2 are commented out but still in preamble of spec file

fixed

>  - invalid patch url (Patch3)

url is correct:
go to http://www.oakton.edu/user/3/jwade/cyrus/cyrus-imapd-2.1.3/
and you will see that patch file, but if you click it, you'll get 404 error, probably some problem on their server

>  - warnings about hardcoded library paths - might be solved by replacing 
>    hard-coded paths with macros

fixed

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