Bug 310641

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.