[Bug 225897] Merge Review: ImageMagick

[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=225897


Orcan 'oget' Ogetbil <orcanbahri at yahoo.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
                 CC|                            |orcanbahri at yahoo.com
         AssignedTo|nobody at fedoraproject.org    |orcanbahri at yahoo.com
               Flag|                            |fedora-review?




--- Comment #1 from Orcan 'oget' Ogetbil <orcanbahri at yahoo.com>  2008-10-30 02:40:24 EDT ---
I reviewed ImageMagick. It needs some work to yield the guidelines:

* Latest version is not packaged. At the time of this review, version 6.4.5-0
was out.

* The %{VER} & %{Patchlevel} trick in the beginning of the spec file may not be
needed now because the source URL of the new version contains "-0" already.

* The files Magick++/{AUTHORS,ChangeLog,LICENSE,NEWS,README} must be included
in the %doc of ImageMagick-c++ subpackage.

* The directories Magick++/demo (and maybe Magick++/tests} and its contents
should be included in the %doc of ImageMagick-c++-devel subpackage.

* %{name} macro is used inconsistently. In some places ImageMagick is used,
whereas in some other places %{name} is used. The latter is preferred in all
occasions. Another inconsistency is: you use regular notation for all the
commands (mv,rm,sed etc.) but for perl you use %{__perl}.

* We recommend %defattr(-root,root,-)

* The extra specification for two of the files
  %attr(755,root,root)
is redundant because these files already have the right permissions.

* Parallel make should be enabled. If the package doesn't compile with parallel
make you should note this in the SPEC file.

* Packages must NOT contain any .la libtool archives, these should be removed
in the spec. In my mock build the current ImageMagick rpm contains .la files in
   /usr/lib64/ImageMagick-6.4.0/modules-Q16/coders/

* If you don't want to build the modules you can use configure's 
   --with-modules=no 
option


* The documentation is very large. It should go to a subpackage. Btw, why is
%{_datadir}/doc/ImageMagick* not in the %doc ? Maybe this should go to the the
-doc subpackage.

* I don't think these are needed for building:
   BuildRequires: automake >= 1.7 autoconf >= 2.58, libtool >= 1.5

Afaik, this one is not supported anymore, can be removed:
   BuildRequires: libungif-devel

Since you BR: perl-devel already, you won't need
   BuildRequires: perl(ExtUtils::MakeMaker), perl

* Double BR: freetype-devel

* Is there a particular reason why you don't Require:
libpng-devel,libtiff-devel,libwmf-devel,libxml2-devel in the -devel package?

* You might want to use the "--enable_static=no" flag for faster compilation.
This might also save some lines (from the SPEC) file dedicated to removing the
static libraries.

* It would be nice if you explain in the SPEC file what the patches and sed's
do.

* Source1 is not used.

* Why are these files removed?
   find $RPM_BUILD_ROOT -name "*.bs" |xargs rm -f
   find $RPM_BUILD_ROOT -name ".packlist" |xargs rm -f
   find $RPM_BUILD_ROOT -name "perllocal.pod" |xargs rm -f

* Adding djvu and jbig supports will be nice (although not necessary).

* DIE_RPATH_DIE is cool

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.