Bug 497622

- What's this for?

%if %{fedora} < 11
%exclude %{_bindir}/psize.pyc
%exclude %{_bindir}/psize.pyo
%endif

- Instead of
 pushd bin
 make install DESTDIR=$RPM_BUILD_ROOT INSTALL='install -p' CPPROG="cp -p"
 popd
and so onyou can just use
 make -C bin install DESTDIR=$RPM_BUILD_ROOT INSTALL='install -p' CPPROG="cp -p"
 make -C src/aaa_lib install DESTDIR=$RPM_BUILD_ROOT INSTALL='install -p' CPPROG="cp -p"


- Why do you generate autoconf files, aren't they distributed in the tarball..?

(In reply to comment #1)
> - What's this for?
> 
> %if %{fedora} < 11
> %exclude %{_bindir}/psize.pyc
> %exclude %{_bindir}/psize.pyo
> %endif
> 

A dumb way to prevent byte compiling .py scripts that reside in /usr/bin on F10 but not on -devel:

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=182498

When I have a chance to build the srpm on my devel machine, i can take this part out.

> - Instead of
>  pushd bin
>  make install DESTDIR=$RPM_BUILD_ROOT INSTALL='install -p' CPPROG="cp -p"
>  popd
> and so onyou can just use
>  make -C bin install DESTDIR=$RPM_BUILD_ROOT INSTALL='install -p' CPPROG="cp
> -p"
>  make -C src/aaa_lib install DESTDIR=$RPM_BUILD_ROOT INSTALL='install -p'
> CPPROG="cp -p"
> 

ah, thanks.

> 
> - Why do you generate autoconf files, aren't they distributed in the tarball..?  

Only because I patch the configure.ac and makefile.am files, so the autoconf stuff needs to be regenerated.

rpmlint output:

apbs.src:53: W: setup-not-quiet
apbs-devel.x86_64: W: no-documentation
4 packages and 0 specfiles checked; 0 errors, 2 warnings.

- Use "%setup -q" to fix the first warning.

- Your patches are incorrectly named, add a abps- prefix to them. Send the patches upstream.


MUST: The package does not yet exist in Fedora. The Review Request is not a duplicate. OK
MUST: The spec file for the package is legible and macros are used consistently. NEEDSFIX
- You are mixing $RPM_BUILD_ROOT and %{buildroot}, which is not allowed.
- Please add comments about why the patches are necessary.
- You can shorten the install section with the hints given above.
- Use the same description as in the review request. The one you have now in the SPEC is a bit too long and contains unnecessary information (the last two paragraphs).

MUST: The package must be named according to the Package Naming Guidelines. OK
MUST: The spec file name must match the base package %{name}. OK
MUST: The package must be licensed with a Fedora approved license and meet the  Licensing Guidelines. OK

MUST: The License field in the package spec file must match the actual license.
- Stuff under contrib/ seems to be licensed under LGPLv2+, but nothing seems to be packaged from there.

MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. OK
MUST: The package MUST successfully compile and build into binary rpms. OK
MUST: The spec file MUST handle locales properly. OK
MUST: Optflags are used and time stamps preserved. OK
MUST: Packages containing shared library files must call ldconfig. OK
MUST: A package must own all directories that it creates or require the package that owns the directory. OK
MUST: Files only listed once in %files listings. OK
MUST: Permissions on files must be set properly. OK
MUST: Clean section exists. OK
MUST: Large documentation files must go in a -doc subpackage. OK

MUST: All relevant items are included in %doc. Items in %doc do not affect runtime of application. NEEDSFIX
- Drop INSTALL and NEWS from %doc, include README.

MUST: Header files must be in a -devel package. OK
MUST: Static libraries must be in a -static package. OK
MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'. OK
MUST: If a package contains library files with a suffix then library files ending in .so must go in a -devel package. OK
MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency. OK
MUST: Packages does not contain any .la libtool archives. OK
MUST: Desktop files are installed properly. OK
MUST: No file conflicts with other packages and no general names. OK
MUST: Buildroot cleaned before install. OK
SHOULD: %{?dist} tag is used in release. OK
SHOULD: If the package does not include license text(s) as separate files from upstream, the packager should query upstream to include it. OK
SHOULD: The package builds in mock. OK

(In reply to comment #3)
> rpmlint output:
> 
> apbs.src:53: W: setup-not-quiet
> apbs-devel.x86_64: W: no-documentation
> 4 packages and 0 specfiles checked; 0 errors, 2 warnings.
> 
> - Use "%setup -q" to fix the first warning.
> 

done.

> - Your patches are incorrectly named, add a abps- prefix to them. Send the
> patches upstream.
> 

done and done.

> 
> MUST: The spec file for the package is legible and macros are used
> consistently. NEEDSFIX
> - You are mixing $RPM_BUILD_ROOT and %{buildroot}, which is not allowed.
> - Please add comments about why the patches are necessary.
> - You can shorten the install section with the hints given above.
> - Use the same description as in the review request. The one you have now in
> the SPEC is a bit too long and contains unnecessary information (the last two
> paragraphs).
> 

done.

> 
> MUST: The License field in the package spec file must match the actual license.
> - Stuff under contrib/ seems to be licensed under LGPLv2+, but nothing seems to
> be packaged from there.
> 

the libraries that are built in contrib/ are lumped into libapbs.so* (along with some of the BSD licensed functions) - I've made note of the dual license, is it sufficient/OK to annotate the .so files as "BSD and LGPLv2+" in the %files section and add a PACKAGE-LICENSING file?

> 
> MUST: All relevant items are included in %doc. Items in %doc do not affect
> runtime of application. NEEDSFIX
> - Drop INSTALL and NEWS from %doc, include README.
> 

done.

Updated files:
Spec URL: http://www.stanford.edu/~fenn/packs/apbs.spec
SRPM URL: http://www.stanford.edu/~fenn/packs/apbs-1.1.0-2.fc10.src.rpm

(In reply to comment #4)
> > MUST: The License field in the package spec file must match the actual license.
> > - Stuff under contrib/ seems to be licensed under LGPLv2+, but nothing seems to
> > be packaged from there.
> > 
> 
> the libraries that are built in contrib/ are lumped into libapbs.so* (along
> with some of the BSD licensed functions) - I've made note of the dual license,
> is it sufficient/OK to annotate the .so files as "BSD and LGPLv2+" in the
> %files section and add a PACKAGE-LICENSING file?

Damn. Then the current packaging is a no-go: you must strip apbs of the contrib packages and package them separately.

At least maloc and pmg have upstreams here: http://fetk.org/

Btw, you're maintaining PyMOL, right? Does it include an own copy of apbs? If so, it should be modified to use this package instead.

(In reply to comment #5)
> (In reply to comment #4)
> > > MUST: The License field in the package spec file must match the actual license.
> > > - Stuff under contrib/ seems to be licensed under LGPLv2+, but nothing seems to
> > > be packaged from there.
> > > 
> > 
> > the libraries that are built in contrib/ are lumped into libapbs.so* (along
> > with some of the BSD licensed functions) - I've made note of the dual license,
> > is it sufficient/OK to annotate the .so files as "BSD and LGPLv2+" in the
> > %files section and add a PACKAGE-LICENSING file?
> 
> Damn. Then the current packaging is a no-go: you must strip apbs of the contrib
> packages and package them separately.
> 
> At least maloc and pmg have upstreams here: http://fetk.org/
> 

Argh - OK, I'll talk with upstream, who I believe may be able to correct this.

> Btw, you're maintaining PyMOL, right?

Yes.

> Does it include an own copy of apbs?

No - thus the reason for this package.  :(

ping, any progress?

(In reply to comment #7)
> ping, any progress?  
>

upstream suggests separating the libraries out.

the packages that would require this:

pmgZ: GPLv2+ (http://www.fetk.org/codes/pmg/index.html)
maloc: GPLv2+ (http://www.fetk.org/codes/maloc/index.html)
aqua: no license info available, can't locate source online
opal: LBNLBSD (http://nbcr.net/software/opal/)
python-ZSI: already available for fedora.

the last two aren't required for the apbs binary, only for server-client applications utilizing apbs.

I should also note the licenses are different some of the web based downloads than those included with apbs (e.g. maloc carries LGPLv2+ headers in the apbs download).

I'll get in touch with upstream again re. the licensing/availability of aqua.

Still waiting for info regarding the licensing on some of the subpackages, aqua is apparently LGPLv2+ but seems unavailable other than as part of APBS.

ping?

Everything is all set except that aqua seems to only be available as part of the apbs download.  However, its LGPLv2+, which doesn't jive with apbs. Should I just pull aqua out of apbs' SVN repo and make it its own package that way?

Hmm, it seems that aqua is a customized version of PMG as per http://cardon.wustl.edu/MediaWiki/index.php/APBS .

In that case I'm really not sure what should be done. Many possibilities come to mind

- package aqua separately [if it has development that is clearly separate from apbs]
- create a aqua subpackage [development as part of apbs but still other apps might use it]
- don't package aqua at all [an incompatible fork of the PMG library that is used nowhere else]

(In reply to comment #12)
> Hmm, it seems that aqua is a customized version of PMG as per
> http://cardon.wustl.edu/MediaWiki/index.php/APBS .
> 
> In that case I'm really not sure what should be done. Many possibilities come
> to mind
> 
> - package aqua separately [if it has development that is clearly separate from
> apbs]
> - create a aqua subpackage [development as part of apbs but still other apps
> might use it]
> - don't package aqua at all [an incompatible fork of the PMG library that is
> used nowhere else]  

Oh, this is getting fun.  I'll go with option 2, since Nathan seems to have taken over the aqua development anyway. It looks like this will also be necessary with pmgZ, as again - it seems to primarily be developed as part of apbs, not really its own independent library. Only maloc can really be its "own" library - Mike Holst still manages that.

Here's a shot at an rpm for pmgZ - I can't seem to find much in the way of guidelines for subpackages in this sort of instance - should independent package requests be made for each (with blockers added for this package)?

Spec URL: http://www.stanford.edu/~fenn/packs/pmgz.spec
SRPM URL: http://www.stanford.edu/~fenn/packs/apbs-pmgz-1.1.0-1.20090706svn1360.fc11.src.rpm

If the releases are made along apbs, then you can just build them all in the same spec and do e.g.
 %package -n aqua
and 
 %package -n pmgZ
and so on...

Probably best to ask first upstream, though, if those are meant to be distributed as individual packages.

(In reply to comment #14)
> If the releases are made along apbs, then you can just build them all in the
> same spec and do e.g.
>  %package -n aqua
> and 
>  %package -n pmgZ
> and so on...
> 
> Probably best to ask first upstream, though, if those are meant to be
> distributed as individual packages.  

contacted (out of office until the 14th, apparently) - it seems like building dependencies and then using said deps in a binary build in a single spec file would be difficult to do - are there any examples of this to go by?

(In reply to comment #15)
> contacted (out of office until the 14th, apparently) - it seems like building
> dependencies and then using said deps in a binary build in a single spec file
> would be difficult to do - are there any examples of this to go by?  

Doesn't it build them automatically, or do you end up with a static library? Shipping that would be one option..

But basically what you need to do is first build the dependency and install it in the build directory (say, %{_builddir}/aqua) and set the linker and include paths so that they use the aqua in that directory.

In %install you'd then install aqua manually in %{buildroot} alongside with the normal 'make install' of apbs.

(In reply to comment #16)
> (In reply to comment #15)
> > contacted (out of office until the 14th, apparently) - it seems like building
> > dependencies and then using said deps in a binary build in a single spec file
> > would be difficult to do - are there any examples of this to go by?  
> 
> Doesn't it build them automatically, or do you end up with a static library?
> Shipping that would be one option..
> 

it builds them automatically, then glues them together into one big libapbs.so library. So I could patch the makefiles to not include them in libapbs.so and add the necessary deps to the apbs binary build.  Perhaps that would be easiest.

> But basically what you need to do is first build the dependency and install it
> in the build directory (say, %{_builddir}/aqua) and set the linker and include
> paths so that they use the aqua in that directory.
> 

Or this, but I'd still need to patch the makefiles to not include aqua/pmg/etc into libapbs.

> In %install you'd then install aqua manually in %{buildroot} alongside with 
> the normal 'make install' of apbs.  

OK, I think I have an idea of what to try now.  More later.

I've made the maloc package as a separate submission:

https://bugzilla.redhat.com/show_bug.cgi?id=510822

And soon I'll have a version of apbs compatible with this and the current list of recommendations.

OK, here's a shot at the split version of the package:

Spec URL: http://www.stanford.edu/~fenn/packs/apbs.spec
SRPM URL: http://www.stanford.edu/~fenn/packs/apbs-1.1.0-3.fc10.src.rpm

notes:
circular dep. problems:
- pmgZ links against aqua and vice versa - i linked aqua against pmgZ, but pmgZ has an undefined non-weak-symbol as a result.
- pmgZ and aqua both want to link to a library (src/mg) built in the apbs package, so both pmgZ and aqua have undefined non-weak-symbols that reflect this.

I'll mention all this upstream, FWIW.

(In reply to comment #19)
> notes:
> circular dep. problems:
> - pmgZ links against aqua and vice versa - i linked aqua against pmgZ, but pmgZ
> has an undefined non-weak-symbol as a result.

Maybe you can rebuild the former to get it use the latter library?

> - pmgZ and aqua both want to link to a library (src/mg) built in the apbs
> package, so both pmgZ and aqua have undefined non-weak-symbols that reflect
> this.
>
> I'll mention all this upstream, FWIW.  

Ugh. This really looks like they're not intended to be broken up but should be just shipped in the main package, if they still are available in libapbs...

(In reply to comment #20)
> (In reply to comment #19)
> > notes:
> > circular dep. problems:
> > - pmgZ links against aqua and vice versa - i linked aqua against pmgZ, but pmgZ
> > has an undefined non-weak-symbol as a result.
> 
> Maybe you can rebuild the former to get it use the latter library?
> 

That could work, sure.

> > - pmgZ and aqua both want to link to a library (src/mg) built in the apbs
> > package, so both pmgZ and aqua have undefined non-weak-symbols that reflect
> > this.
> >
> > I'll mention all this upstream, FWIW.  
> 
> Ugh. This really looks like they're not intended to be broken up but should be
> just shipped in the main package, if they still are available in libapbs...  

Yeah, the choices here aren't that great, but I think I'd rather keep them separated with some broken linkages than break the fedora licensing... right?

(In reply to comment #21)
> > > - pmgZ and aqua both want to link to a library (src/mg) built in the apbs
> > > package, so both pmgZ and aqua have undefined non-weak-symbols that reflect
> > > this.
> > >
> > > I'll mention all this upstream, FWIW.  
> > 
> > Ugh. This really looks like they're not intended to be broken up but should be
> > just shipped in the main package, if they still are available in libapbs...  
> 
> Yeah, the choices here aren't that great, but I think I'd rather keep them
> separated with some broken linkages than break the fedora licensing... right?  

What do you mean, break Fedora licensing..?

(In reply to comment #22)
> (In reply to comment #21)
> > > > - pmgZ and aqua both want to link to a library (src/mg) built in the apbs
> > > > package, so both pmgZ and aqua have undefined non-weak-symbols that reflect
> > > > this.
> > > >
> > > > I'll mention all this upstream, FWIW.  
> > > 
> > > Ugh. This really looks like they're not intended to be broken up but should be
> > > just shipped in the main package, if they still are available in libapbs...  
> > 
> > Yeah, the choices here aren't that great, but I think I'd rather keep them
> > separated with some broken linkages than break the fedora licensing... right?  
> 
> What do you mean, break Fedora licensing..?  

Sorry, that didn't sound right - I meant combining libaqua/pmgZ (LGPLv2+) with libapbs (BSD) into a single library isn't compatible with fedora licensing?

(In reply to comment #23)
> > > Yeah, the choices here aren't that great, but I think I'd rather keep them
> > > separated with some broken linkages than break the fedora licensing... right?  
> > 
> > What do you mean, break Fedora licensing..?  
> 
> Sorry, that didn't sound right - I meant combining libaqua/pmgZ (LGPLv2+) with
> libapbs (BSD) into a single library isn't compatible with fedora licensing?  

No problem, BSD is compatible with LGPL, so the result is LGPLv2+.

(In reply to comment #24)
> (In reply to comment #23)
> > > > Yeah, the choices here aren't that great, but I think I'd rather keep them
> > > > separated with some broken linkages than break the fedora licensing... right?  
> > > 
> > > What do you mean, break Fedora licensing..?  
> > 
> > Sorry, that didn't sound right - I meant combining libaqua/pmgZ (LGPLv2+) with
> > libapbs (BSD) into a single library isn't compatible with fedora licensing?  
> 
> No problem, BSD is compatible with LGPL, so the result is LGPLv2+.  

I did not know that.  Well, here's the combined version:

Spec URL: http://www.stanford.edu/~fenn/packs/apbs.spec
SRPM URL: http://www.stanford.edu/~fenn/packs/apbs-1.1.0-4.fc10.src.rpm

apbs has support for:
  --with-mpich=PATH       toplevel MPICH directory
  --with-mpich2=PATH      toplevel MPICH2 directory
  --with-lam=PATH         toplevel LAM-MPI directory
  --with-openmpi          enable OpenMPI compilation
so I suggest you add these to the package in the future.

I have suggested an MPI packaging draft and an environment modules packaging draft, which would standardize the way MPI stuff is packaged.

- Add BR: arpack-devel and --with-arpack to enable support for ARPACK.

- Add BR: python-devel and --with-python to enable support for Python.

- Remove maloc in the %prep phase after %setup to make sure it isn't used.

- There are tests in examples/, run them in %check with

%check
for dir in examples/*/;
 do make -C $dir test
done

**

rpmlint output:
apbs.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libapbsmainroutines.so.1.0.0 /usr/lib64/libblas.so.3
apbs-devel.x86_64: W: no-documentation
2 packages and 0 specfiles checked; 0 errors, 2 warnings.

- Fix the unused-direct-shlib-dependency with
https://fedoraproject.org/wiki/Common_Rpmlint_issues#unused-direct-shlib-dependency

- The -devel warning is expected.


MUST: The package does not yet exist in Fedora. The Review Request is not a duplicate. OK
MUST: The spec file for the package is legible and macros are used consistently. OK
MUST: The package must be named according to the Package Naming Guidelines. OK
MUST: The spec file name must match the base package %{name}. OK
MUST: The package must be licensed with a Fedora approved license and meet the  Licensing Guidelines. OK

MUST: The License field in the package spec file must match the actual license. OK
- License of pmgZ, aqua and contrib/blas/mblasd.f is LGPLv2+, the rest is BSD.
- License tag can be either "LGPLv2+ and BSD" or "LGPLv2+".

MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. OK
MUST: The package MUST successfully compile and build into binary rpms. OK
MUST: The spec file MUST handle locales properly. N/A
MUST: Optflags are used and time stamps preserved. OK
MUST: Packages containing shared library files must call ldconfig. OK

MUST: A package must own all directories that it creates or require the package that owns the directory. OK
- I strongly suggest using
 %{_libdir}/libapbs.so.*
 %{_libdir}/libapbsmainroutines.so.*
instead of
 %{_libdir}/*apbs.so.*
 %{_libdir}/*mainroutines.so.*
and the same thing for the .so files in -devel. Also use
 %{_bindir}/apbs
 %{_bindir}/psize.py
instead of 
 %{_bindir}/*

MUST: Files only listed once in %files listings. OK
MUST: Debuginfo package is complete. OK
MUST: Permissions on files must be set properly. OK

MUST: Clean section exists. OK
- The current clean section is a bit silly, don't you think?
 rm -rf %{_builddir}/pmgZ
 rm -rf %{_builddir}/aqua
 rm -rf %{buildroot}
Drop the two first lines.

MUST: Large documentation files must go in a -doc subpackage. NEEDSWORK
- doc/programmer/html is 14 MB, and needs to go in a -doc subpackage.
- examples/ is also 14 MB, but I don't think it should go in as it would need some work since the Makefiles are autogenerated and will need quite a lot of modification to work on an installed system.

MUST: All relevant items are included in %doc. Items in %doc do not affect runtime of application. OK
MUST: Header files must be in a -devel package. OK
MUST: Static libraries must be in a -static package. N/A
MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'. N/A
MUST: If a package contains library files with a suffix then library files ending in .so must go in a -devel package. OK
MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency. OK
MUST: Packages does not contain any .la libtool archives. OK
MUST: Desktop files are installed properly. N/A
MUST: No file conflicts with other packages and no general names. OK
MUST: Buildroot cleaned before install. OK
SHOULD: %{?dist} tag is used in release. OK

SHOULD: If the package does not include license text(s) as separate files from upstream, the packager should query upstream to include it. NEEDSWORK
- Included COPYING is BSD, LGPLv2+ COPYING is missing.

SHOULD: The package builds in mock. OK

(In reply to comment #26)
> apbs has support for:
>   --with-mpich=PATH       toplevel MPICH directory
>   --with-mpich2=PATH      toplevel MPICH2 directory
>   --with-lam=PATH         toplevel LAM-MPI directory
>   --with-openmpi          enable OpenMPI compilation
> so I suggest you add these to the package in the future.
> 
> I have suggested an MPI packaging draft and an environment modules packaging
> draft, which would standardize the way MPI stuff is packaged.
> 
> - Add BR: arpack-devel and --with-arpack to enable support for ARPACK.
> 
> - Add BR: python-devel and --with-python to enable support for Python.
> 

Adding --with-arpack enables building of a "driver" binary that is part of the tools folder, which is just copied to the share directory in the install-data-local macro, so its not required for APBS.  Would it be best to set up a -tools subpackage with all the extras? (see here:
http://cardon.wustl.edu/MediaWiki/index.php/An_overview_of_the_APBS_package#Other_tools_distributed_with_APBS
for more info) And I'm also worried about having a binary just called "driver" lying around - could that be an issue?

Adding --enable-python doesn't do anything other than build ZSI (which it shouldn't do) - which I realized, python-ZSI should be a requires so ApbsClient.py runs properly.

I'm working on the remaining issues, but I wanted to check with this before I finish it off.

(In reply to comment #27)
> Adding --with-arpack enables building of a "driver" binary that is part of the
> tools folder, which is just copied to the share directory in the
> install-data-local macro, so its not required for APBS.  Would it be best to
> set up a -tools subpackage with all the extras? (see here:
> http://cardon.wustl.edu/MediaWiki/index.php/An_overview_of_the_APBS_package#Other_tools_distributed_with_APBS
> for more info) And I'm also worried about having a binary just called "driver"
> lying around - could that be an issue?

Yes, "driver" is a no-go and must be renamed to e.g. abps-driver if packaged. If it is compiled, how is it installed in the share directory..?

Actually, you'll have to rename everythin in the tools/ folder in the package.

IMHO they should be packaged, since they are included in the abps distribution.
 
> Adding --enable-python doesn't do anything other than build ZSI (which it
> shouldn't do) - which I realized, python-ZSI should be a requires so
> ApbsClient.py runs properly.

That's odd. After all
http://cardon.wustl.edu/MediaWiki/index.php/An_overview_of_the_APBS_package#Python_development_tools

(In reply to comment #28)
> (In reply to comment #27)
> > Adding --with-arpack enables building of a "driver" binary that is part of the
> > tools folder, which is just copied to the share directory in the
> > install-data-local macro, so its not required for APBS.  Would it be best to
> > set up a -tools subpackage with all the extras? (see here:
> > http://cardon.wustl.edu/MediaWiki/index.php/An_overview_of_the_APBS_package#Other_tools_distributed_with_APBS
> > for more info) And I'm also worried about having a binary just called "driver"
> > lying around - could that be an issue?
> 
> Yes, "driver" is a no-go and must be renamed to e.g. abps-driver if packaged.
> If it is compiled, how is it installed in the share directory..?
> 

its labeled as "noinst_PROGRAMS," but in the install-data-local script in the master Makefile.am, the *entire* tools subdirectory is manually copied to ${prefix}/share.

> Actually, you'll have to rename everythin in the tools/ folder in the package.
> 
> IMHO they should be packaged, since they are included in the abps distribution.
> 

sure, I'll prepend apbs- to all the binaries and put them in a -tools subpackage.

> > Adding --enable-python doesn't do anything other than build ZSI (which it
> > shouldn't do) - which I realized, python-ZSI should be a requires so
> > ApbsClient.py runs properly.
> 
> That's odd. After all
> http://cardon.wustl.edu/MediaWiki/index.php/An_overview_of_the_APBS_package#Python_development_tools  

Right, but main and noinput.py aren't handled by any of the build scripts (except for manually copying the entire tools subdir), and there are several other .py files in the tools/python folder, so I'm not sure what to include and/or where things should go. The tools/python/vgrid folder builds a noinst_PROGRAM called _vgrid.so (which I think should be compiled as a library?) and contains several .py files that might be useful, but again are left untouched by the build scripts...

I think it would be best to just take the binaries that should obviously be included (such as the binaries in tools/mesh) and ask upstream about the more ambiguous stuff.

(In reply to comment #26)
> apbs has support for:
>   --with-mpich=PATH       toplevel MPICH directory
>   --with-mpich2=PATH      toplevel MPICH2 directory
>   --with-lam=PATH         toplevel LAM-MPI directory
>   --with-openmpi          enable OpenMPI compilation
> so I suggest you add these to the package in the future.
> 

Will do.

> I have suggested an MPI packaging draft and an environment modules packaging
> draft, which would standardize the way MPI stuff is packaged.
> 
> - Add BR: arpack-devel and --with-arpack to enable support for ARPACK.
> 
> - Add BR: python-devel and --with-python to enable support for Python.
> 

Done, and moved relevant binaries from tools into a -tools subpackage

> - Remove maloc in the %prep phase after %setup to make sure it isn't used.
> 

done.

> - There are tests in examples/, run them in %check with
> 
> %check
> for dir in examples/*/;
>  do make -C $dir test
> done
> 

some of the directories don't contain makefiles, so instead I'm using:

%check
ln -s %{buildroot}%{_bindir}/apbs bin/apbs
make -C examples test
make -C examples test-opal

This works, but the resulting examples take ~4hrs to run on my AMD opteron 275. I can cut it down to a 1-2 of the examples...

> 
> rpmlint output:
> apbs.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libapbsmainroutines.so.1.0.0 /usr/lib64/libblas.so.3
> apbs-devel.x86_64: W: no-documentation
> 2 packages and 0 specfiles checked; 0 errors, 2 warnings.
> 
> - Fix the unused-direct-shlib-dependency with
> https://fedoraproject.org/wiki/Common_Rpmlint_issues#unused-direct-shlib-dependency
> 

done.

> 
> - License of pmgZ, aqua and contrib/blas/mblasd.f is LGPLv2+, the rest is BSD.
> - License tag can be either "LGPLv2+ and BSD" or "LGPLv2+".
> 

went with the former.

> - I strongly suggest using
>  %{_libdir}/libapbs.so.*
>  %{_libdir}/libapbsmainroutines.so.*
> instead of
>  %{_libdir}/*apbs.so.*
>  %{_libdir}/*mainroutines.so.*
> and the same thing for the .so files in -devel. Also use
>  %{_bindir}/apbs
>  %{_bindir}/psize.py
> instead of 
>  %{_bindir}/*
> 

done.

> MUST: Clean section exists. OK
> - The current clean section is a bit silly, don't you think?
>  rm -rf %{_builddir}/pmgZ
>  rm -rf %{_builddir}/aqua
>  rm -rf %{buildroot}
> Drop the two first lines.
> 

oops, fixed.

> MUST: Large documentation files must go in a -doc subpackage. NEEDSWORK
> - doc/programmer/html is 14 MB, and needs to go in a -doc subpackage.
> - examples/ is also 14 MB, but I don't think it should go in as it would need
> some work since the Makefiles are autogenerated and will need quite a lot of
> modification to work on an installed system.
> 

done.

> 
> SHOULD: If the package does not include license text(s) as separate files from
> upstream, the packager should query upstream to include it. NEEDSWORK
> - Included COPYING is BSD, LGPLv2+ COPYING is missing.
> 

done.

Spec URL: http://www.stanford.edu/~fenn/packs/apbs.spec
SRPM URL: http://www.stanford.edu/~fenn/packs/apbs-1.1.0-5.fc10.src.rpm

(In reply to comment #30)
> (In reply to comment #26)
> > apbs has support for:
> >   --with-mpich=PATH       toplevel MPICH directory
> >   --with-mpich2=PATH      toplevel MPICH2 directory
> >   --with-lam=PATH         toplevel LAM-MPI directory
> >   --with-openmpi          enable OpenMPI compilation
> > so I suggest you add these to the package in the future.
> > 
> 
> Will do.

Just no hurry - wait until the two drafts become guidelines :)


**

> > - There are tests in examples/, run them in %check with

clip
 
> some of the directories don't contain makefiles, so instead I'm using:

clip

> This works, but the resulting examples take ~4hrs to run on my AMD opteron 275.
> I can cut it down to a 1-2 of the examples...

Ugh. Do they give an error code if the result is not the wanted one..?

You could probably just drop the %check phase.


**

Package does not build:
RPM build errors:
    File not found: /builddir/build/BUILDROOT/apbs-1.1.0-5.fc11.x86_64/usr/bin/ApbsClient.pyc
    File not found: /builddir/build/BUILDROOT/apbs-1.1.0-5.fc11.x86_64/usr/bin/ApbsClient.pyo

This is due to RPM in Fedora 11 not byte compiling files in %{_bindir}. Instead of %exclude you could just delete the files after %install with rm -f which doesn't complain about possible non-existing files.

(In reply to comment #31)
> (In reply to comment #30)
> > (In reply to comment #26)
> > > apbs has support for:
> > >   --with-mpich=PATH       toplevel MPICH directory
> > >   --with-mpich2=PATH      toplevel MPICH2 directory
> > >   --with-lam=PATH         toplevel LAM-MPI directory
> > >   --with-openmpi          enable OpenMPI compilation
> > > so I suggest you add these to the package in the future.
> > > 
> > 
> > Will do.
> 
> Just no hurry - wait until the two drafts become guidelines :)
> 

Right!

> > This works, but the resulting examples take ~4hrs to run on my AMD opteron 275.
> > I can cut it down to a 1-2 of the examples...
> 
> Ugh. Do they give an error code if the result is not the wanted one..?
> 
> You could probably just drop the %check phase.
> 

There is no error code, it just prints out that the result was not within the given error tolerance.

I just took out the %check.

> 
> Package does not build:
> RPM build errors:
>     File not found:
> /builddir/build/BUILDROOT/apbs-1.1.0-5.fc11.x86_64/usr/bin/ApbsClient.pyc
>     File not found:
> /builddir/build/BUILDROOT/apbs-1.1.0-5.fc11.x86_64/usr/bin/ApbsClient.pyo
> 
> This is due to RPM in Fedora 11 not byte compiling files in %{_bindir}. Instead
> of %exclude you could just delete the files after %install with rm -f which
> doesn't complain about possible non-existing files.  

I just removed the %exclude statement and built using F11 (since I won't be packaging this for F10 anyway).

Spec URL: http://www.stanford.edu/~fenn/packs/apbs.spec
SRPM URL: http://www.stanford.edu/~fenn/packs/apbs-1.1.0-6.fc11.src.rpm

(In reply to comment #32)
> > Package does not build:
> > RPM build errors:
> >     File not found:
> > /builddir/build/BUILDROOT/apbs-1.1.0-5.fc11.x86_64/usr/bin/ApbsClient.pyc
> >     File not found:
> > /builddir/build/BUILDROOT/apbs-1.1.0-5.fc11.x86_64/usr/bin/ApbsClient.pyo
> > 
> > This is due to RPM in Fedora 11 not byte compiling files in %{_bindir}. Instead
> > of %exclude you could just delete the files after %install with rm -f which
> > doesn't complain about possible non-existing files.  
> 
> I just removed the %exclude statement and built using F11 (since I won't be
> packaging this for F10 anyway).
> 
> Spec URL: http://www.stanford.edu/~fenn/packs/apbs.spec
> SRPM URL: http://www.stanford.edu/~fenn/packs/apbs-1.1.0-6.fc11.src.rpm  

.. but won't you be building for EPEL, if you need this for pymol?

**

We usually don't add license files ourselves.

**

You're using ATLAS for ARPACK? You might want to use it for blas, too:
 --with-blas="-L%{_libdir}/atlas -lf77blas -latlas"
should do it.

**

For the tools installation I suggest using a loop:

for bin in tools/manip/{psize.py,coulomb,born} tools/mesh/{mgmesh,dxmath,mergedx2,mergedx,value,uhbd_asc2bin,smooth,dx2mol,dx2uhbd,similarity,multivalue,benchmark,analysis} arpack/driver; do
 install -p -m 755 $bin %{buildroot}%{_bindir}/apbs-`basename $bin`
done

Well, I guess this package is good to go.

APPROVED

Before importing, check the stuff in comment #33; I'd add
 rm -rf %{buildroot}%{_bindir}/*.py{c,o}
to the end of install to be able to build on F-10 and EPEL.

(In reply to comment #33)
> (In reply to comment #32)
> > > Package does not build:
> > > RPM build errors:
> > >     File not found:
> > > /builddir/build/BUILDROOT/apbs-1.1.0-5.fc11.x86_64/usr/bin/ApbsClient.pyc
> > >     File not found:
> > > /builddir/build/BUILDROOT/apbs-1.1.0-5.fc11.x86_64/usr/bin/ApbsClient.pyo
> > > 
> > > This is due to RPM in Fedora 11 not byte compiling files in %{_bindir}. Instead
> > > of %exclude you could just delete the files after %install with rm -f which
> > > doesn't complain about possible non-existing files.  
> > 
> > I just removed the %exclude statement and built using F11 (since I won't be
> > packaging this for F10 anyway).
> > 
> > Spec URL: http://www.stanford.edu/~fenn/packs/apbs.spec
> > SRPM URL: http://www.stanford.edu/~fenn/packs/apbs-1.1.0-6.fc11.src.rpm  
> 
> .. but won't you be building for EPEL, if you need this for pymol?
> 

Right... in that case, using rm -f in %install isn't enough - brp-python-bytecompile runs after %install, so it just recreates the .pyc/.pyo files. The other option is to rename the files so they don't have a .py extension, but I'm not sure if that will break other tools. Could I just use a different spec file for the F-10 and EPEL branches?

> 
> We usually don't add license files ourselves.
> 

The package doesn't include a LGPL license file (for some reason, they included the GPL license file with LGPL licensed code) - I'll mention this to upstream.

> 
> You're using ATLAS for ARPACK? You might want to use it for blas, too:
>  --with-blas="-L%{_libdir}/atlas -lf77blas -latlas"
> should do it.
> 

Yes - the tools/arpack/ folder makes calls to ATLAS, even though the configure script doesn't look for it (grrr).  Added the recommended flags.

> 
> For the tools installation I suggest using a loop:
> 
> for bin in tools/manip/{psize.py,coulomb,born}
> tools/mesh/{mgmesh,dxmath,mergedx2,mergedx,value,uhbd_asc2bin,smooth,dx2mol,dx2uhbd,similarity,multivalue,benchmark,analysis}
> arpack/driver; do
>  install -p -m 755 $bin %{buildroot}%{_bindir}/apbs-`basename $bin`
> done  

thanks for all the pointers on this package, its been a challenge!

(In reply to comment #36)
> (In reply to comment #33)
> > (In reply to comment #32)
> > > I just removed the %exclude statement and built using F11 (since I won't be
> > > packaging this for F10 anyway).
> > 
> > .. but won't you be building for EPEL, if you need this for pymol?
> > 
> 
> Right... in that case, using rm -f in %install isn't enough -
> brp-python-bytecompile runs after %install, so it just recreates the .pyc/.pyo
> files. The other option is to rename the files so they don't have a .py
> extension, but I'm not sure if that will break other tools. Could I just use a
> different spec file for the F-10 and EPEL branches?

Duh.

Yes, you can use a different spec file, or even better:

%if 0%{?fedora}<11 || 0%{?rhel} == 4 || 0%{?rhel} == 5
%exclude %{_bindir}/*.pyc
%exclude %{_bindir}/*.pyo
%endif

(In reply to comment #37)
> (In reply to comment #36)
> > (In reply to comment #33)
> > > (In reply to comment #32)
> > > > I just removed the %exclude statement and built using F11 (since I won't be
> > > > packaging this for F10 anyway).
> > > 
> > > .. but won't you be building for EPEL, if you need this for pymol?
> > > 
> > 
> > Right... in that case, using rm -f in %install isn't enough -
> > brp-python-bytecompile runs after %install, so it just recreates the .pyc/.pyo
> > files. The other option is to rename the files so they don't have a .py
> > extension, but I'm not sure if that will break other tools. Could I just use a
> > different spec file for the F-10 and EPEL branches?
> 
> Duh.
> 
> Yes, you can use a different spec file, or even better:
> 
> %if 0%{?fedora}<11 || 0%{?rhel} == 4 || 0%{?rhel} == 5
> %exclude %{_bindir}/*.pyc
> %exclude %{_bindir}/*.pyo
> %endif  

will do.  I'll hold off on requesting CVS access until I hear something regarding maloc version numbering.

New Package CVS Request
=======================
Package Name: apbs
Short Description: adaptive poisson boltzmann solver
Owners: timfenn
Branches: F-11 EL-5
InitialCC: timfenn

CVS Done
No need to put an owner on the initallCC list

builds for F-11 and devel done, but EL-5 fails due to arpack not being available.

Package Change Request
======================
Package Name: apbs
New Branches: epel7
Owners: timfenn
InitialCC: timfenn

Git done (by process-git-requests).