Bug 951496 (gimp-lensfun)

Summary: Review Request: gimp-lensfun - gimp plugin to correct lens distortion
Product: [Fedora] Fedora Reporter: Graham White <graham_alton>
Component: Package ReviewAssignee: Rex Dieter <rdieter>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: anto.trande, graham_alton, gwhite, i, luya, nphilipp, package-review, rdieter
Target Milestone: ---Flags: rdieter: fedora-review+
gwync: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
URL: http://grahamwhiteuk.fedorapeople.org/pkgreviews/gimp-lensfun/
Whiteboard:
Fixed In Version: gimp-lensfun-0.2.3-1.fc21 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2015-07-13 19:10:57 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On: 1048570    
Bug Blocks: 1000885    

Description Graham White 2013-04-12 10:32:23 UTC
Spec URL: http://grahamwhiteuk.fedorapeople.org/gimplensfun/gimplensfun.spec
SRPM URL: http://grahamwhiteuk.fedorapeople.org/gimplensfun/gimplensfun-0.2.2-1.fc18.src.rpm

Description: GimpLensfun is a Gimp plugin to correct lens distortion using the lensfun library and database.

Fedora Account System Username: grahamwhiteuk

Other Notes: my first review request ticket so sponsorship required (no sponsor yet identified)

Introduction:

I am new to the Fedora project outside of my company (IBM) but have been active with Red Hat based distributions since the year 2000.  I was certified RHCE on Red Hat 7 in 2001.  I think it's about time I gave something back so I'm starting with this simple package that I find extremely useful and build for myself at home.  I currently lead our internal project to deliver Fedora to desktop machines inside the company so very familiar with building RPMS, mock and koji already.  Professionally, I have worked in internal Linux support, High Performance Clustering, our Linux Integration Centre and Emerging Technologies (http://www.linkedin.com/in/grahamwhiteuk).  I have occasionally blogged about Fedora on the Internet (http://gibbalog.blogspot.co.uk/search/label/fedora).  You can also find me on Twitter (https://twitter.com/graham_alton) or chat to me on Freenode (Gibba).

Comment 1 Graham White 2013-04-15 13:36:38 UTC
Successful Koji Build: http://koji.fedoraproject.org/koji/taskinfo?taskID=5253393

Comment 2 Antonio T. (sagitter) 2013-04-16 17:12:58 UTC
Hi Graham.
Just some comments.

- "Compilers used to build packages must honor the applicable compiler flags set in the system rpm configuration. For C, C++, and Fortran code, the %{optflags} macro contains these flags."
http://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags

See that Makefile uses right flags.

- Source archive contains files released with GPLv3+ (see src/gimplensfun.c, src/LUT.h files). I think License tag must be GPLv3+.
http://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Mixed_Source_Licensing_Scenario

Comment 3 Graham White 2013-04-16 21:21:16 UTC
Thanks Antonio for the feedback.

I have updated the spec and rebuilt the SRPM for the GPLv3+ license.

The use of %{optflags} is problematic with this package.

The makefile sets:
CFLAGS = -O3 -Wall $(shell gimptool-2.0 --cflags && pkg-config --cflags lensfun exiv2) -fopenmp

It's importing CFLAGS from gimptool and pkg-config that include links to libraries via the -I flag.  Therefore, a straight use of CFLAGS="%{optflags}" breaks the build as the libraries cannot be found.

I presume the gimp-lqr-plugin RPM had a similar issue as it also does not use %{optflags}.

I could still include %{optflags} if I run gimptool and pkg-config in the spec file and pass the result into the call to make - would this be preferred to omitting %{optflags} altogether?

I guess another approach would be for me to submit something upstream to alter their makefile so we can pass CFLAGS in more easily in the spec.  The current git version of the makefile has changed from the release version I'm building here anyway but is still using gimptool and pkg-config in CFLAGS.

If it's deemed best to leave %{optflags} out then I will add a comment to the spec to document why it's not there.

Comment 4 Antonio T. (sagitter) 2013-04-17 16:33:38 UTC
(In reply to comment #3)
> Thanks Antonio for the feedback.
> 
> I have updated the spec and rebuilt the SRPM for the GPLv3+ license.
> 
> The use of %{optflags} is problematic with this package.
> 
> The makefile sets:
> CFLAGS = -O3 -Wall $(shell gimptool-2.0 --cflags && pkg-config --cflags
> lensfun exiv2) -fopenmp
> 
> It's importing CFLAGS from gimptool and pkg-config that include links to
> libraries via the -I flag.  Therefore, a straight use of
> CFLAGS="%{optflags}" breaks the build as the libraries cannot be found.
> 
> I presume the gimp-lqr-plugin RPM had a similar issue as it also does not
> use %{optflags}.
> 
> I could still include %{optflags} if I run gimptool and pkg-config in the
> spec file and pass the result into the call to make - would this be
> preferred to omitting %{optflags} altogether?

Try to:

- Patch Makefile into %prep section with a simple replacement of "-O3 -Wall" with "@@OPTFLAGS@@"

- In %build, add 

sed -e 's|@@OPTFLAGS@@|%{optflags}|g' -i Makefile'

Comment 5 Michael Schwendt 2013-04-17 18:08:18 UTC
Correct. Note that

  http://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags

is a MUST item, also with regard to what must be done when filtering/adding to the set of flags. That's because some of the flags are security related, and e.g. -g is required for automatic generation of -debuginfo packages:
https://fedoraproject.org/wiki/Packaging:Guidelines#Debuginfo_packages

$ rpm --eval %optflags
-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4  -m64 -mtune=generic

Comment 6 Graham White 2013-04-25 14:11:50 UTC
Spec URL: http://grahamwhiteuk.fedorapeople.org/pkgreviews/gimp-lensfun/gimp-lensfun.spec
SRPM URL: http://grahamwhiteuk.fedorapeople.org/pkgreviews/gimp-lensfun/gimp-lensfun-0.2.2-2.20130423git6ab226c12a.fc18.src.rpm

Successful Koji Build: http://koji.fedoraproject.org/koji/taskinfo?taskID=5301586

Change Summary:
* %optflags added to the build
* updated to postrelease code (changed package version)
* changed package name

The addition of %optflags has been done by a merged upstream pull request to fix the makefile.  Hence, the package has been moved to a postrelease version from git and follows the Fedora Packaging Naming Guidelines for the updated version string.  It would be nice to get a 0.2.3 release but upstream have not given me an outlook on when this might be available.

I have also changed the package name from gimplensfun to gimp-lensfun.  This has been done to fit in with the upstream name change following the same convention (i.e. they have introduced a hyphen into the name).  Conveniently, this seems to fit better with Fedora's package naming and other gimp plugin names.  The package name change has not been added to the changelog in the spec file since this change has not been seen by clients given this package isn't included in Fedora yet.

Comment 7 Graham White 2013-05-23 07:43:31 UTC
Spec URL: http://grahamwhiteuk.fedorapeople.org/pkgreviews/gimp-lensfun/gimp-lensfun.spec
SRPM URL: http://grahamwhiteuk.fedorapeople.org/pkgreviews/gimp-lensfun/gimp-lensfun-0.2.3-1.fc18.src.rpm

Successful Koji Build: http://koji.fedoraproject.org/koji/taskinfo?taskID=5413262

Change Summary:
* Bumped to official 0.2.3 build
* Removed git build stuff

Since building the git version (above) the 0.2.3 official release of gimp-lensfun has happened.  This is an updated spec file and rebuilt srpm.  Successful Koji build against rawhide instead of F18 this time.

Comment 8 Graham White 2013-12-17 09:54:23 UTC
Successful F18 Koji Build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=6304317

Successful F19 Koji Build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=6304311

Successful F20 Koji Build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=6304277

Comment 9 Graham White 2013-12-17 09:58:39 UTC
Packages for F18, F19 and F20 updated and uploaded at http://grahamwhiteuk.fedorapeople.org/pkgreviews/gimp-lensfun/ and made available as a yum repository http://grahamwhiteuk.fedorapeople.org/pkgreviews/gimp-lensfun/gimp-lensfun.repo

Comment 10 Rex Dieter 2014-09-08 14:45:06 UTC
Christopher added a blocker to bug #1048570 , but the .spec here doesn't specifically require lensfun-0.2.8 (yet).  So, does it really need the newer version (or not)?

Comment 11 Rex Dieter 2014-09-08 14:47:47 UTC
Oh, and if my question (comment #10) gets clarified, I'd be happy to help with the review/sponsorship process asap :)

Comment 12 Graham White 2014-09-09 10:28:37 UTC
gimplensfun will build against lensfun 0.2.7 as well as 0.2.8 so 0.2.8 isn't a hard requirement, it's more of a nice-to-have since the later version includes updates that gimplensfun can take advantage of.  There are no docs in gimplensfun to indicate which is the minimum version but I can probably find out what the minimum is from upstream if required.

Comment 13 Nils Philippsen 2014-09-09 14:45:47 UTC
I've just submitted F-19, F-20 updates for lensfun-0.2.8:

https://admin.fedoraproject.org/updates/lensfun-0.2.8-3.fc20
https://admin.fedoraproject.org/updates/lensfun-0.2.8-3.fc19

The sooner it gets karma, the sooner you can depend on it :o).

Comment 14 Graham White 2014-09-10 19:37:53 UTC
I've tested lensfun-0.2.8-3 via gimplensfun version 0.2.3 and gimp 2.8.14 and all working OK for me on F20 - karma added.  However, gimplensfun still doesn't *need* to depend on this particular level of lensfun or above, it works quite happily with 0.2.7 as well without any rebuilding required.

Comment 15 Graham White 2015-04-13 20:29:05 UTC
Successful F21 Koji Build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=9470843

Comment 16 Graham White 2015-06-16 14:15:41 UTC
Successful F22 Koji Build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=10070503

Repo updated at https://grahamwhiteuk.fedorapeople.org/pkgreviews/gimp-lensfun/

Is someone ever going to sponsor this? Started over 2 years ago now!

Comment 17 Rex Dieter 2015-06-16 14:21:18 UTC
Fair enough, I threaten to help back in comment #11, so here we go. :)

Comment 18 Rex Dieter 2015-06-16 14:41:13 UTC
Naming: ok

Sources: NOT ok

1.  MUST: Source0 URL doesn't work,

spectool -g *.spec
Getting http://dl.bintray.com/content/seebk/GIMP-Lensfun/gimplensfun-0.2.3.tar.gz to ./gimplensfun-0.2.3.tar.gz
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0curl: (22) The requested URL returned error: 404 Not Found

while we're at it, I'd encourage you to use the %{version} macro in the URL, instead of hard-coding 0.2.3 (to make future updates easier), so:

Source0: https://github.com/seebk/GIMP-Lensfun/releases/download/%{version}/gimplensfun-%{version}.tar.gz

confirmed that source has the same checksum,
$ md5sum *.gz
2ac8cc61ab3672aa01311e47dfd911b0  gimplensfun-0.2.3.tar.gz


2.  SHOULD document patches.  
Patch0: %{name}-remove-cci.patch
What's the purpose of this patch? why is it needed? any bug references?

3.  SHOULD omit deprecated rpm bits: Group:, BuildRoot:, %clean section, from %install: rm -rf $RPM_BUILD_ROOT

4.  License: NOT ok.

$ licensecheck  *
gimplensfun.cpp: GPL (v3 or later)
LUT.hpp: GPL (v3 or later)

MUST use
License: GPLv3+
(unless you can clarify upstream intent to want only GPLv3, and and a .spec comment saying so)

scriptlets: ok

5. dependencies: NOT ok

These MUST be removed:
Requires:       lensfun
Requires:       exiv2
rpm should pull both of these library dependencies in via autorequires... unless does gimp-lensfun really need exiv2 (and not just the exiv2 library from exiv2-libs)?


Fix these, and we should be close to approval.

Comment 19 Graham White 2015-06-16 15:36:06 UTC
Thanks for the feedback.  It's been a while since I've run the review tool on the spec file.

I've taken the following actions:
  1) fixed the issues identified above in #18
  2) Run a couple of scratch builds against F21 and F22
       http://koji.fedoraproject.org/koji/taskinfo?taskID=10071542
       http://koji.fedoraproject.org/koji/taskinfo?taskID=10071546
  3) Tested installation and functionality on F22
  4) Run through fedora-review

I'd welcome any further feedback if there are still issues.

Comment 20 Rex Dieter 2015-06-16 15:45:53 UTC
Got a new spec/srpm links for me?

(hint: they shouldn't be the same as before, since you bumped Release, and added changelog entries, right? :) )

Comment 21 Graham White 2015-06-16 18:27:10 UTC
Spec URL: https://grahamwhiteuk.fedorapeople.org/pkgreviews/gimp-lensfun/gimp-lensfun.spec
SRPM URL: https://grahamwhiteuk.fedorapeople.org/pkgreviews/gimp-lensfun/gimp-lensfun-0.2.3-1.fc22.src.rpm

I've updated the spec file at the URL above and uploaded the SRPM too.

I did add a changelog entry but didn't bump the release number.  Since this has never been release I figured the release number means nothing at this stage.

Comment 22 Graham White 2015-06-16 18:28:34 UTC
Damn and blast, I've just realised I left the "Requires" in the spec file... scratch that lot then, I'll try again!

Comment 24 Rex Dieter 2015-06-16 19:27:53 UTC
Thanks, APPROVED (and sponsored)

For future reviews, make sure to increment Release: tag too whenever you make changes.

Next steps,
http://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Add_Package_to_Source_Code_Management_.28SCM.29_system_and_Set_Owner

Let me know if you need anything else.

Comment 25 Graham White 2015-06-16 20:16:25 UTC
New Package SCM Request
=======================
Package Name: gimp-lensfun
Short Description: Gimp plugin to correct lens distortion
Upstream URL: http://seebk.github.io/GIMP-Lensfun/
Owners: grahamwhiteuk
Branches: f21 f22 f23
InitialCC:

Comment 26 Gwyn Ciesla 2015-06-17 01:34:56 UTC
Git done (by process-git-requests).

Comment 27 Fedora Update System 2015-06-19 10:18:41 UTC
gimp-lensfun-0.2.3-1.fc21 has been submitted as an update for Fedora 21.
https://admin.fedoraproject.org/updates/gimp-lensfun-0.2.3-1.fc21

Comment 28 Fedora Update System 2015-06-19 10:20:16 UTC
gimp-lensfun-0.2.3-1.fc22 has been submitted as an update for Fedora 22.
https://admin.fedoraproject.org/updates/gimp-lensfun-0.2.3-1.fc22

Comment 29 Fedora Update System 2015-06-20 23:56:59 UTC
gimp-lensfun-0.2.3-1.fc22 has been pushed to the Fedora 22 testing repository.

Comment 30 Fedora Update System 2015-07-13 19:10:57 UTC
gimp-lensfun-0.2.3-1.fc22 has been pushed to the Fedora 22 stable repository.

Comment 31 Fedora Update System 2015-07-13 19:14:42 UTC
gimp-lensfun-0.2.3-1.fc21 has been pushed to the Fedora 21 stable repository.