Bug 181450
Summary: | Review Request: clamav-exim - Clam AV support files for Exim | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Tim Jackson <rpm> |
Component: | Package Review | Assignee: | Orion Poplawski <orion> |
Status: | CLOSED NOTABUG | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | dwmw2, will |
Target Milestone: | --- | ||
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2006-09-12 18:14:00 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | 190492 | ||
Bug Blocks: | 201449 |
Description
Tim Jackson
2006-02-14 09:35:47 UTC
- rpmlint checks return: W: clamav-exim no-url-tag W: clamav-exim no-documentation E: clamav-exim incoherent-logrotate-file /etc/logrotate.d/clamd.exim E: clamav-exim non-standard-uid /var/run/clamd.exim clamexim E: clamav-exim non-standard-gid /var/run/clamd.exim exim E: clamav-exim non-standard-dir-perm /var/run/clamd.exim 0750 E: clamav-exim non-standard-gid /var/log/clamd.exim exim E: clamav-exim non-root-group-log-file /var/log/clamd.exim exim W: clamav-exim dangerous-command-in-%post chmod ^ If you really need a log file, perhaps create in %install? E: clamav-exim init-script-name-with-dot /etc/rc.d/init.d/clamd.exim E: clamav-exim no-status-entry /etc/rc.d/init.d/clamd.exim W: clamav-exim no-reload-entry /etc/rc.d/init.d/clamd.exim E: clamav-exim subsys-not-used /etc/rc.d/init.d/clamd.exim W: clamav-exim incoherent-init-script-name clamd.exim - license (GPL) OK, need text in %doc. ? - No upstream, is this okay? - macro use not consistent between %var and %{var} Good: - package meets naming guidelines - package meets packaging guidelines - spec file legible, in am. english - package compiles on devel (x86) - no missing BR - no unnecessary BR - no locales - not relocatable - owns all directories that it creates - no duplicate files - %clean ok - code, not content - no need for -docs - nothing in %doc affects runtime - no need for .desktop file Thanks very much for the review. I should have mentioned that I knew rpmlint barfed lots, mostly due to this being a very weird package. > W: clamav-exim no-url-tag There isn't a relevant URL. > W: clamav-exim no-documentation There are no docs, though maybe I should write a short README. > E: clamav-exim incoherent-logrotate-file /etc/logrotate.d/clamd.exim Is this the filename (i.e. clamd.exim) that it's complaining about? If so, I know it's not the same as the package but the convention wasn't invented by me but does make sense. > E: clamav-exim non-standard-uid /var/run/clamd.exim clamexim > E: clamav-exim non-standard-gid /var/run/clamd.exim exim > E: clamav-exim non-standard-dir-perm /var/run/clamd.exim 0750 > E: clamav-exim non-standard-gid /var/log/clamd.exim exim I don't really understand these; the uid/gid/perms are intended and correct. > E: clamav-exim non-root-group-log-file /var/log/clamd.exim exim So, clamexim.root then? Howveer, users in the "exim" group might conceivably want read-access to the logs. > W: clamav-exim dangerous-command-in-%post chmod > ^ If you really need a log file, perhaps create in %install? I could do. However, this stuff all came from the original spec fragments that were checked into the clamav package by David Woodhouse (dwmw2@redhat). Given that David is more experienced than me I was deferring to his judgement. Any other thoughts from others would be welcome. > E: clamav-exim init-script-name-with-dot /etc/rc.d/init.d/clamd.exim See above discussion about the clamd.exim convention. > E: clamav-exim no-status-entry /etc/rc.d/init.d/clamd.exim > W: clamav-exim no-reload-entry /etc/rc.d/init.d/clamd.exim > E: clamav-exim subsys-not-used /etc/rc.d/init.d/clamd.exim These are all bogus considering that the init script is essentially just a pointer to the main clamav one. > W: clamav-exim incoherent-init-script-name clamd.exim See above discusion about the clamd.exim convention. > license (GPL) OK, need text in %doc. Well, there is no text "upstream" (because there isn't an upstream) so this isn't essential. > No upstream, is this okay? Well, there's no upstream to have, this package is really not much more than metadata for packaging consistency > macro use not consistent between %var and %{var} I'll review, though I didn't think there was a problem with %x and %{x} where used for clarity. Thanks again. (In reply to comment #2) > Thanks very much for the review. I should have mentioned that I knew rpmlint > barfed lots, mostly due to this being a very weird package. Yeah, we'll have to muddle through... :-) > > W: clamav-exim no-documentation > > There are no docs, though maybe I should write a short README. I think that would be good - explicitly stating the license as well as a copy of the GPL. > > E: clamav-exim incoherent-logrotate-file /etc/logrotate.d/clamd.exim > > Is this the filename (i.e. clamd.exim) that it's complaining about? If so, I > know it's not the same as the package but the convention wasn't invented by me > but does make sense. I think you need to use clamd-exim here and elsewhere (init.d). "." usually implies a meaningful suffix. > > E: clamav-exim non-standard-uid /var/run/clamd.exim clamexim > > E: clamav-exim non-standard-gid /var/run/clamd.exim exim > > E: clamav-exim non-standard-dir-perm /var/run/clamd.exim 0750 > > E: clamav-exim non-standard-gid /var/log/clamd.exim exim > > I don't really understand these; the uid/gid/perms are intended and correct. > Yeah, looks like this is what clamav does. > > E: clamav-exim non-root-group-log-file /var/log/clamd.exim exim > > So, clamexim.root then? Howveer, users in the "exim" group might conceivably > want read-access to the logs. Well, others seem to do this too, and seems sensible. > > W: clamav-exim dangerous-command-in-%post chmod > > ^ If you really need a log file, perhaps create in %install? > > I could do. However, this stuff all came from the original spec fragments that > were checked into the clamav package by David Woodhouse (dwmw2@redhat). Given > that David is more experienced than me I was deferring to his judgement. Any > other thoughts from others would be welcome. Okay, looks like this is what clamav spec does too. > > E: clamav-exim init-script-name-with-dot /etc/rc.d/init.d/clamd.exim > > See above discussion about the clamd.exim convention. Yeah, should be clamd-exim > > E: clamav-exim no-status-entry /etc/rc.d/init.d/clamd.exim > > W: clamav-exim no-reload-entry /etc/rc.d/init.d/clamd.exim > > E: clamav-exim subsys-not-used /etc/rc.d/init.d/clamd.exim > > These are all bogus considering that the init script is essentially just a > pointer to the main clamav one. > > > W: clamav-exim incoherent-init-script-name clamd.exim > > See above discusion about the clamd.exim convention. > > > license (GPL) OK, need text in %doc. > > Well, there is no text "upstream" (because there isn't an upstream) so this > isn't essential. > > > No upstream, is this okay? > > Well, there's no upstream to have, this package is really not much more than > metadata for packaging consistency I'll ask some questions on the list. OK, I've looked into this. The /usr/share/clamav/clamd-wrapper script, provided by clamav-server, hardcodes "clamd.XXX" as the service name in several places. So either we stick with clamd.exim as the service name or have to file a bug with Enrico and get him to patch the main ClamAV package. I presume there was some reason why clamd.XXX was chosen. Also, this package should possibly be renamed clamav-server-exim. I think you should bail on using clamd-wrapper. Use it as a base for a new clamav-exim (or clamav-server-exim) script, but change it so you can use the expected naming conventions for the init and logrotate scripts. If "clamd.SERVICE" truly is wrong, I think it should be fixed "upstream" in clamd-wrapper rather than just ignoring clamd-wrapper. I've emailed the Enrico offline to solicit his comments. (In reply to comment #6) > If "clamd.SERVICE" truly is wrong, I think it should be fixed "upstream" in > clamd-wrapper rather than just ignoring clamd-wrapper. I've emailed the Enrico > offline to solicit his comments. See also bug 190492 I've actually already added this support in a subpackage of exim: exim-clamav. I'll go ahead and close this out. |