Bug 310641 - Review Request: GREYCstoration - An image denoising and interpolation tool
Review Request: GREYCstoration - An image denoising and interpolation tool
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Ian Chapman
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-09-28 07:41 EDT by Marc Bradshaw
Modified: 2007-11-30 17:12 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-10-10 21:47:17 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
packages: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Marc Bradshaw 2007-09-28 07:41:11 EDT
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.
Comment 1 Marc Bradshaw 2007-09-28 08:09:06 EDT
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
Comment 2 Jason Tibbitts 2007-09-29 12:30:09 EDT
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.
Comment 3 Marc Bradshaw 2007-09-30 01:48:25 EDT
Thanks for the comments, both issues fixed in this update.

SRPM URL: http://marcbradshaw.co.uk/packages/GREYCstoration-2.5.2-3.src.rpm
Comment 4 Ian Chapman 2007-10-01 15:17:48 EDT
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.
Comment 5 Jason Tibbitts 2007-10-01 15:35:58 EDT
There is no shortage of things to review, so please do go ahead.
Comment 6 Ian Chapman 2007-10-01 18:26:40 EDT
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.

Comment 7 Ian Chapman 2007-10-01 19:19:15 EDT
10. Oh and %{?dist} is not permitted in changelogs now AFAIK
Comment 8 Marc Bradshaw 2007-10-02 07:02:28 EDT
Ref point 7, /usr/bin/gimptool-2.0 is required during the build of the gimp plugin.
Comment 9 Ian Chapman 2007-10-02 08:44:55 EDT
(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 :-)
Comment 10 Marc Bradshaw 2007-10-04 08:10:54 EDT
Working through those now, will have an update shortly.
Comment 11 Ian Chapman 2007-10-04 10:35:57 EDT
No worries, if you need any help with the patches let me know. I can test it on
a 64bit system.
Comment 12 Marc Bradshaw 2007-10-04 20:02:42 EDT
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@marcbradshaw.co.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.
Comment 13 Ian Chapman 2007-10-05 14:21:32 EDT
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.
Comment 14 Ian Chapman 2007-10-07 15:38:16 EDT
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.

Comment 15 Marc Bradshaw 2007-10-08 21:40:11 EDT
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.
Comment 16 Ian Chapman 2007-10-09 13:46:36 EDT
(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.
Comment 17 Kevin Kofler 2007-10-09 19:33:48 EDT
> 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.
Comment 18 Marc Bradshaw 2007-10-09 23:02:58 EDT
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
Comment 19 Ian Chapman 2007-10-10 17:51:12 EDT
(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 :-)
Comment 20 Ian Chapman 2007-10-10 18:14:17 EDT
* 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.
Comment 21 Marc Bradshaw 2007-10-10 18:41:27 EDT
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
Comment 22 Ian Chapman 2007-10-10 19:30:17 EDT
Ok looks good!

Packages is APPROVED!
Comment 23 Marc Bradshaw 2007-10-10 19:43:32 EDT
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
Comment 24 Kevin Fenzi 2007-10-10 21:01:32 EDT
cvs done.
Comment 25 Marc Bradshaw 2007-10-10 21:47:17 EDT
Thanks Kevin,
Imported and Built.

Note You need to log in before you can comment on or make changes to this bug.