Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 1064656 - Review Request: elk - FP-LAPW Code
Summary: Review Request: elk - FP-LAPW Code
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Will Benton
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2014-02-13 03:33 UTC by marcindulak
Modified: 2014-03-08 20:59 UTC (History)
3 users (show)

Fixed In Version: elk-2.2.10-3.el6
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2014-03-03 03:08:08 UTC
willb: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description marcindulak 2014-02-13 03:33:50 UTC
Spec URL: http://marcindulak.fedorapeople.org/packages/elk/r02/elk.spec
SRPM URL: http://marcindulak.fedorapeople.org/packages/elk/r02/elk-2.2.10-2.fc21.src.rpm
Description: An all-electron full-potential linearised augmented-plane wave (FP-LAPW) code with many advanced features.
Fedora Account System Username: marcindulak

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

Comment 1 Will Benton 2014-02-13 17:10:37 UTC
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.

Comment 2 Susi Lehtola 2014-02-14 13:22:20 UTC
(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.

Comment 3 marcindulak 2014-02-18 19:28:38 UTC
(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.

Comment 4 Susi Lehtola 2014-02-18 19:36:18 UTC
(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.

Comment 5 Will Benton 2014-02-19 02:03:32 UTC
(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.

Comment 6 Will Benton 2014-02-19 03:10:20 UTC
Review granted; thanks again!

Comment 7 Susi Lehtola 2014-02-19 07:36:56 UTC
(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."

Comment 8 Will Benton 2014-02-19 13:20:45 UTC
(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.

Comment 9 Susi Lehtola 2014-02-19 13:50:36 UTC
(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 :)

Comment 10 Will Benton 2014-02-19 14:37:15 UTC
(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!

Comment 11 marcindulak 2014-02-20 12:41:42 UTC
New Package SCM Request
=======================
Package Name: elk
Short Description: FP-LAPW Code
Owners: marcindulak
Branches: f19 f20 el6 epel7
InitialCC:

Comment 12 Gwyn Ciesla 2014-02-20 13:51:53 UTC
Git done (by process-git-requests).

Comment 13 Fedora Update System 2014-02-20 20:45:26 UTC
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

Comment 14 Fedora Update System 2014-02-20 20:47:01 UTC
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

Comment 15 Fedora Update System 2014-02-20 20:47:39 UTC
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

Comment 16 Fedora Update System 2014-02-22 01:50:39 UTC
elk-2.2.10-3.el6 has been pushed to the Fedora EPEL 6 testing repository.

Comment 17 Fedora Update System 2014-03-03 03:08:08 UTC
elk-2.2.10-3.fc20 has been pushed to the Fedora 20 stable repository.

Comment 18 Fedora Update System 2014-03-03 03:11:00 UTC
elk-2.2.10-3.fc19 has been pushed to the Fedora 19 stable repository.

Comment 19 Fedora Update System 2014-03-08 20:59:39 UTC
elk-2.2.10-3.el6 has been pushed to the Fedora EPEL 6 stable repository.


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