Bug 467235 - Review Request: globus-callout - Globus Toolkit - Globus Callout Library
Summary: Review Request: globus-callout - Globus Toolkit - Globus Callout Library
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Orcan Ogetbil
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: gpt 453848 453849 453851
Blocks: 467239
TreeView+ depends on / blocked
 
Reported: 2008-10-16 14:31 UTC by Mattias Ellert
Modified: 2009-05-12 03:55 UTC (History)
3 users (show)

Fixed In Version: 0.7-3.fc11
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-04-27 21:20:24 UTC
Type: ---
Embargoed:
oget.fedora: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Mattias Ellert 2008-10-16 14:31:51 UTC
Spec URL: http://www.grid.tsl.uu.se/repos/globus/fedora/9/info/globus-callout.spec
SRPM URL: http://www.grid.tsl.uu.se/repos/globus/fedora/9/src/SRPMS/globus-callout-0.7-0.2.fc9.src.rpm
Description:
The Globus Toolkit is an open source software toolkit used for
building Grid systems and applications. It is being developed by the
Globus Alliance and many others all over the world. A growing number
of projects and companies are using the Globus Toolkit to unlock the
potential of grids for their cause.

The globus-callout package contains:
Globus Callout Library

BuildRequires:	gpt
BuildRequires:	globus-libtool-devel >= 1
BuildRequires:	globus-common-devel >= 3
BuildRequires:	globus-core-devel >= 4

Based on the globus toolkit 4.2.1 release.

Comment 1 Mattias Ellert 2009-01-06 00:49:55 UTC
This package was updated due to the renaming of the GPT package.

SRPM: http://www.grid.tsl.uu.se/repos/globus/fedora/10/src/SRPMS/globus-callout-0.7-0.3.fc10.src.rpm

SPEC: http://www.grid.tsl.uu.se/repos/globus/info/globus-callout.spec

Comment 2 Mattias Ellert 2009-03-16 00:00:04 UTC
New version
- Added s390x as 64 bit arch
- Added comment documenting source
- Adapt to changes in the globus-core package

SRPM:
http://www.grid.tsl.uu.se/repos/globus/fedora/10/src/SRPMS/globus-callout-0.7-0.5.fc10.src.rpm
http://www.grid.tsl.uu.se/repos/globus/info/globus-callout.spec

Comment 3 Orcan Ogetbil 2009-04-08 04:47:41 UTC
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.

- rpmlint "globus-callout-devel.x86_64: W: no-documentation" can be ignored.

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

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

* 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.

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

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

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

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

! Please make the descriptions span 80 columns

? Why are you packaging the .filelist files?

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

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

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

! 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

* 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.

! 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.

Comment 4 Mattias Ellert 2009-04-17 13:44:28 UTC
(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

Comment 5 Orcan Ogetbil 2009-04-17 18:32:19 UTC
(In reply to comment #4)
> (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.)
> 

This looks good to me except the inclusion of the external license file. Thanks.

> > ? 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.

Sorry, I just confused myself :)

> 
> > * 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:
> 

No, I meant during the linking phase: (from build.log)
   libtool-gcc64pthr: link: /usr/bin/gcc -shared  .libs/globus_callout.o .libs/globus_callout_error.o  -L/usr/lib64 -lglobus_common -ldl -lltdl -lpthread  -m64 -mtune=generic -m64 -m64   -Wl,-soname -Wl,libglobus_callout.so.0 -o .libs/libglobus_callout.so.0.0.7

But apparently, there is no general consensus among the packagers about this. We do need to discuss this in the mailing list. For now, you don't need to worry about it.

> 
> 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  

Good resource. I will go through it this week and make comments in case.


I think this one is good to go now.

-------------------------------------------------
This package (globus-callout) is APPROVED by oget
-------------------------------------------------

Comment 6 Mattias Ellert 2009-04-17 20:23:18 UTC
Thank you for the review.


New Package CVS Request
=======================
Package Name: globus-callout
Short Description: Globus Toolkit - Globus Callout Library
Owners: ellert
Branches: F-9 F-10 F-11 EL-4 EL-5
InitialCC:

Comment 7 Kevin Fenzi 2009-04-17 21:26:53 UTC
cvs done.

Comment 8 Fedora Update System 2009-04-22 16:59:07 UTC
globus-callout-0.7-2.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/globus-callout-0.7-2.fc10

Comment 9 Fedora Update System 2009-04-22 16:59:14 UTC
globus-callout-0.7-2.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/globus-callout-0.7-2.fc9

Comment 10 Fedora Update System 2009-04-22 16:59:19 UTC
globus-callout-0.7-2.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/globus-callout-0.7-2.fc11

Comment 11 Fedora Update System 2009-04-27 21:20:17 UTC
globus-callout-0.7-2.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 12 Fedora Update System 2009-04-27 21:21:14 UTC
globus-callout-0.7-2.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 13 Fedora Update System 2009-05-12 03:55:38 UTC
globus-callout-0.7-3.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.


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