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@xxxxxxxxxx> 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. _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review