[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

--- Comment #4 from Richard W.M. Jones <rjones at redhat.com> 2012-03-18 05:55:22 EDT ---
(In reply to comment #1)
> 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

Fixed.

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

Fixed.

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

Fixed.

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

My reading of that section is that dependencies on /usr/bin/* are
permitted.  I'd prefer to keep them because the program does in
fact run those binaries, like /usr/bin/ocamlc, so it seems more logical
to depend on the binaries, not the packages (which might change in future).

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

Fixed.

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

No, the daemon is started by each user that requires it.

> 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

This is actually an upstream problem.  I need to add all the
upstream Makefile rules so that whenjobs can be compiled as
native code.  It was just easier to use bytecode compilation,
and since it's not very performance-sensitive I didn't bother
much about it.  One benefit of fixing this upstream is that we
wouldn't need the prelink stuff (because prelink doesn't break
native-compiled programs).

(In reply to comment #2)
> and one last one, the release do not have %{?dist}.

Fixed!

I have just pushed the changes to the upstream git repo for now:
http://git.annexia.org/?p=whenjobs.git;a=commitdiff;h=7e01f4e655c5ceadbb571f4ded19427b41ae2933
since item #7 needs a big amount of upstream work to fix properly.
Hopefully I can get some time to do that this week.

Thanks for the initial review.

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