https://bugzilla.redhat.com/show_bug.cgi?id=1055393
--- Comment #3 from Michel Alexandre Salim <michel+fdr(a)sylvestre.me> ---
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...
(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