Fedora Merge Review: ImageMagick http://cvs.fedora.redhat.com/viewcvs/devel/ImageMagick/ Initial Owner: nmurray
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
Added Hans' email to CC since he did the last known maintenance. Sorry if this was not desired.
Additionally, * There is a script in the source that builds the docs for the C API. It would be nice to build it and include in the -devel or -doc package.
(In reply to comment #1) > * 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. It IMHO is still needed, because a "-0" is a real exception by that upstream.
(In reply to comment #2) > Added Hans' email to CC since he did the last known maintenance. Sorry if this > was not desired. Yes, Norm Murray seems to not have too much time to spend on ImageMagick, so I've been co-maintaining and really doing most of the work lately. I'll take a look at fixing the issues you've mentioned, but I want to do that in a new branch for F-11, so first: Package Change Request ====================== Package Name: ImageMagick New Branches: F-10 <early branch request>
(In reply to comment #1) > * 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 For reference, see 185237
(In reply to comment #6) > (In reply to comment #1) > > * 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 > > For reference, see 185237 This is bug 185237 ...
Please do not kill the *.la files except there's a real way to solve the issue which otherwise would show up as in bug #185237; I'm the reporter of that bug.
I didn't know about bug #185237 and the related bugs until now. I remember the time ImageMagick broke 2 years ago but it was fixed so quick that I didn't even had to check bugzilla. Well, if it would break things it is better to keep the .la files. Does upstream know about this issue? Or is this a "feature" ?
cvs done.
I successfully built ImageMagick (6.4.0.10) with --enable-static=no \ --with-modules=no \ and tried "convert", "display", "identify". They worked flawlessly for all the cases I tried (The package I built was free of .la files.). Please confirm this.
(In reply to comment #1) > I reviewed ImageMagick. It needs some work to yield the guidelines: > Thanks, Below is my response to items which I did not address the rest is fixed in the latest release which you can find here: http://koji.fedoraproject.org/koji/taskinfo?taskID=933562 > * 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/ > The .la files are still needed, see bug 185237, also as these are not in the standard library path, so they can do no harm. > Afaik, this one is not supported anymore, can be removed: > BuildRequires: libungif-devel > That is not true, libungif was a patent free *drop in* replacement for giflib, now that the patent is no longer valid we use giflib again, and giflib-devel provides libungif-devel, so this BR drags in giflib-devel, I've changed it to directly BR giflib-devel > * Is there a particular reason why you don't Require: > libpng-devel,libtiff-devel,libwmf-devel,libxml2-devel in the -devel package? > Because non of the ImageMagick headers use headers from these packages, it would be nice if you would check such things yourself before making comments like this in a review. Usually the reviewer does this the other way around he checks which headers from other packages the headers need, and if they are all Required by 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. I assume you mean --disable-static, done. > * Adding djvu and jbig supports will be nice (although not necessary). > jbig is patented and thus not included in Fedora
Thanks for the update! > > * 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/ > > > > > The .la files are still needed, see bug 185237, also as these are not in the > standard library path, so they can do no harm. > I realized that. I have got confused because when I built and installed ImageMagick without .la files, it worked. But I was sloppy enough to forget to remove the old .la files from a previous installation. So keeping them is *required*. > > * Is there a particular reason why you don't Require: > > libpng-devel,libtiff-devel,libwmf-devel,libxml2-devel in the -devel package? > > > > Because non of the ImageMagick headers use headers from these packages, it > would be nice if you would check such things yourself before making comments > like this in a review. Usually the reviewer does this the other way around he > checks which headers from other packages the headers need, and if they are all > Required by the -devel package. > I apologize. It was towards the end of a long review... I had to be careful and shouldn't make assumptions. > > * 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. > > I assume you mean --disable-static, done. > ./configure --help doesn't tell a --disable-static flag. If it works one way or the other, it's fine. > > * Adding djvu and jbig supports will be nice (although not necessary). > > > > jbig is patented and thus not included in Fedora Ok. What about djvu? All you need to do is add a BR: djvulibre-devel . It is enabled by the default ./configure Two more things: * There are two items (a directory and a file) that are in the source but do not end up in any of the packages: *** The files inside www/source/ are referenced in the docs. See, for example: file:///usr/share/doc/ImageMagick-6.4.5/www/magick-wand.html file:///usr/share/doc/ImageMagick-6.4.5/www/magick-core.html But the directory www/source isn't included in the -doc subpackage. I think it is a flaw of the Makefile script. Could you manually insert that directory into the -doc subpackage? *** Similarly, the file www/Magick++/COPYING doesn't end up in any subpackage. I think this file should go to the %doc of ImageMagick-c++ . * rpmlint says now: ImageMagick.x86_64: W: shared-lib-calls-exit /usr/lib64/libMagickWand.so.1.0.0 exit.5 This seems rather annoying. It is hard (or more appropriately, time-consuming) to check the entire code to see when these calls are made. Shall we ask the upstream if these calls would break anything or have you investigated the situation already?
(In reply to comment #13) > Ok. What about djvu? All you need to do is add a BR: djvulibre-devel . It is > enabled by the default ./configure > I've done a new version: http://koji.fedoraproject.org/koji/taskinfo?taskID=934612 with djvulibre support enabled, I've put the djvu libre plugin in a separate plugin package so as to not drag in djvulibre for everyone who wants ImageMagick, as it ways in at 2 MB's and we are already getting complaints about all the stuff ImageMagick drags in. > Two more things: > * There are two items (a directory and a file) that are in the source but do > not end up in any of the packages: > > *** The files inside www/source/ are referenced in the docs. See, for > example: > file:///usr/share/doc/ImageMagick-6.4.5/www/magick-wand.html > file:///usr/share/doc/ImageMagick-6.4.5/www/magick-core.html > But the directory www/source isn't included in the -doc subpackage. I > think it is a flaw of the Makefile script. Could you manually > insert that directory into the -doc subpackage? > Added. > *** Similarly, the file www/Magick++/COPYING doesn't end up in any > subpackage. I think this file should go to the %doc of > ImageMagick-c++ . > Added. > * rpmlint says now: > ImageMagick.x86_64: W: shared-lib-calls-exit > /usr/lib64/libMagickWand.so.1.0.0 exit.5 > This seems rather annoying. It is hard (or more appropriately, time-consuming) > to check the entire code to see when these calls are made. Shall we ask the > upstream if these calls would break anything or have you investigated the > situation already? I cannot find the source for this I'll mail upstream about it.
Thanks. There is one minor thing that I keep forgetting to say. There are two doc directories created, which is a consequence of the unusual version numbering of upstream: /usr/share/doc/ImageMagick-%{version} /usr/share/doc/ImageMagick-%{VER} I understand that the Makefile installs the document files in the latter one., but the %doc of the main package installs the files in the former one. It can be argued but I don't think we actually need two doc directories. This can be resolved by using the --docdir flag of configure or directly patching/sed'ding the configure file. I am leaving this up to you. You can fix this at a later release since it is a purely cosmetics issue. ---------------------------------------------- This package (ImageMagick) is APPROVED by oget ----------------------------------------------
I guess (since you recently built another release in koji without addressing this issue) that you decided to leave the document directories the way they are. A reasoning would have been nice. Anyways, I'm closing the bug.
*** Bug 476453 has been marked as a duplicate of this bug. ***