Bug 785727 - Review Request: ocaml-camlimages - OCaml image processing library
Review Request: ocaml-camlimages - OCaml image processing library
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
All Linux
medium Severity medium
: ---
: ---
Assigned To: Hans de Goede
Fedora Extras Quality Assurance
Depends On:
Blocks: 716146
  Show dependency treegraph
Reported: 2012-01-30 08:34 EST by Bruno Wolff III
Modified: 2012-03-26 14:01 EDT (History)
5 users (show)

See Also:
Fixed In Version: ocaml-camlimages-4.0.1-2.fc16
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Last Closed: 2012-03-21 15:04:21 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
hdegoede: fedora‑review+

Attachments (Terms of Use)

  None (edit)
Description Bruno Wolff III 2012-01-30 08:34:02 EST
Spec URL: http://people.fedoraproject.org/~bruno/ocaml-camlimages.spec
SRPM URL: http://people.fedoraproject.org/~bruno/ocaml-camlimages-4.0.1-1.fc17.src.rpm
This is an image processing library, which provides some basic
functions of image processing and loading/saving various image file
formats. In addition the library can handle huge images that cannot be
(or can hardly be) stored into the memory (the library automatically
creates swap files and escapes them to reduce the memory usage).
Comment 1 Bruno Wolff III 2012-01-30 08:37:53 EST
Note that ocaml-camlimages was previously packaged for Fedora. The last successful build was for F13. There have been significant changes in the build process between the current and previous versions of the package since then.

I have done a scratch build at: http://koji.fedoraproject.org/koji/taskinfo?taskID=3745888
Comment 2 Jerry James 2012-02-23 12:55:59 EST
I believe that these lines:

%define _use_internal_dependency_generator 0
%define __find_requires /usr/lib/rpm/ocaml-find-requires.sh
%define __find_provides /usr/lib/rpm/ocaml-find-provides.sh

should not be used anymore, even though they still appear on the wiki.  When I removed those lines from some ocaml packages, I got the same Provides/Requires as before, plus additional (correct) Requires on C libraries that doing the above apparently suppresses.  See:

Comment 3 Bruno Wolff III 2012-02-25 00:29:19 EST
Thanks. It doesn't look like I can expect much help from the ocaml-devel list based on the recent archives.

I am not going to do new builds just yet with those lines removed, but it is good to know that there may be issues with the wiki recommendations.
Comment 4 Hans de Goede 2012-03-08 13:51:42 EST
I'll be reviewing this tonight, assigning to me.
Comment 5 Hans de Goede 2012-03-09 03:02:01 EST
I ran out of steam last night, but today is another day. So about the /usr/lib/rpm/ocaml-find-requires.sh stuff, I just checked ocaml-lablgtk, which is one of the most active maintained ocaml packages, and it has this in
its changelog:

* Tue Jan  5 2010 Richard W.M. Jones <rjones@redhat.com> - 2.14.0-4
- Use upstream RPM 4.8 dependency generator.

So it seems that rpm itself now (well for quite a while actually) knows how to handle ocaml dependencies and those lines indeed can be dropped.

Looking at that same package it also seems that the opt part can now be build unconditionally.
Comment 6 Hans de Goede 2012-03-09 03:20:01 EST
- rpmlint checks return:
  ocaml-camlimages.src:89: W: macro-in-comment %exclude
  ocaml-camlimages.src:89: W: macro-in-comment %{_libdir}
  ocaml-camlimages.src:103: W: macro-in-comment %{_libdir}
  ocaml-camlimages.src: W: invalid-url Source1: camlimages-2.2.0-htmlref.tar.gz
  3 packages and 0 specfiles checked; 0 errors, 4 warnings.
 These can all be ignored.
- package meets naming guidelines
- package meets packaging guidelines
- license (GPLv2 with exceptions) OK, text in %doc, matches source
- spec file legible, in am. english
- source matches upstream
- package compiles on devel (x86)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file
- devel package ok
- no .la files
- post/postun ldconfig not needed since no libs directly under %{_libdir}
- devel requires base package n-v-r 

Should Fix
-Drop these 3 lines (use rpms internal dep generator instead):
 %define _use_internal_dependency_generator 0
 %define __find_requires /usr/lib/rpm/ocaml-find-requires.sh
 %define __find_provides /usr/lib/rpm/ocaml-find-provides.sh
-Make opt unconditional (not 100% sure about this, but if lablgtk does it can probably be dropped and
 it makes the spec easier to read)
-Using: https://bitbucket.org/camlspotter/camlimages/get/v4.0.1.tar.gz as Souce0 works fine for me,
 wget gets a v4.0.1.tar.gz, spectool -g  gets a v4.0.1.tar.gz and rpmlint likes it (iow it can download and
 verify the source)
-Doing rm -rf buildroot in %install and having a %clean is no longer needed now a days, and should not
 be done when not manually defining buildroot inside the spec
- -devel packages should require the main package like this:
 Requires:       %{name}%{?_isa} = %{version}-%{release}
 So please add %{?_isa} there.
-You could also drop the 2 %defattr lines, those are also not needed now a days

Since there are no blockers, this package is APPROVED! But please do consider fixing some of the should fix items.
Comment 7 Bruno Wolff III 2012-03-09 08:43:13 EST
Thanks for the review!

I'll fix the should fixes before I ask for unretirement / do the new initial import.
I might do a little tonight, but most likely I'll do it Saturday during the day.
Comment 8 Bruno Wolff III 2012-03-10 11:09:26 EST
I have an updated version that fixes the should fixes at:
Comment 9 Bruno Wolff III 2012-03-10 11:23:09 EST
Package Change Request
Package Name: ocaml-camlimages
Short Description: OCaml image processing library
New Branches: f15 f16 f17 devel
Owners: bruno

This technically an unretirement, though ocaml-calimages has been gone for multiple releases now and I don't think has a branch for any active releases.

There should have been a new line between the two URLs in the previous comment so I am going to copy them here again so they are usable.
Comment 10 Gwyn Ciesla 2012-03-12 08:08:42 EDT
Git done (by process-git-requests).

Created new branches, please take ownership of devel, and possibly EL-6.
Comment 11 Bruno Wolff III 2012-03-12 08:31:50 EDT
The package still shows as deprecated in el6 and devel, so I can't take ownership there.
Comment 12 Bruno Wolff III 2012-03-12 09:34:21 EDT
The package also seems to be blocked from building even on the instances that I am approved for. Builds are failing for being blocked on f15 through devel.
el6 does not have one of the build dependencies (ocaml-omake), so at least for the near term the package should stay blocked in el6.
Comment 13 Gwyn Ciesla 2012-03-12 09:55:11 EDT
I unretired EL-6 and devel.  You'll need to file a ticket with rel-eng to
unblock the Fedora branched.
Comment 14 Fedora Update System 2012-03-13 16:49:22 EDT
ocaml-camlimages-4.0.1-2.fc17 has been submitted as an update for Fedora 17.
Comment 15 Fedora Update System 2012-03-13 16:50:20 EDT
ocaml-camlimages-4.0.1-2.fc15 has been submitted as an update for Fedora 15.
Comment 16 Fedora Update System 2012-03-13 16:50:31 EDT
ocaml-camlimages-4.0.1-2.fc16 has been submitted as an update for Fedora 16.
Comment 17 Fedora Update System 2012-03-15 22:45:32 EDT
ocaml-camlimages-4.0.1-2.fc17 has been pushed to the Fedora 17 testing repository.
Comment 18 Fedora Update System 2012-03-21 15:04:21 EDT
ocaml-camlimages-4.0.1-2.fc17 has been pushed to the Fedora 17 stable repository.
Comment 19 Fedora Update System 2012-03-25 23:55:44 EDT
ocaml-camlimages-4.0.1-2.fc15 has been pushed to the Fedora 15 stable repository.
Comment 20 Fedora Update System 2012-03-25 23:59:17 EDT
ocaml-camlimages-4.0.1-2.fc16 has been pushed to the Fedora 16 stable repository.
Comment 21 Fedora Update System 2012-03-26 13:57:06 EDT
ocaml-camlimages-4.0.1-2.fc15 has been pushed to the Fedora 15 stable repository.
Comment 22 Fedora Update System 2012-03-26 14:01:06 EDT
ocaml-camlimages-4.0.1-2.fc16 has been pushed to the Fedora 16 stable repository.

Note You need to log in before you can comment on or make changes to this bug.