[Bug 803089] Review Request: whenjobs - Replacement for cron with dependencies

[bugzilla at redhat.com]

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=803089

Michael Scherer <misc at zarb.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |misc at zarb.org
               Flag|                            |fedora-review?

--- Comment #1 from Michael Scherer <misc at zarb.org> 2012-03-17 18:05:16 EDT ---
Let me start the review, but I am not a ocaml specialist.

1) %{_libdir}/whenjobs/ is unowned, you should add %dir %{_libdir}/whenjobs/ in
the %files section

2) given that this requires f17 to be built, I think you can remove %defattr (
unless the plan is to backport to EL 5, but I doubt ) 

3) same goes for %clean and the rm -Rf $RPMBUILDROOT at the start of %install,
if I am not wrong, so you can remove them ( no need to keep unused cruft )

4) per  https://fedoraproject.org/wiki/Packaging/Guidelines#File_Dependencies ,
could you change the requires on /usr/bin/ocamlc to the package name ?
same goes for the buildRequires on perl-doc. 

5) nitpicking, but 
BuildRequires:   pcre-devel, ocaml-pcre-devel

is better on 2 lines, as this ease review of a potential changes. But that's
not blocking for the review.

6) shouldn't whenjobsd be start at boot, with a systemd file ? ( given your
blog posts and the use case, this sound logical to me, but maybe I missed
something )

7) you detect if the bytecode is created or not at the beggining of the spec,
but do not act on it as explained on :

https://fedoraproject.org/wiki/Packaging:OCaml#Bytecode-only_architectures

So either there is something missing, or something not used, or something magic 
I would bet on the 1st, but maybe that's the 3rd

-- 
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.