Bug 167258 - Review Request: up-imapproxy: University of Pittsburgh IMAP Proxy
Review Request: up-imapproxy: University of Pittsburgh IMAP Proxy
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Tom "spot" Callaway
David Lawrence
http://www.imapproxy.org
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2005-08-31 19:41 EDT by Jeff Carlson
Modified: 2007-11-30 17:11 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2005-09-12 08:06:50 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
New initscript for up-imapproxy (1.67 KB, text/plain)
2005-09-01 10:51 EDT, Paul Howarth
no flags Details
Patch to spec file (3.25 KB, patch)
2005-09-01 11:01 EDT, Paul Howarth
no flags Details | Diff
New init script (1.66 KB, text/plain)
2005-09-02 09:11 EDT, Jeff Carlson
no flags Details
imapproxy init script (1.66 KB, text/plain)
2005-09-09 08:33 EDT, Jeff Carlson
no flags Details

  None (edit)
Description Jeff Carlson 2005-08-31 19:41:28 EDT
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
Description: 
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
if possible.
Comment 1 Jeff Carlson 2005-08-31 19:49:39 EDT
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
installed.

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.
Comment 2 Paul Howarth 2005-09-01 10:49:45 EDT
(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
name?

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

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

Comment 3 Paul Howarth 2005-09-01 10:51:22 EDT
Created attachment 118343 [details]
New initscript for up-imapproxy

The attached initscript addresses most of the rpmlint issues (apart from name
coherency).
Comment 4 Paul Howarth 2005-09-01 11:01:18 EDT
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(post): /sbin/chkconfig
Requires(preun): /sbin/chkconfig, /sbin/service
Requires(postun): /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
silently
- Use "|| :" at the end of commands in scriptlets because RPM will abort
scripts in the event of errors
Comment 5 Ville Skyttä 2005-09-01 13:38:58 EDT
Jeff, FYI: you can run rpmlint with the -i/--info option and it will emit a 
more verbose explanation about about most warnings/errors. 
Comment 6 Matt Selsky 2005-09-02 05:22:27 EDT
Just FYI, UP = University of Pittsburgh, not University of Pennsylvania.
Comment 7 Jeff Carlson 2005-09-02 09:11:41 EDT
Created attachment 118391 [details]
New init script

Slightly modified init script in light of s/Pennsylvania/Pittsburgh/.
Comment 8 Jeff Carlson 2005-09-02 09:18:26 EDT
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.

http://www.ultimateevil.org/~jeff/Fedora/up-imapproxy-1.2.3-4.src.rpm
http://www.ultimateevil.org/~jeff/Fedora/up-imapproxy.spec
Comment 9 Paul Howarth 2005-09-02 12:45:53 EDT
I think this package should be reviewed by someone that actively uses IMAP,
which rules out me...
Comment 10 Rex Dieter 2005-09-02 13:10:16 EDT
In the specfile, when using the 'install' command, I'd suggest using the -p
option to preserve time stamps.
Comment 11 Jeff Carlson 2005-09-02 21:16:29 EDT
(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?
Comment 12 Toshio Kuratomi 2005-09-03 12:12:48 EDT
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.
Comment 13 Jeff Carlson 2005-09-03 19:57:22 EDT
(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.

http://www.ultimateevil.org/~jeff/Fedora/up-imapproxy-1.2.3-5.src.rpm
http://www.ultimateevil.org/~jeff/Fedora/up-imapproxy.spec
Comment 14 Tom "spot" Callaway 2005-09-08 17:53:40 EDT
Review:

- 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:

lockfile=/var/lock/subsys/imapproxy

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

APPROVED.
Comment 15 Jeff Carlson 2005-09-09 08:33:43 EDT
Created attachment 118632 [details]
imapproxy init script

One more correction to the init script attached to case.
Comment 16 Jeff Carlson 2005-09-09 10:04:08 EDT
I just saw that the upstream released a new version, right after I imported into
CVS, so...

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
standards:
 - 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:

http://www.ultimateevil.org/~jeff/Fedora/up-imapproxy-1.2.4-1.src.rpm
http://www.ultimateevil.org/~jeff/Fedora/up-imapproxy.spec

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.

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