[Bug 467235] Review Request: globus-callout - Globus Toolkit - Globus Callout Library

Fri Apr 17 13:44:31 UTC 2009

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

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

--- Comment #4 from Mattias Ellert <mattias.ellert at fysast.uu.se>  2009-04-17 09:44:28 EDT ---
(In reply to comment #3)
> Here are my comments for this package. Please note that the bug that I pointed
> in bug 453851#c19 needs fixed in order to build this package in koji.

The file in GPT is needed. The file in globus-common is also needed by a few
Globus packages, but not by any of the packages I have packaged for Fedora so
far - and it looks like it might be possible to work around it. I have prepared
an update of globus-common where the file is dropped, I just haven't imported
it into CVS yet. If you want to use it in a chain build for reviewing purposes
it is available here:

http://www.grid.tsl.uu.se/repos/globus/info/new/globus-common-10.2-2.fc10.src.rpm

(Since this update also backports the fixes based on your comments of this
package to globus-common I would like your feedback on this package before
importing it to CVS, since these could possible influence this update as well.)

> ? Where does the version number come from? I don't see it in the sources. And
> why is the release number 0.x ?

The version number comes from the GPT source package description file
pkgdata/pkg_data_src.gpt.in

The release was 0.x because I wanted to have a clear upgrade path from my third
party repository containing the packages and the official Fedora version once
the packages are accepted. My plan was to change the release to 1 when
importing it to CVS. My new package that I list below now gives the release
number as 1 already, but I didn't import it to my third party repo.

> ! Please move the explanation of Source8 to where you define Source8.

Fixed

> * The file, containing the text of the license(s) for the package must be
> included in %doc if (and only if) the source package includes the text of the
> license(s) in its own file. The original source tree does not contain the
> license file in the callout/source/ folder, so Source9 should be left out.

Fixed

> ? Should the license be ASL 2.0? The source files say they are ASL 2.0 on their
> headers.

The licence is Apache-2.0 without any changes, and the specfile already states
ASL 2.0 accordingly.

> * Description needs to be descriptive. What is a callout library?

This descriptions has been extracted from the upstream GPT source package
description file pkgdata/pkg_data_src.gpt.in. Upstream obviously thought this
description was sufficient. For most globus packages the description in the GPT
source package description is a bit more understandable, but for this package
it is a bit cryptic. I have added a sentence from the Doxygen documentation to
the package description.

> * The new guidelines suggest that %global should be preferred over %define

Fixed

> ! Could you collect all your "%global"s at one place?

The globals are now first in the file (which seems to be the custom nowadays).
Except for the global that defines %_name which must come after the Name tag,
since it uses %name in its definition and this is not defined before the Name
tag is parsed. Defining %_name right after the Name tag is also the most
logical place for it.

> ! Please make the descriptions span 80 columns

Fixed

> ? Why are you packaging the .filelist files?

These are part of the package GPT metadata. They are needed by GPT.

> * Fedora specific compiler flags are not honored in the linking phase. At the
> least, "-g -Wall" needs passed.

They are. The package uses the %configure macro which expands to:

  CFLAGS="${CFLAGS:-%optflags}" ; export CFLAGS ; 
  CXXFLAGS="${CXXFLAGS:-%optflags}" ; export CXXFLAGS ; 
  FFLAGS="${FFLAGS:-%optflags}" ; export FFLAGS ; 
  for i in $(find . -name config.guess -o -name config.sub) ; do 
           [ -f /usr/lib/rpm/redhat/$(basename $i) ] && %{__rm} -f $i &&
%{__cp} -fv /usr/lib/rpm/redhat/$(basename $i) $i ; 
  done ; 
  ./configure --build=%{_build} --host=%{_host} \
 --target=%{_target_platform} \
 --program-prefix=%{?_program_prefix} \
 --prefix=%{_prefix} \
 --exec-prefix=%{_exec_prefix} \
 --bindir=%{_bindir} \
 --sbindir=%{_sbindir} \
 --sysconfdir=%{_sysconfdir} \
 --datadir=%{_datadir} \
 --includedir=%{_includedir} \
 --libdir=%{_libdir} \
 --libexecdir=%{_libexecdir} \
 --localstatedir=%{_localstatedir} \
 --sharedstatedir=%{_sharedstatedir} \
 --mandir=%{_mandir} \
 --infodir=%{_infodir}

The build log says:

 /usr/bin/gcc -DPACKAGE_NAME=\"\" -DPACKAGE_TARNAME=\"\" -DPACKAGE_VERSION=\"\"
-DPACKAGE_STRING=\"\" -DPACKAGE_BUGREPORT=\"\" -DPACKAGE=\"globus_callout\"
-DVERSION=\"0.7\" -I. -I.. -I../library/oldgaa -I/usr/include/globus
-I/usr/lib/globus/include -O -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2
-fexceptions -fstack-protector --param=ssp-buffer-size=4 -m32 -march=i386
-mtune=generic -fasynchronous-unwind-tables -D_REENTRANT -Wall -c
globus_callout_error.c  -fPIC -DPIC -o .libs/globus_callout_error.o

All the Fedora compiler options are there.

> * The doc package is fairly small. Why don't you put this documentation in the
> devel subpackage?

The globus library APIs are documented using doxygen comments. These are used
to generate the documentation in the GPT doc packages. For some globus packages
these packages are very large, while for others they are quite small. In order
to provide consistent and user predictable packaging for all globus packages,
the GPT doc packages should be packaged as separate doc RPMs for all globus
packages regardless of their size.

> ! Please replace /usr/share with %{_datadir}

Fixed

> ! Please explain the non-trivial things you do in the SPEC file with comments.
> Why do you remove those files in %build? Why are those sed's for? etc

Fixed

> * On the main package:
>     Requires:       globus-libtool >= 1
>     BuildRequires:  globus-libtool-devel >= 1
>     BuildRequires:  globus-core >= 4 ,
>   on the devel subpackage:
>     Requires:       globus-libtool-devel >= 1
>     Requires:       globus-core >= 4
>     Requires:       pkgconfig
>   are redundant. They will be picked up by other dependencies.

The Requires and BuildRequires are autogenerated from the information in the
GPT source package description file pkgdata/pkg_data_src.gpt.in, to manually
delete some of them is not maintainable - and such changes are very likely to
be dropped at the next upstream release since then the list will be regenerated
from the new GPT source package description file.

I did remove the Requires on pkgconfig - that one is not listed in the GPT
file.

> ! If what you want to do is to erase lines, you can replace
>    for l in $RPM_BUILD_ROOT%{_datadir}/globus/packages/%{_name}/*.filelist ; do
>       grep -v 'lib.*\.la$' < $l > $l.new
>       mv $l.new $l
>    done
> with 
>    sed -i '/lib.*\.la$/d' \
>        $RPM_BUILD_ROOT%{_datadir}/globus/packages/%{_name}/*.filelist
> to make the code simpler. Similarly for the .a files.
> 
> Also, the multiple instances of
>    cat some_file |sed s!some!expression! > new_file
>    mv new_file some_file
> can be replaced by
>    sed -i -e s!some!expression! \
>           -e s!other!expression! \
>           -e s!yetanother!expression! \
>    some_file
> 
> These changes would shorten your SPEC file quite a bit. Again, explaining these
> things in the SPEC file as comments would be a great bonus for reviewers and
> other packagers who check your package.  

I was always hesitant to use "sed -i" for compatibility reasons. But doing some
tests shows that you have to go way back to RHL7.3 to get to a sed that doesn't
understand the -i flag. I have taken your advice.

New version is available here:

http://www.grid.tsl.uu.se/repos/globus/info/new/globus-callout-0.7-1.fc10.src.rpm
http://www.grid.tsl.uu.se/repos/globus/info/new/globus-callout.spec

PS. I have collected the answers to the review comments by you and others in a
Draft packaging guidelines document for Globus packages. This is available
here:

http://fedoraproject.org/wiki/PackagingDrafts/Globus

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.