Bug 459946 - Review Request: pfscalibration - Scripts and programs for photometric calibration
Summary: Review Request: pfscalibration - Scripts and programs for photometric calibra...
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Lucian Langa
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: pfstools
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-08-25 02:58 UTC by Ulrich Drepper
Modified: 2009-01-15 03:04 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-01-15 02:54:41 UTC
lucilanga: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Ulrich Drepper 2008-08-25 02:58:05 UTC
Spec URL: http://people.redhat.com/drepper/pfscalibration.spec
SRPM URL: http://people.redhat.com/drepper/pfscalibration-1.3-1.fc9.src.rpm
Description: 

PFScalibration package provides an implementation of the Robertson et al. 2003
method for the photometric calibration of cameras and for the recovery of high
dynamic range (HDR) images from the set of low dynamic range (LDR) exposures.

Comment 1 Ulrich Drepper 2008-09-03 14:28:20 UTC
Anybody going to volunteer?

Comment 2 Lucian Langa 2008-12-15 19:08:49 UTC
BuildRequires is missing pfstools-devel and libtool

Please use complete URL to the upstream tarball in Source0.

There is also a newer upstream release.

Comment 3 Ulrich Drepper 2009-01-02 20:56:00 UTC
I've updated the files.  The BuildRequires are added and I've updated to the most recent release.

Note that in this update upstream included two files with questionable (at best) copyright.  I've added a patch to not rely on those files, they are not used.  Do they have to be removed from the .src.rpm?

I have deliberately not added the complete URL.  This would defeat the mirroring sourceforge is doing.  It would be a bad idea to insist on this.  Unfortunately sourceforge doesn't provide a nice URL interface to packages.

Spec URL: http://people.redhat.com/drepper/pfscalibration.spec
SRPM URL: http://people.redhat.com/drepper/pfscalibration-1.4-1.fc10.src.rpm

Please review again.

Comment 4 Kevin Fenzi 2009-01-03 01:16:44 UTC
Not a review, but some comments: 

For the first, you should probibly exclude them if Fedora can't distribute them at all. See: http://fedoraproject.org/wiki/Packaging/SourceURL#When_Upstream_uses_Prohibited_Code

You can (and should) specify a full URL for sourceforge hosted projects. 
See: http://fedoraproject.org/wiki/Packaging/SourceURL#Sourceforge.net

Comment 5 Ulrich Drepper 2009-01-03 01:41:56 UTC
(In reply to comment #4)
> For the first, you should probibly exclude them if Fedora can't distribute them
> at all. See:
> http://fedoraproject.org/wiki/Packaging/SourceURL#When_Upstream_uses_Prohibited_Code
wiki/Packaging/SourceURL#Sourceforge.net

It's not that bad.  The code is simply copyrighted, you need a license to use it.  From what that page syas there is no reason to modify the tarball and exclude the file.  The binaries are not tainted.

Comment 6 Lucian Langa 2009-01-03 11:52:56 UTC
nrutil.cpp and nrutil.h are public domain. (http://www.nr.com/public-domain.html)
I do not think there is need for patch1.

Source0 URL is still wrong, it should be:

http://downloads.sourceforge.net/pfstools/%{name}-%{version}.tar.gz

Comment 7 Ulrich Drepper 2009-01-03 19:27:39 UTC
(In reply to comment #6)
> nrutil.cpp and nrutil.h are public domain.
> (http://www.nr.com/public-domain.html)
> I do not think there is need for patch1.

The files used in the package are not on the list.  This might be an oversight but it's really not necessary to bicker about this.  The code is using so little from these files it's absolutely unnecessary to pull in so much code. The patch is also an optimization.


> Source0 URL is still wrong, it should be:
> 
> http://downloads.sourceforge.net/pfstools/%{name}-%{version}.tar.gz

That's not what the URL referenced in comment #4 says and it shouldn't matter:

  http://downloads.sourceforge.net/%{name}/%{name}-%{version}.tar.gz

This works just as well.

Comment 8 Ulrich Drepper 2009-01-06 08:43:31 UTC
With one little change it builds even if libtool in the buildroot is changed:

Spec URL: http://people.redhat.com/drepper/pfscalibration.spec
SRPM URL: http://people.redhat.com/drepper/pfscalibration-1.4-2.fc10.src.rpm


Please review this.  pfstools is now in Fedora.  It is a prerequisite for this package and pfstools alone isn't really useful without pfscalibration and pfstmo.

Comment 9 Lucian Langa 2009-01-06 09:33:41 UTC
-Fix %changelog entry for each bump of version

pfscalibration.x86_64: W: incoherent-version-in-changelog 1.4-1 ['1.4-2.fc10', '1.4-2']

-Source0 is 404 ! (sf project name is pfstools, %{name} expands to pfscalibration)

-CXXFLAGS set to -O3 overrides Fedora's default level (-O2)

Comment 10 Ulrich Drepper 2009-01-06 18:59:56 UTC
I've fixed all the problems pointed out and then some more (correct use of OpenMP).  New versions of the files (same name):

Spec URL: http://people.redhat.com/drepper/pfscalibration.spec
SRPM URL: http://people.redhat.com/drepper/pfscalibration-1.4-2.fc10.src.rpm

Comment 11 Lucian Langa 2009-01-06 20:25:40 UTC
The compiler should be called with the full set of %{optflags} or the resulted debuginfo package will be broken.
It looks like package does not honour external set of CXXFLAGS called by rpmbuild:

+ CFLAGS='-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic'
+ export CFLAGS
+ CXXFLAGS='-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic'
+ export CXXFLAGS
+ FFLAGS='-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -I/usr/lib64/gfortran/modules'
+ export FFLAGS
+ ./configure ....

so yo needs to be patched somehow.

Please also include COPYING file in the %doc section of the package.
It would be also nice to add README files, ChangeLog and AUTHORS.

Please bump the release version of the package for each set of modification.
Thank you.

Comment 12 Ulrich Drepper 2009-01-07 09:00:40 UTC
(In reply to comment #11)
> The compiler should be called with the full set of %{optflags} or the resulted
> debuginfo package will be broken.

The flags provided are used.  There are just some additional flags passed.  Nothing got broken.

Anyway, I have changed it slightly, similar to the way it is done in the pfstmo package.  I'm now replacing the -O2 option only with some additional options for higher optimization.  These higher optimizations are appropriate in this case.

> Please also include COPYING file in the %doc section of the package.
> It would be also nice to add README files, ChangeLog and AUTHORS.

I did that.

The resulting files are here:

Spec URL: http://people.redhat.com/drepper/pfscalibration.spec
SRPM URL: http://people.redhat.com/drepper/pfscalibration-1.4-3.fc10.src.rpm

Comment 13 Lucian Langa 2009-01-07 09:58:49 UTC
(In reply to comment #12)

> The flags provided are used.  There are just some additional flags passed. 
> Nothing got broken.
- Fedora specific compilation flags are not honoured. (rpm --eval %optflags)
Overriding or filtering parts of these flags is permitted only if there's a good reason to do so.

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


> Anyway, I have changed it slightly, similar to the way it is done in the pfstmo
> package.  I'm now replacing the -O2 option only with some additional options
> for higher optimization.  These higher optimizations are appropriate in this
> case.
- -O3 flag is generally forbidden (it makes debugging difficult).

Comment 14 Ulrich Drepper 2009-01-07 18:14:54 UTC
There is a good reason for using slightly different flags and -O3 doesn't make debugging harder (where does this myth come from?).  And it makes a difference for code with is vectorizable.  This is the case here, this is purely mathematical code which can be optimized well by the compiler.

Insisting on the rules is certainly a good thing, in general.  But I know what I'm doing, I know which gcc options can benefit.

Comment 15 Lucian Langa 2009-01-08 06:51:21 UTC
Please put a small note in the spec file explaining O3 preference.

All rest of the %optflags are honoured except "-g". Stripping out "-g" 
will result in an incomplete -debuginfo package.

Comment 16 Ulrich Drepper 2009-01-08 07:06:45 UTC
Indeed, -g has been filtered out.  I missed that, sorry.  I fixed it and added a comment:

Spec URL: http://people.redhat.com/drepper/pfscalibration.spec
SRPM URL: http://people.redhat.com/drepper/pfscalibration-1.4-4.fc10.src.rpm

Comment 17 Lucian Langa 2009-01-08 10:38:44 UTC
Thanks for the update. Expect an formal review shortly.

Comment 18 Lucian Langa 2009-01-08 13:00:42 UTC
Review:

OK  source files match upstream:
        1844a6e3f03f585dbc8bcc3eaacf66b9  pfscalibration-1.4.tar.gz
OK  package meets naming and versioning guidelines.
OK  specfile is properly named, is cleanly written and uses macros consistently.
OK  summary is OK.
OK  description is OK.
OK  dist tag is present.
OK  build root is OK.
OK  license field matches the actual license.
OK  license is open source-compatible.
OK  license text included in package.
OK  BuildRequires are proper.
OK  compiler flags are appropriate.
        -O3 preference was explained in specfile
        -g is now present, debuginfo package complete.
OK  %clean is present.
OK  package builds in mock (rawhide, x86_64).
OK  package installs properly.
OK  debuginfo package looks complete.
OK  rpmlint is silent.
OK  final provides and requires are sane:
        pfscalibration = 1.4-4.fc10
        pfscalibration(x86-64) = 1.4-4.fc10
        =
        /bin/bash
        /usr/bin/perl
        dcraw
        jhead
        libc.so.6()(64bit)
        libgcc_s.so.1()(64bit)
        libgomp.so.1()(64bit)
        libm.so.6()(64bit)
        libpfs-1.2.so.0()(64bit)
        libpthread.so.0()(64bit)
        libstdc++.so.6()(64bit)
        perl
        perl(Getopt::Long)
        rpmlib(CompressedFileNames) <= 3.0.4-1
        rpmlib(PayloadFilesHavePrefix) <= 4.0-1
        rtld(GNU_HASH)
N/A no shared libraries are added to the regular linker search paths.
N/A owns the directories it creates.
OK  doesn't own any directories it shouldn't.
OK  no duplicates in %files.
OK  file permissions are appropriate.
OK  code, not content.
OK  documentation is small, so no -doc subpackage is necessary.
OK  %docs are not necessary for the proper functioning of the package.
OK  no headers.
OK  no pkgconfig files.
OK  no static libraries.
OK  no libtool .la files.

remove duplicated %doc at the end of %files section.

APPROVED.

Comment 19 Ulrich Drepper 2009-01-08 15:23:26 UTC
New Package CVS Request
=======================
Package Name: pfscalibration
Short Description: Scripts and programs for photometric calibration
Owners: drepper@redhat.com
Branches: F-9 F-10
InitialCC:

Comment 20 Kevin Fenzi 2009-01-09 05:31:16 UTC
cvs done.

Comment 21 Fedora Update System 2009-01-09 07:57:27 UTC
pfscalibration-1.4-5.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/pfscalibration-1.4-5.fc10

Comment 22 Fedora Update System 2009-01-09 07:57:31 UTC
pfscalibration-1.4-5.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/pfscalibration-1.4-5.fc9

Comment 23 Fedora Update System 2009-01-15 02:54:36 UTC
pfscalibration-1.4-5.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 24 Fedora Update System 2009-01-15 03:04:25 UTC
pfscalibration-1.4-5.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.


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