https://bugzilla.redhat.com/show_bug.cgi?id=1055393 --- Comment #3 from Michel Alexandre Salim <michel+fdr@xxxxxxxxxxxx> --- Incorporated review feedbacks in -2, links and comments below. Spec URL: http://salimma.fedorapeople.org/specs/ocaml/ocaml-biniou.spec SRPM URL: http://salimma.fedorapeople.org/specs/ocaml/ocaml-biniou-1.0.9-2.fc20.src.rpm (In reply to Jerry James from comment #2) > The first nine issues are repeats from the ocaml-easy-format review, because > they apply to this package as well: > [snip -- fixed as in ocaml-easy-format] > 7) Move %define libname down farther (between Summary and License?) and make > it %global instead. Oh, and here's a trick that lets you invoke 1 > subprocess instead of two: > > %global libname %(sed -e 's/^ocaml-//' <<< %{name}) > Oh, that is neat indeed! Already spun a new srpm so that will go in at the next revision > 8) If possible, avoid building the bytecode version on platforms that can > build the binary version; in this case, I see this in the build log after > building the binary version: > > + make opt > make: Nothing to be done for `opt'. > > because the default target builds both the bytecode and binary versions. > I've decomposed the 'default' build target to compensate, see revised spec > 9) Consider adding a %check script; "make test" appears to work for me. > the default target originally ran the test target as well, it's now moved to %check > 10) The description mentions a binary "atdgen", but there is no such binary > in > any of the binary RPMs. > Yanked > 11) I'm concerned about the binary named "bdump". First, that's a pretty > generic name. It doesn't collide with anything currently in Fedora, but > I'm concerned that it may in the future. Also, I'm not certain that > bdump > and the libraries should both go into the main package. Programs that > link against the library probably won't need bdump, which seems to be for > human use. Consider putting them in separate packages. > Good point. I've moved it to the -devel subpackage (and prefixed it with ocaml-), and as it turns out it's only generated when ocamlopt is available so now it's within the %if guard > 12) There is no man page for bdump. > Yes, upstream doesn't seem to provide one > 13) There is a '%' sign in %description; doubling it ("%%") produces the > correct output and avoids confusing rpm. > Fixed > 14) There is no stated justification for the patch. Anyway, it isn't needed. > Drop the patch and do this in %install instead: > Aha, good point -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review