Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=231861
Summary: Review Request: cyrus-imapd - high-performance mail server (IMAP, POP3, ...) Product: Fedora Extras Version: devel Platform: All OS/Version: Linux Status: NEW Severity: normal Priority: normal Component: Package Review AssignedTo: nobody@fedoraproject.org ReportedBy: tjanouse@redhat.com QAContact: fedora-package-review@redhat.com
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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: cyrus-imapd - high-performance mail server (IMAP, POP3, ...)
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=231861
bugzilla@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Severity|normal |medium Priority|normal |medium Product|Fedora Extras |Fedora
lkundrak@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- AssignedTo|nobody@fedoraproject.org |lkundrak@redhat.com Flag| |fedora-review?
------- Additional Comments From lkundrak@redhat.com 2007-06-28 14:46 EST ------- 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: cyrus-imapd - high-performance mail server (IMAP, POP3, ...)
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=231861
lkundrak@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: cyrus-imapd - high-performance mail server (IMAP, POP3, ...)
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=231861
------- Additional Comments From lkundrak@redhat.com 2007-06-28 14:49 EST ------- Created an attachment (id=158151) --> (https://bugzilla.redhat.com/bugzilla/attachment.cgi?id=158151&action=vie...) Rpmlint results for cyrus-imapd x86_64, source and subpackages
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: cyrus-imapd - high-performance mail server (IMAP, POP3, ...)
https://bugzilla.redhat.com/show_bug.cgi?id=231861
------- Additional Comments From lkundrak@redhat.com 2007-10-03 17:03 EST ------- Ping on this.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: cyrus-imapd - high-performance mail server (IMAP, POP3, ...)
https://bugzilla.redhat.com/show_bug.cgi?id=231861
tjanouse@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag| |fedora-cvs?
------- Additional Comments From tjanouse@redhat.com 2007-10-15 11:02 EST ------- Package Change Request ====================== Package Name: cyrus-imapd New Branches: F-8
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: cyrus-imapd - high-performance mail server (IMAP, POP3, ...)
https://bugzilla.redhat.com/show_bug.cgi?id=231861
kevin@tummy.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-cvs? |fedora-cvs+
------- Additional Comments From kevin@tummy.com 2007-10-15 11:29 EST ------- cvs done.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: cyrus-imapd - high-performance mail server (IMAP, POP3, ...)
https://bugzilla.redhat.com/show_bug.cgi?id=231861
tjanouse@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-cvs+ |fedora-cvs?
------- Additional Comments From tjanouse@redhat.com 2007-10-15 12:45 EST ------- 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...
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)?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: cyrus-imapd - high-performance mail server (IMAP, POP3, ...)
https://bugzilla.redhat.com/show_bug.cgi?id=231861
tjanouse@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-cvs? |
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: cyrus-imapd - high-performance mail server (IMAP, POP3, ...)
https://bugzilla.redhat.com/show_bug.cgi?id=231861
bugzilla@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Version|devel |rawhide
dan@danny.cz changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |dan@danny.cz
------- Additional Comments From dan@danny.cz 2008-05-26 10:18 EST ------- I am the new owner, so we can continue in the review.
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
Jay Turner jturner@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- AssignedTo|jturner@redhat.com |nobody@fedoraproject.org
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
Mads Kiilerich mads@kiilerich.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |mads@kiilerich.com
--- Comment #10 from Mads Kiilerich mads@kiilerich.com 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.
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 #11 from Michal Hlavinka mhlavink@redhat.com 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
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 #12 from Mads Kiilerich mads@kiilerich.com 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-d...), 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?
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 #13 from Jason Tibbitts tibbs@math.uh.edu 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.
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
Michal Hlavinka mhlavink@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-review? |
--- Comment #14 from Michal Hlavinka mhlavink@redhat.com 2009-10-30 10:33:51 EDT --- fixed, that flag was set by previous reviewer whose account has been closed
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
Jason Tibbitts tibbs@math.uh.edu changed:
What |Removed |Added ---------------------------------------------------------------------------- Summary|Review Request: cyrus-imapd |Merge Review: cyrus-imapd - |- high-performance mail |high-performance mail |server (IMAP, POP3, ...) |server (IMAP, POP3, ...)
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
Michal Hlavinka mhlavink@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |NEW
--- Comment #15 from Michal Hlavinka mhlavink@redhat.com 2009-11-25 06:36:08 EDT --- 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.
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 #16 from Mads Kiilerich mads@kiilerich.com 2009-11-25 20:30:31 EDT --- 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 ;-)
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 #17 from Michal Hlavinka mhlavink@redhat.com 2009-11-26 08:39:45 EDT --- (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.
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@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.
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
Michal Sekletar msekleta@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |msekleta@redhat.com AssignedTo|nobody@fedoraproject.org |msekleta@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 #19 from Michal Sekletar msekleta@redhat.com 2011-08-18 09:37:39 EDT --- Created attachment 518874 --> https://bugzilla.redhat.com/attachment.cgi?id=518874 cyrus-imapd fedpkg lint log
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
Michal Sekletar msekleta@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag| |fedora-review?
--- Comment #20 from Michal Sekletar msekleta@redhat.com 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
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 #21 from Michal Hlavinka mhlavink@redhat.com 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
package-review@lists.fedoraproject.org