Bug 542765

http://www.five-ten-sg.com/libghemical.spec
http://www.five-ten-sg.com/libghemical-2.99.1-10.fc12.src.rpm

http://www.five-ten-sg.com/libghemical.spec
http://www.five-ten-sg.com/libghemical-2.99.1-11.fc12.src.rpm   
http://koji.fedoraproject.org/koji/taskinfo?taskID=1911782

Since all dependent packages are now available (it looks like that BZ #542759 can be closed now) I'll review this package.

Here are two questions:

Naming of the packages: since the basename of this pacakge is already libghemical I think it would be better to omit the sub-package "libghemical-libs" and put the library directly in the base package.

Data package: What is the purpose of the data files in the -data sub-package? If the library won't work without it, I would rather omit this sub-package as well and put everything into the base package.

Sorry about the delay - I now have some time to work on these packages.

naming - I thought there was some fedora requirement that .so objects be in -lib subpackages.

data - as a separate package, that is arch-independent, which we lose if placed in the main package.

(In reply to comment #4)
> Sorry about the delay - I now have some time to work on these packages.
> 
> naming - I thought there was some fedora requirement that .so objects be in
> -lib subpackages.
> 
> data - as a separate package, that is arch-independent, which we lose if placed
> in the main package.    

No, this is not necessary and not intended:

- if the package contains only a library, then the *.so.* files go into the base package which name matches the upstream tarball

- exception: if the package contains a library and e.g. a binary which uses the library and if this library may be also used by other packages then it is justified to create a -libs package (see e.g. tcp_wrappers, tcp_wrappers-libs)

- if the arch-independent data is needed for the base package, then it should not be separated (and for sure the COPYING file should never be separated from the binaries)

- in general: separate files into sub-packages only in case it is necessary, reasons could be:
* the situation with the libraries as described above
* very large documentation
* additional data which is not really needed for the base package (e.g. additional levels for a game)
* -devel

The main reasons is to save same disk space for people who don't need the additional files.

So in case of this package, all files should go into the base package (no -libs, no -data) besides the devel files which are already correct in the -devel package.

fixed.

http://www.five-ten-sg.com/libghemical.spec
http://www.five-ten-sg.com/libghemical-2.99.1-12.fc12.src.rpm   
http://koji.fedoraproject.org/koji/taskinfo?taskID=2093385

Thanks for the new package, here is now the full review:

* rpmlint: OK
rpmlint RPMS/i686/libghemical-* SRPMS/libghemical-2.99.1-12.fc13.src.rpm SPECS/libghemical.spec
libghemical.i686: W: shared-lib-calls-exit /usr/lib/libghemical.so.5.0.0 exit
libghemical-devel.i686: W: no-documentation
4 packages and 1 specfiles checked; 0 errors, 2 warnings.

According to rpmlint's help the usage of exit in libraries is discouraged since the calling application can not handle the error. However, in this case it seems to be a design decisions of the upstream evelopers. This will not block the review. But depending on your involvement with upstream it would probably be worth to ask them for the reasons and/or explain them why its discouraged.

The no-documentation for the -devel package is also a false positive, it seems that there is no API documentation available.

* naming: OK
- name matches upstream
- spec file name matches package name

* sources: OK
- md5sum: d2dae2d7d786d3cba335cb29d85033ea  libghemical-2.99.1.tar.gz
- sources matches upstream
- Source0 tag ok
- spectool -g  works

* License: TODO
- License in spec file matches the actual license (of the sources)
- License GPLv2+ acceptable
- However, for most of the data files the license is not 100% clear. A few of the data files use the following license:
"The files in this directory were downloaded from:
http://www.amber.ucsf.edu/amber/dbase.html
At the download page there was the following copyright notice:
'As has always been the case, the parameter information in the above file is
in the public domain, and may be redistributed or used in other programs. Any
such use should include proper citations, and any changes in the parameters
should be prominently noted.'"

- Please can you ask upstream for a statement about the licenses of all of the data files?

* spec file written in English and legible: TODO
Please can you enhance the description of the main package a little bit to include some information what is the purpose of "libghemical"?

* compilation: TODO
- supports parallel build
- RPM_OPT_FLAGS are correctly used
- builds in koji: F14
- However, in the %build section the LIBS variable generated by %configure is fully overwritten by the custom LIBS make parameter. 

If you call "rpmlint libghemical" on a system having the library installed, there will be some warnings:

----
libghemical.i686: W: undefined-non-weak-symbol /usr/lib/libghemical.so.5.0.0 dlopen
libghemical.i686: W: undefined-non-weak-symbol /usr/lib/libghemical.so.5.0.0 lm7_set_plots_orbital_index
libghemical.i686: W: undefined-non-weak-symbol /usr/lib/libghemical.so.5.0.0 getorb_
...
----
The warnings show that the library uses symbols but does not link the appropriate libraris.

Additionally there are warnings like:
----
libghemical.i686: W: unused-direct-shlib-dependency /usr/lib/libghemical.so.5.0.0 /usr/lib/libSCmbptr12.so.7
libghemical.i686: W: unused-direct-shlib-dependency /usr/lib/libghemical.so.5.0.0 /usr/lib/libSCoint3.so.7
...
----
These warnings show that a couple of the manually added libraries are not needed on the other hand (this is not that critical as the previous warnings).

Sure, I don't know upstream's intention, but IMHO it should be rather done like this:

1. don't overwrite the LIBS in the spec file
2. ensure, that libghemical is linked against all libraries from which libghemical uses any symbols
- this should be changed probably in the Makefile.am's of the package
- finally there should be no undefined non-weak symbols

Some more information can be found here: https://fedoraproject.org/wiki/Features/ChangeInImplicitDSOLinking

Additionally the Libs variable in the pkg-config file looks rather strange. In general Libs should only contain the library itself. 


* BuildRequires: TODO
It looks like that the following BuildRequirements are not needed:
BuildRequires:  gcc-gfortran
BuildRequires:  openbabel-devel >= 2.2

* locales handling: OK

* ldconfig in %post and %postun: OK

* package owns all directories that it creates: OK

* %files section: TODO (minor)
Please add a "/" at the line
%{_datadir}/%{name}
in the %files section to explicit state that this is a directory.

* no files listed twice in %files: OK

* file permissions: OK
- %defattr used
- actual permissions in packages OK

* %clean section: OK

* macro usage: TODO (minor)
please use %{_includedir}/%{name} for consistency

* code vs. content: TODO
This package contains content (the various data files). Let's wait for upstream's feedback regarding the Licenses and ask then Fedora Legal...

* main package should not contain development related parts: OK

* large documentation into subpackage: OK (n/a)

* header files in -devel subpackage: OK

* static libraries in -static package: OK (n/a)

* *.so link in -devel package: OK

* - devel package requires base package using fully versioned dependency: OK

* packages must not contain *.la files: OK

* GUI applications must provide *.desktop file: OK (n/a)

* packages must not own files/dirs already owned by other packages: OK

* rm -rf $RPM_BUILD_ROOT at the beginning of %install: OK

* all filenames UTF-8: OK

* functional test: OK
- ghemical can be compiled and linked against libghemical and works without any problems

* debuginfo sub-package: OK
- non-empty

exit in library - yes, that seems to be the way they wrote the code, and I don't see that changing since there are a LOT of such calls.

data files license - the amber and tripos data files are public domain, and used in a many other chemistry packages. I will check with upstream, but I doubt there is a more explicit license.

I added a bit of description for the libghemical package. 

Removed unnecessary build-requires.

rpmlint undefined-non-weak-symbol vs. unused-direct-shlib-dependency : for an example, if we don't add -ldl, then we get dlopen as an undefined-non-weak-symbol. If we do add -ldl, we get unused-direct-shlib-dependency for dlopen.so. I have opted to prevent the undefined-non-weak-symbol errors.

The upstream autoconf packaging does not seem to pickup the proper mpqc libraries, unless we override 'make LIBS=', which overrides everything. Unless we want to redo the whole autoconf package, overriding LIBS= seems to work.

http://www.five-ten-sg.com/libghemical.spec
http://www.five-ten-sg.com/libghemical-2.99.1-13.fc12.src.rpm   
http://koji.fedoraproject.org/koji/taskinfo?taskID=2096296

data files license issue: upstream says:

I have myself typed the contents of the Tripos files from
a printed copy of an article printed in a scientific journal.
Therefore I assume the tripos data files and the rest of
libghemical are under the same license. The parameter
files other than Amber and Tripos are written my me
as well.

with Best Regards,

Tommi Hassinen

(In reply to comment #8)
> The upstream autoconf packaging does not seem to pickup the proper mpqc
> libraries, unless we override 'make LIBS=', which overrides everything. Unless
> we want to redo the whole autoconf package, overriding LIBS= seems to work.
> 

Well, what you are doing is a workaround, not a fix. The proper fix is to modify configure.ac (and his friends) accordingly, and rerun autoreconf. This shouldn't be too hard. Just add the missing libraries using a similar scheme to the exising ones. At the end of the day, you can submit the fix upstream, so everyone, and not just Fedora users, can benefit from it.

fixed by patching configure.ac

http://www.five-ten-sg.com/libghemical.spec
http://www.five-ten-sg.com/libghemical-2.99.1-14.fc12.src.rpm   
http://koji.fedoraproject.org/koji/taskinfo?taskID=2103279

buildrequires gcc-gfortran, since it needs libgfortranbegin in the link.

Sorry for the late response.

(In reply to comment #11)
> http://www.five-ten-sg.com/libghemical.spec
> http://www.five-ten-sg.com/libghemical-2.99.1-14.fc12.src.rpm   
> http://koji.fedoraproject.org/koji/taskinfo?taskID=2103279
> 
> buildrequires gcc-gfortran, since it needs libgfortranbegin in the link.    

Thanks for the new package and the bug fixes, here is the update of my review:

* spec file written in English and legible: OK
* BuildRequires: OK
* files section: OK

* compilation: TODO
Thanks for fixing the issue, the library looks now good. 
Probably it would be better to split the patch into two files: one for using atlas and the 2nd one for fixing the compile issue.

There is one minor remaining issue: the pkg-config file still contains lots of uneeded libraries and also references to the current compiler directory which prevent compiling anything against libchemical just by using this pkg-config file:

echo "void main() {}" > a.c && gcc `pkg-config --libs libghemical` -o a a.c

/tmp/ccZEt1Bd.o: In function `main':
a.c:(.text+0x0): multiple definition of `main'
/usr/lib/gcc/i686-redhat-linux/4.4.3/../../../libfl.a(libmain.o):(.text+0x0): first defined here
/usr/lib/gcc/i686-redhat-linux/4.4.3/../../../libfl.a(libmain.o): In function `main':
(.text+0x19): undefined reference to `yylex'
collect2: ld returned 1 exit status

If the Libs variable in libghemical.pc would only contain "-L${libdir} -lghemical" everything would work fine.

* License & code vs. content: TODO
Thanks for asking upstream. I feel that there is no problem, but I don't think I have the knowledge for the final decision. Let's get the final OK from FE-LEGAL:

@spot: This package contains a number of data files which are needed for chemical calculations of chemical bindings, etc.

On set of the files has the following license:
" The files in this directory were downloaded from:
        http://www.amber.ucsf.edu/amber/dbase.html
At the download page there was the following copyright notice:

As has always been the case, the parameter information in the above file is
in the public domain, and may be redistributed or used in other programs. Any
such use should include proper citations, and any changes in the parameters
should be prominently noted."

and for the other one upstream gave the statement in comment #9. Based on these statements and that other distributions (like Debian) have no issues, I assume there is no problem.

The licensing on that data (Public Domain) looks acceptable to me. Lifting FE-Legal.

(In reply to comment #13)
> The licensing on that data (Public Domain) looks acceptable to me. Lifting
> FE-Legal.    

Thanks, spot!

So the only remaining issue is the change in the pkgconfig file - once this is done, this review can be approved. ;-)

pkgconfig file patched.

http://www.five-ten-sg.com/libghemical.spec
http://www.five-ten-sg.com/libghemical-2.99.1-15.fc12.src.rpm   
http://koji.fedoraproject.org/koji/taskinfo?taskID=2128853

(In reply to comment #15)
> http://www.five-ten-sg.com/libghemical.spec
> http://www.five-ten-sg.com/libghemical-2.99.1-15.fc12.src.rpm   
> http://koji.fedoraproject.org/koji/taskinfo?taskID=2128853    

Please can you check whether the files were correctly uploaded? The spec file is  at release 14 and the src.rpm redirects to -14.fc12.src.rpm, too.

Oops, forgot to update the production web server from the staging server.

http://www.five-ten-sg.com/libghemical.spec
http://www.five-ten-sg.com/libghemical-2.99.1-15.fc12.src.rpm   
http://koji.fedoraproject.org/koji/taskinfo?taskID=2128853   

should be good now.

Thanks for the new package. All mentioned issues were fixed.

-> APPROVED

New Package CVS Request
=======================
Package Name: libghemical
Short Description: Libraries for the Ghemical chemistry package
Owners: carllibpst
Branches: F-12 F-13
InitialCC:

CVS done (by process-cvs-requests.py).

Just a small comment about the arch-independent data. If it's large (I haven't checked) you could split it into a .noarch subpackage called -data.

708K installed, but it gzips down to 117K so that is presumably the size in the .rpm file. I don't know what the normal limits are on noarch data before we really need to make it a noarch data subpackage. I think that is just repository size that it saves.