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.
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.
(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...
Created attachment 118343 [details] New initscript for up-imapproxy The attached initscript addresses most of the rpmlint issues (apart from name coherency).
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
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. http://www.ultimateevil.org/~jeff/Fedora/up-imapproxy-1.2.3-4.src.rpm http://www.ultimateevil.org/~jeff/Fedora/up-imapproxy.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. http://www.ultimateevil.org/~jeff/Fedora/up-imapproxy-1.2.3-5.src.rpm http://www.ultimateevil.org/~jeff/Fedora/up-imapproxy.spec
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.
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 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.