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@xxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED CC| |orcanbahri@xxxxxxxxx AssignedTo|nobody@xxxxxxxxxxxxxxxxx |orcanbahri@xxxxxxxxx Flag| |fedora-review? --- Comment #1 from Orcan 'oget' Ogetbil <orcanbahri@xxxxxxxxx> 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. _______________________________________________ Fedora-package-review mailing list Fedora-package-review@xxxxxxxxxx http://www.redhat.com/mailman/listinfo/fedora-package-review