Thanks for packaging this, Marcin.  I've successfully built locally and am waiting for a koji build to complete now.  Here are some initial notes on the package, though:

* The most serious problem is bundling.  The source directory contains (apparently) bundled BLAS and LAPACK implementations, a bundled libxc file, and (as far as I can tell) Jack Dongarra's modified version of a public domain implementation of the error function from the NSWC Library of Mathematical Subroutines.  This kind of bundling seems to be cultural in the Fortran world, but you will need to remove these in %prep and build against versions available in Fedora.

* The %if here appears vestigial; is that the case?

    %if 0%{?fedora} >= 21
    %global BLAS -L%{_libdir} -lopenblas
    %global LAPACK -L%{_libdir} -llapack
    %else
    %global BLAS -L%{_libdir} -lopenblas
    %global LAPACK -L%{_libdir} -llapack
    %endif

* The %build section is awfully hairy, but it's not obvious that you could make it a lot simpler without involving upstream.  It looks like you've been using this spec for some time; do you have a sense for how robust your macros are here?

* In %check, I think it would make more sense to use an environment variable for NPROC (I had to get a clarification on when this macro body would be evaluated):

    %global NPROC %(cat /proc/cpuinfo | grep processor | wc -l)

Please let me know once you have a chance to address these and I'll continue with the review.

(In reply to Will Benton from comment #1)
> * The %if here appears vestigial; is that the case?
> 
>     %if 0%{?fedora} >= 21
>     %global BLAS -L%{_libdir} -lopenblas
>     %global LAPACK -L%{_libdir} -llapack
>     %else
>     %global BLAS -L%{_libdir} -lopenblas
>     %global LAPACK -L%{_libdir} -llapack
>     %endif

This is also incorrect. -lopenblas already contains LAPACK libraries, and further linkage against reference LAPACK can only lead to trouble.

(In reply to Will Benton from comment #1)
> Thanks for packaging this, Marcin.  I've successfully built locally and am
> waiting for a koji build to complete now.  Here are some initial notes on
> the package, though:

thanks

> 
> * The most serious problem is bundling.  The source directory contains
> (apparently) bundled BLAS and LAPACK implementations, a bundled libxc file,
> and (as far as I can tell) Jack Dongarra's modified version of a public
> domain implementation of the error function from the NSWC Library of
> Mathematical Subroutines.  This kind of bundling seems to be cultural in the
> Fortran world, but you will need to remove these in %prep and build against
> versions available in Fedora.

in fact there was only one clearly "bundled" library (apart from erf) used by elk: fftw.
BLAS and LAPACK were compiled but not used - i removed the source dirs
in %prep and modified the makefile to show this explicitly. libxc was not bundled (in a sense being included in the elk source an compiled and linked as such), but elk included some fortran modules from libxc for interface purposes.

I have switched to libxc-devel provided fortran modules, at the cost of having -I%{_fmoddir} in gfortran compilation options. I switched also to fftw3.

As for erf.f90 - it looks like erf is used only in src/stheta_mp.f90 so i just removed the "external erf" from there + rm src/erf.f90.

> 
> * The %if here appears vestigial; is that the case?
> 
>     %if 0%{?fedora} >= 21
>     %global BLAS -L%{_libdir} -lopenblas
>     %global LAPACK -L%{_libdir} -llapack
>     %else
>     %global BLAS -L%{_libdir} -lopenblas
>     %global LAPACK -L%{_libdir} -llapack
>     %endif
> 

this part is cleaned, and superfluous -llapack removed

> * The %build section is awfully hairy, but it's not obvious that you could
> make it a lot simpler without involving upstream.  It looks like you've been
> using this spec for some time; do you have a sense for how robust your
> macros are here?

i think elk build instructions have not changed since the introduction
of libxc interface to elk (~3 years ago), but the module-load-dependent parts
relevant for Fedora are new - in the past i was building just one openmpi version.
The length of those build/install/check parts is a consequence of requirements (http://fedoraproject.org/wiki/Packaging:MPI#Packaging_of_MPI_software) that one should try to build all supported mpi (i don't build with mvapich on el6 anyway - as this would introduce even more logic into the spec).

> 
> * In %check, I think it would make more sense to use an environment variable
> for NPROC (I had to get a clarification on when this macro body would be
> evaluated):
> 
>     %global NPROC %(cat /proc/cpuinfo | grep processor | wc -l)

i do now: export NPROC=2

> 
> Please let me know once you have a chance to address these and I'll continue
> with the review.

Spec URL: http://marcindulak.fedorapeople.org/packages/elk/r03/elk.spec
SRPM URL: http://marcindulak.fedorapeople.org/packages/elk/r03/elk-2.2.10-3.fc21.src.rpm

koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=6543991

Additional comment: it looks like most test failures are due to OMP_NUM_THREADS!=1, but i leave the tests as is for now - elk is supposed to handle openmp.

(In reply to marcindulak from comment #3)
> > * In %check, I think it would make more sense to use an environment variable
> > for NPROC (I had to get a clarification on when this macro body would be
> > evaluated):
> > 
> >     %global NPROC %(cat /proc/cpuinfo | grep processor | wc -l)
> 
> i do now: export NPROC=2

Another alternative would be to use %{_smp_mflags} to define NPROC.

(In reply to marcindulak from comment #3)

> in fact there was only one clearly "bundled" library (apart from erf) used
> by elk: fftw.
> BLAS and LAPACK were compiled but not used - i removed the source dirs
> in %prep and modified the makefile to show this explicitly. libxc was not
> bundled (in a sense being included in the elk source an compiled and linked
> as such), but elk included some fortran modules from libxc for interface
> purposes.
> 
> I have switched to libxc-devel provided fortran modules, at the cost of
> having -I%{_fmoddir} in gfortran compilation options. I switched also to
> fftw3.

Thanks for making these changes!  (Packages do need to delete bundled source in %prep even if it is not linked in as part of the build; see:  http://fedoraproject.org/wiki/Packaging:Treatment_Of_Bundled_Libraries)

As far as NPROC goes, it is probably OK to define it as 2, or to extract a number out from %{_smp_mflags} as Susi Lehtola suggests (which would give you 3 on a uniprocessor machine), or to define it as

    export NPROC=$(cat /proc/cpuinfo | grep -c processor)

or whatever; it's a matter of taste.

I am double-checking everything else and will finish the review when I have.

Review granted; thanks again!

(In reply to Will Benton from comment #5)
> Thanks for making these changes!  (Packages do need to delete bundled source
> in %prep even if it is not linked in as part of the build; see: 
> http://fedoraproject.org/wiki/Packaging:Treatment_Of_Bundled_Libraries)

You're misreading. 

"Bundled libraries (and/or their source code) must be explicitly deleted during %prep."

(In reply to Susi Lehtola from comment #7)
> (In reply to Will Benton from comment #5)
> > Thanks for making these changes!  (Packages do need to delete bundled source
> > in %prep even if it is not linked in as part of the build; see: 
> > http://fedoraproject.org/wiki/Packaging:Treatment_Of_Bundled_Libraries)
> 
> You're misreading. 
> 
> "Bundled libraries (and/or their source code) must be explicitly deleted
> during %prep."

I think you may be misreading what I wrote, since I said that bundled library sources must be deleted in %prep.  I was clarifying for Marcin that even if the binaries his spec built didn't link in bundled libraries, he would still have to delete the source for those libraries in %prep.  The original spec did not do this, but the current one does.

(In reply to Will Benton from comment #8)
> I think you may be misreading what I wrote, since I said that bundled
> library sources must be deleted in %prep.  I was clarifying for Marcin that
> even if the binaries his spec built didn't link in bundled libraries, he
> would still have to delete the source for those libraries in %prep.  The
> original spec did not do this, but the current one does.

... sorry, you are right. I must have jumped over the critical part :)

(In reply to Susi Lehtola from comment #9)

> ... sorry, you are right. I must have jumped over the critical part :)

No worries, and thanks for your feedback on this review!

New Package SCM Request
=======================
Package Name: elk
Short Description: FP-LAPW Code
Owners: marcindulak
Branches: f19 f20 el6 epel7
InitialCC:

Git done (by process-git-requests).

elk-2.2.10-3.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/elk-2.2.10-3.fc19

elk-2.2.10-3.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/elk-2.2.10-3.fc20

elk-2.2.10-3.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/elk-2.2.10-3.el6

elk-2.2.10-3.el6 has been pushed to the Fedora EPEL 6 testing repository.

elk-2.2.10-3.fc20 has been pushed to the Fedora 20 stable repository.

elk-2.2.10-3.fc19 has been pushed to the Fedora 19 stable repository.

elk-2.2.10-3.el6 has been pushed to the Fedora EPEL 6 stable repository.