Bug 185535
Summary: | Review Request: lurker | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | John Dennis <jdennis> |
Component: | Package Review | Assignee: | Nobody's working on this, feel free to take it <nobody> |
Status: | CLOSED NOTABUG | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | gauret, j |
Target Milestone: | --- | Flags: | j:
fedora-review-
|
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2007-02-22 19:23:21 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: |
Description
John Dennis
2006-03-15 16:56:57 UTC
Some quick comments: Please use a complete URL in Source0 so that spectool can automatically fetch the source. If Sourceforge wasn't throwing "Internal server error" at the moment I'd give more detail (and I could test out a build of the package). gcc-c++ is not permitted in BuildRequires:. I imagine that RPM will figure out what you have in Requires:, but I can't verify that at the moment. It's much cleaner to refer to %{SOURCE1} rather than $RPM_SOURCE_DIR/lurker-httpd.conf.in. There's rarely a good reason to ever use $RPM_SOURCE_DIR. Is %{buildsubdir} guaranteed to be defined? I think that world-writable directory is going to be a blocker. If the end user wants to open the permissions up, that would be their business. I think modern MTAs can be configured to deliver as the appropriate user so this shouldn't be a big deal in practise. You define %{mta_owner} and %{mta_group} but don't reference them anywhere. I find that defining a macro for something that's referenced only once in the spec (like, say, %{httpdconffile}) is a bit confusing, but that's just personal taste. BTW, recent guideline changes permit gcc-c++ in BuildRequires:, even though it's probably cleaner to leave it out. Also, the current version seems to be 2.1; there are what look to be serious security implications for any version older than 2.1. (Sorry for spamming, but I was starting on a review when I noticed.) Thank you for your careful review Jason and your thoughtful comments. I'm sorry I haven't responded sooner but I've got higher priority tasks than lurker packaging which have captured my attention. FYI, it was my plan to address the issue of a world writable database files by adding the owner and group to the configure script and passing it configure via %{mta_owner} and %{mta_group}. I do realized it is not strictly required to have this be available to configure because the RPM %files section can enforce the permissions but it is my belief that making this explicit is a win in terms of readability, upstream use, and those who may use configure outside the context of rpm. This is why these two variables appeared in the spec file, once configure could accept them they would be passed and set in the %files section. As for modern MTA configuration with respect to mail delivery I can attest as the previous maintainer for postfix, dovecot and mailman this is an area of endless user confusion :-( (Did you know postfix will deliver under the user id associated with the alias file the alias was found in?) Now throw into the mix per user procmail :-( It can be a mess and the source of lots of bug reports and list questions. I believe the best solution is to be absolutely explicit over the expected owner and group and thus will allevate 1 axis of confusion. As indicated above I will modify the spec file to make this absolutely explicit and locked down. With respect to requiring a C++ compiler, the code is C++ so that seemed necessary. It sounds like you may be more familar with this particular issue than I am so I will defer to you on this topic. You are correct, %{buildsubdir} is not guarenteed to exist, I'll fix that. I'm not sure I share your opinion that RPM_SOURCE_DIR should never be used. I appreciate that SOURCEn will always properly refer to it but I find use of SOURCEn to reduce readability and comprehenion of spec files. Whenever I encouter this use when reading a spec file I have to mentally expand the macro to understand what the spec file is doing, I find it much more comprensible if the operation is explicit. Hmm... I suppose the same argument could be used against %{httpdconffile} :-) At the time I packaged up lurker only the 1.3 version was available. Looks like the 2.0 release occurred shortly after I completed my work but before I got my review request in. I agree, 2.1 should be version in extras. I'll go back and do that, but it won't be immediate, I've got other tasks on my plate. I'm also going to have to figure out the differences between 1.3 and 2.x. Well, what is the current status of this review request? No response in ages; I'm closing this. |