Red Hat Bugzilla – Bug 167258
Review Request: up-imapproxy: University of Pittsburgh IMAP Proxy
Last modified: 2007-11-30 17:11:12 EST
Spec Name or Url: http://www.ultimateevil.org/~jeff/Fedora/up-imapproxy.spec
SRPM Name or Url: http://www.ultimateevil.org/~jeff/Fedora/up-imapproxy-1.2.3-3.src.rpm
imapproxy was written to compensate for webmail clients that are
unable to maintain persistent connections to an IMAP server. Most
webmail clients need to log in to an IMAP server for nearly every
single transaction. This behaviour can cause tragic performance
problems on the IMAP server. imapproxy tries to deal with this problem
by leaving server connections open for a short time after a webmail
client logs out. When the webmail client connects again, imapproxy
will determine if there's a cached connection available and reuse it
Before this gets reviewed, I want to ask about rpmlint output:
W: up-imapproxy incoherent-version-in-changelog up-imapproxy-1.2.3-3 1.2.3-3
I don't understand this one. This changelog is just like the last package I
submitted, which was fine. Is it because of the additional '-' in the package name?
E: up-imapproxy init-script-without-chkconfig-postin /etc/rc.d/init.d/imapproxy
Um, I thought it was a bad idea to enable a service just because it is being
E: up-imapproxy no-status-entry /etc/rc.d/init.d/imapproxy
I didn't write the init script. It is packaged in the source. I could write
another one if it is required.
W: up-imapproxy no-reload-entry /etc/rc.d/init.d/imapproxy
See above comment.
W: up-imapproxy service-default-enabled /etc/rc.d/init.d/imapproxy
I don't know what this means.
E: up-imapproxy subsys-not-used /etc/rc.d/init.d/imapproxy
I think I might know what this means, but I could be wrong. However, again, I
didn't write the init script.
W: up-imapproxy incoherent-init-script-name imapproxy
Is this because the init script is "imapproxy" and the package is
"up-imapproxy?" This seems rather trivial, especially since "up" just means
"University of Pennsylvania." The source tarball is "up-imapproxy," the daemon
is "in.imapproxy," and the service is called "imapproxy." I don't think this is
a significant issue.
(In reply to comment #1)
> Before this gets reviewed, I want to ask about rpmlint output:
> W: up-imapproxy incoherent-version-in-changelog up-imapproxy-1.2.3-3 1.2.3-3
> I don't understand this one. This changelog is just like the last package I
> submitted, which was fine. Is it because of the additional '-' in the package
Just use the epoch/version/release numbers, don't include the package name at all.
> E: up-imapproxy init-script-without-chkconfig-postin /etc/rc.d/init.d/imapproxy
> Um, I thought it was a bad idea to enable a service just because it is being
It wants you to do a "chkconfig -add" in %post; this doesn't necessarily enable
the service - if you have "-" for the default runlevels then the service will be
disabled by default.
> E: up-imapproxy no-status-entry /etc/rc.d/init.d/imapproxy
> I didn't write the init script. It is packaged in the source. I could write
> another one if it is required.
> W: up-imapproxy no-reload-entry /etc/rc.d/init.d/imapproxy
> See above comment.
The initscript in the source is rather old-fashioned. I'll attach a new one to
this report shortly.
> W: up-imapproxy service-default-enabled /etc/rc.d/init.d/imapproxy
> I don't know what this means.
The chkconfig line in the initscript specifies default runlevels of 2345, so a
"chkconfig -add imapproxy" will enable the service in those runlevels.
> E: up-imapproxy subsys-not-used /etc/rc.d/init.d/imapproxy
> I think I might know what this means, but I could be wrong. However, again, I
> didn't write the init script.
The initscript doesn't track state by creating a /var/lock/subsys/in.imapproxyd
file when running.
> W: up-imapproxy incoherent-init-script-name imapproxy
> Is this because the init script is "imapproxy" and the package is
> "up-imapproxy?" This seems rather trivial, especially since "up" just means
> "University of Pennsylvania." The source tarball is "up-imapproxy," the daemon
> is "in.imapproxy," and the service is called "imapproxy." I don't think this is
> a significant issue.
Neither do I. You can't completely get rid of the incoherency because the
package is called up-imapproxy and the actual daemon is called in.imapproxyd, so
I'd leave this as it is.
More suggestions coming soon...
Created attachment 118343 [details]
New initscript for up-imapproxy
The attached initscript addresses most of the rpmlint issues (apart from name
Created attachment 118347 [details]
Patch to spec file
The attached patch addresses the remaining rpmlint issues, plus a few others:
- BuildReqs e2fsprogs-devel, krb5-devel, and zlib-devel are deps of BuildReq
openssl-devel and can therefore be omitted
- Additional BuildReq ncurses-devel is needed
- Instead of "Requires: /sbin/chkconfig /sbin/service", more finely-tuned
dependencies can be used for scriptlets:
Requires(preun): /sbin/chkconfig, /sbin/service
- Don't change/set ownership of files in %install, do it using attributes in
%files instead; otherwise, packages can't be built by non-root users who can't
do file giveaways
- You could use a %ghost /var/run/pimpstats file so that the stats file gets
removed if the package is uninstalled
- Use a %post script to add the service, and a %postun script to restart the
(new version of the) service after an upgrade
- Redirect output of /sbin/service to Dave Null so that the scripts operate
- Use "|| :" at the end of commands in scriptlets because RPM will abort
scripts in the event of errors
Jeff, FYI: you can run rpmlint with the -i/--info option and it will emit a
more verbose explanation about about most warnings/errors.
Just FYI, UP = University of Pittsburgh, not University of Pennsylvania.
Created attachment 118391 [details]
New init script
Slightly modified init script in light of s/Pennsylvania/Pittsburgh/.
Although I didn't actually apply Paul's patch directly, I believe I addressed
all the suggestions he made. So here's a new SRPM and spec.
I think this package should be reviewed by someone that actively uses IMAP,
which rules out me...
In the specfile, when using the 'install' command, I'd suggest using the -p
option to preserve time stamps.
(In reply to comment #9) (Paul)
> I think this package should be reviewed by someone that actively uses IMAP,
> which rules out me...
Perhaps someone who also uses SquirrelMail would be ideal.
(In reply to comment #10) (Rex)
> In the specfile, when using the 'install' command, I'd suggest using the -p
> option to preserve time stamps.
Preserving the time stamp of a binary which was just now compiled doesn't seem
to matter. Also that of the config file which will be edited before the program
is used also seems unnecessary. That just leaves the init script, which I could
do post CVS import.
Tom, are you going to be the one to approve this one like last time?
I can see the logic of the binary. The config file should have timestamp
preserved, though. That way the base config file will have the same timestamp
when the package is upgraded.
(In reply to comment #12) (Toshio)
> I can see the logic of the binary. The config file should have timestamp
> preserved, though. That way the base config file will have the same timestamp
> when the package is upgraded.
Ok, I have added -p to install on the conf and init files. New package built.
- rpmlint checks:
E: up-imapproxy incoherent-subsys /etc/rc.d/init.d/imapproxy in.imapproxyd
W: up-imapproxy incoherent-init-script-name imapproxy
You don't have a subsys lock file. Adding something like:
to your initscript will fix that first error.
The other warning is safe to ignore.
- package meets naming guidelines
- package meets packaging guidelines
- license (GPL) OK, text in %doc, matches source
- spec file legible, in am. english
- source matches upstream
- package compiles on devel (x86)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file
Created attachment 118632 [details]
imapproxy init script
One more correction to the init script attached to case.
I just saw that the upstream released a new version, right after I imported into
I bumped my spec file version.
I found they added a new doc, added.
I found they updated their included RPM spec, but it still doesn't meet Fedora
- Doesn't include the license in doc.
- Resets _prefix to /usr/local (violates FHS, IMO).
The new version's init file is unchanged, so I'm keeping the one Paul gave me.
My updates are here:
If there are no complaints, then I assume it's still approved, and I'll build as
soon as my requested branches are in CVS.