Anybody going to volunteer?

BuildRequires is missing pfstools-devel and libtool

Please use complete URL to the upstream tarball in Source0.

There is also a newer upstream release.

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.

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

(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.

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

(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.

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.

-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)

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

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.

(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

(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).

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.

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.

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

Thanks for the update. Expect an formal review shortly.

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.

New Package CVS Request
=======================
Package Name: pfscalibration
Short Description: Scripts and programs for photometric calibration
Owners: drepper
Branches: F-9 F-10
InitialCC:

cvs done.

pfscalibration-1.4-5.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/pfscalibration-1.4-5.fc10

pfscalibration-1.4-5.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/pfscalibration-1.4-5.fc9

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.

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.