Hide Forgot
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.
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.
Created attachment 158151 [details] Rpmlint results for cyrus-imapd x86_64, source and subpackages
Ping on this.
Package Change Request ====================== Package Name: cyrus-imapd New Branches: F-8
cvs done.
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)?
I am the new owner, so we can continue in the review.
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.
(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
>> 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?
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.
fixed, that flag was set by previous reviewer whose account has been closed
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.
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 ;-)
(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.
>> 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.
Created attachment 518874 [details] cyrus-imapd fedpkg lint log
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
(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