Package Name: GREYCstoration Spec URL: http://marcbradshaw.co.uk/packages/GREYCstoration.spec SRPM URL: http://marcbradshaw.co.uk/packages/GREYCstoration-2.5.2-1.src.rpm Description: GREYCstoration is an image regularization algorithm which is able to process a color image by locally removing small variations of pixel intensities while preserving significant global image features, such as edges and corners. The most direct application of image regularization is image denoising. By extension, it can also be used to inpaint or resize images.
Whoops, missed a build requires there, let's try that again. SRPM URL: http://marcbradshaw.co.uk/packages/GREYCstoration-2.5.2-2.src.rpm
A few comments: License: should should be "CeCILL", not "CeCiLL", although I can't imagine this makes any significant difference. The debuginfo package is completely empty. This is bad. It looks like the makefile is stripping the binaries by calling strip directly, so you will need to do some patching. rpmlint complains about both of these.
Thanks for the comments, both issues fixed in this update. SRPM URL: http://marcbradshaw.co.uk/packages/GREYCstoration-2.5.2-3.src.rpm
Jason, I intend to review this in the next couple of days in return for Marc performing a review for me. I've assigned it to myself but as you've already made some suggestions, if you wish to perform a full review, of course feel free to assign it to yourself.
There is no shortage of things to review, so please do go ahead.
1. It's usual to begin Patches at 0 and not 1. Not a problem but for the sake of commonality you might want to change it. 2. Patches should ideally include a brief descriptor in the filename, for example changing GREYCstoration-2.5.2.1.patch to GREYCstoration-2.5.2.1-dontstrip.patch 3. "non standard" buildroot. You should probably change this to %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n) 4. Extraneous options in the %build section. Is there a reason why you're defining -DSUPPORT_LH7 -DMKSTEMP? 5. Your package will almost certainly fail to build on ppc64 and x86_64 because the makefile hard codes lib when they use lib64. One solution might be to use %ifarch statements and sed to substitute lib for lib64 where appropriate on 64bit archs. 6. Unnecessary options for %setup, just use: %setup -q 7. Unncessary buildrequire: gimp 8. Consider also installing GREYCstoration_gui.tcl with appropriate icon and in a sub package (eg GREYCstoration-gui) so it's not forced on anyone who just wants the command line or gimp version. 9. The makefile is broken with regards to parallel building. It doesn't cause the buildjob to fail but reverts to -j1 anyway. Probably because make doesn't realise it's calling itself. Ideally this should be fixed, but if not then drop %{?_smp_mflags} and add a comment in the SPEC about it.
10. Oh and %{?dist} is not permitted in changelogs now AFAIK
Ref point 7, /usr/bin/gimptool-2.0 is required during the build of the gimp plugin.
(In reply to comment #8) > Ref point 7, /usr/bin/gimptool-2.0 is required during the build of the gimp plugin. Interesting, I tried building it without gimp in mock and it built successfully. I'll double check that though, it was getting late :-)
Working through those now, will have an update shortly.
No worries, if you need any help with the patches let me know. I can test it on a 64bit system.
A test on 64bit would be good thanks. I have patched the makefile to stop it calling itself, hopefully this should address point 9. http://marcbradshaw.co.uk/packages/GREYCstoration-2.5.2-4.src.rpm * Thu Oct 04 2007 Marc Bradshaw <packages.uk> 2.5.2-4 - Fixed Buildroot, Patch, Build and Setup - Added GUI - Added support for more image formats in Makefile - Fixed build on systems with lib64 GREYCstoration-gimp.i386: W: no-documentation GREYCstoration-gui.i386: W: no-documentation Expected as there is no documentation for those components.
Ok just to let you know that I'm still looking at it but there's a few more issues. It does compile on 64bit archs now but not devel so there's some bug fixing to do, I'll look into that and figure out why. Secondly you're missing at least 1 requires but I'll post a more detailed finding shortly. Just keeping you posted.
1. You're missing a requires hicolor-icon-theme-0.10-2 for the gui package 2. You're missing a requires tcl for the gui package I'd normally expect a desktop-file-utils buildrequires but it appears OK as it must be being dragged in by another dependency. You're correct about gimp, it is needed. Apart from the issues above I have q query about the gui package, what's the rationale for installing it in the %{_datadir} rather than %{_bindir}? Apart from the above everything seems OK now. For some reason it does not build on x86_64-devel, see http://dribble.org.uk/buildlogs/GREYCstoration-2.5.2-4/8/x86_64/ It appears to be dragging in C code without an extern "C" macro but I have no idea where or why, it compiles fine on F7 and FC-6 for x86_64 and compiles fine in devel for i386. So we have two choices, you can either ask on one of the mailing lists for help from someone who knows a lot more about C++ than me, or (annoyingly!) we might have to excludearch: x86_64. Whatever option you'd rather go with.
1,2 Will fix. 3 I don't think we can rely on pulling it from another package randomly so I will add it to the buildrequires list. 4 Probably a senior moment, it should be bindir and I will change that. Strange that it should only be x86_64 devel. I say we post to the mailing list for help and excludearch if we draw a blank there. I will get a new release with the other fixes up and post to the ML.
(In reply to comment #15) > 1,2 Will fix. Note, I wrote hicolor-icon-theme-0.10-2 which should of course just be hicolor-icon-theme (a cut n paste error) > 3 I don't think we can rely on pulling it from another package randomly so I > will add it to the buildrequires list. No problem. > 4 Probably a senior moment, it should be bindir and I will change that. Great! > Strange that it should only be x86_64 devel. I say we post to the mailing list > for help and excludearch if we draw a blank there. Sounds reasonble, I'll hang fire until you've made the post then. It might even be a "system" bug, but I'm reluctant to ever claim that as it's rarely ever the case. :-) > I will get a new release with the other fixes up and post to the ML. No probs.
> It appears to be dragging in C code without an extern "C" macro It's actually doing the opposite, it's wrapping C++ stuff (templates and so) in extern "C". It might be a bug in one of the library headers you're using. Maybe a missing '}' somewhere? That would also explain the missing '}' reported at the end of the file.
Reply from Mamoru Tasaka on the fedora-devel-list This srpm can be rebuilt on all arch, dist-f8. http://koji.fedoraproject.org/koji/taskinfo?taskID=189122 I guess the build failure you saw is because your build used gimp 2.4.0 rc1. Update gimp to 2.4.0 rc3. (current rawhide Fedora's gimp is gimp-2.4.0-0.rc3.1.fc8). Similar discussion is found on https://bugzilla.redhat.com/show_bug.cgi?id=250210#c9 Regards, Mamoru --- Ian, can you make the gimp update and confirm this fixes the issue. A new SRPM will be up shortly at http://marcbradshaw.co.uk/packages/GREYCstoration-2.5.2-5.src.rpm
(In reply to comment #18) > Ian, can you make the gimp update and confirm this fixes the issue. Success, the new version of Gimp did the trick. The local repo I use was still set to pull updates from the old devel path which change a few weeks ago so was consequently out of date. Thats been fixed :-)
* rpmlint: W: no-documentation No problem. * Package named correctly: YES * Patches named correctly: YES * Spec file named correctly: YES * Licence(s) acceptable: YES * Licence field matches: YES * Licence file installed: YES (and included upstream) * Spec file in American English: YES * Source matches upstream: YES (!! but see below) * Locales use %find_lang: N/A * Contains %clean: YES * %install contain rm -rf %{buildroot} or similar: YES * Specfile legible: YES * Compiles and builds ok: YES (mock/i386/x86_64 all releases) * Calls ldconfig in %post/%postun for shlibs: N/A * Owns directories it creates: YES * Duplicate files: NO * Permissions set correctly: YES * Consistent macro use: YES * Separate -doc needed (for large docs): N/A * %doc affects runtime: N/A * Headers and static libs in -devel: N/A * .pc files in -devel: N/A * .so in -devel: N/A * -devel requires base: N/A * Contains .la files: NO * Owns files it didn't create: NO * .desktop files included and installed correctly: YES * Filenames valid UTF8: YES Ok there's only one thing I think you should fix. Please use the source zip file labelled as 2.5.2, it is identical to 2.5.2.1 which I believe is a phantom version accidentally uploaded by upstream. It will also alleviate the ugliness in the SPEC file in relation to the .1 suffix stuck on everything. Remember to strip it from your patch filename too.
The archives seem identical and it will clean the spec file a little. A new SRPM will be up shortly at http://marcbradshaw.co.uk/packages/GREYCstoration-2.5.2-6.src.rpm
Ok looks good! Packages is APPROVED!
Thanks for the review. New Package CVS Request ======================= Package Name: GREYCstoration Short Description: An image denoising and interpolation tool Owners: deebs Branches: F-7 InitialCC: Cvsextras Commits: yes
cvs done.
Thanks Kevin, Imported and Built.