Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=592579 --- Comment #13 from David A. Wheeler <dwheeler@xxxxxxxxxxxx> 2010-05-24 12:08:08 EDT --- I'm looking over the .spec file for overall comments, before I delve into a point-by-point comparison with the guidelines. * Every time you make a change you should bump the "Revision" number, and post the new URL. That'll be *critical* when it's submitted to the repository, but it'd reduce confusion now too. * Patch0 comment says: "The configure file uses graph.cmo in compiling a test program to check for ocamlgraph, but Fedora doesn't include this file in its distributions." I'm confused, there *is* an ocaml-ocamlgraph package. Is the Fedora respository's ocamlgraph the wrong (too old) version? Or, is this just an old comment based on an older version of this spec, now OBE? * Patch1 isn't prefixed with "frama-c-", and that can cause trouble. In general, if someone looks in their rpmbuild/SOURCES, they can get confused (and namespace issues could result) if they aren't prefixed. You should prefix the patch filenames with NAME- (e.g., "frama-c-"). * Rejected Debian Patch2 (0002-Make-Jessie-plugin-use-Jc-from-Why-2.19.patch) comment says, "I don't know why we would want to use that Jessie -> not including this patch". You're not including that patch because we want to use a later version of Why, correct? If so, just say: "We use a later version of Why, so this patch is irrelevant". Although I applaud "I don't know..." comments for their honesty, I think we should track them down so you can simply say why something was done. In this case, there's a good reason: The patch is for an obsolete version of "why". * Rejected Debian patch "0003-Add-dGraphView.cmo-when-linking.patch" says "I'm not sure quite what this patch does, but I'm going to leave it out for now". Need to track this down a little of what this is. My *guess* from the text is that it merges graphview into another another library, and we *don't* want to do that (because then upgrading graphview won't updated the merged file). If that's the situation, please say so directly. * Rejected Debian patch 0004-Use-GSourceView2.patch comment says "I didn't see any Fedora package providing the files that this patch references -> dropping for now". Did you check out "gtksourceview2-devel" and "gtksourceview2"? It's possible that the patch includes the *OCaml* bindings for these and that Fedora doesn't include those bindings. If so, just say that. * In Patch "frama-c-1.4-ptests-fix-for-ocaml-3.11.2.patch" there needs to be a comment explaining (1) what it's for, and (2) when you submitted it upstream (and if not, why not). Shouldn't be *long*, but it should say *something*. * Nit: Your "Requires:" statements have inconsistent indenting. Please fix while you're at it. * Nit: "plug in's" is not a word. An "'s" is either a possessive or an appreviation of "is", and this is neither (it's just a plural). I'd suggest "plug-ins" instead. If the spellchecker whines, too bad. * I notice that the entire "%check" section is commented out. A %check section is NOT required, and I'd prefer the comments to nothing, but it'd be nice if it worked :-). * As noted earlier, the .byte files don't work, and probably shouldn't be packaged at all. * I'll check the %files list separately. * In %post, just remove the comment: #chcon -t textrel_shlib_t '%{_libdir}/frama-c/plugins/Ltl_to_acsl.cmxs' I don't see that the commend adds anything. * The %changelog doesn't list "Mark Rader"; it should! -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are on the CC list for the bug. _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review