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.
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
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
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.
(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
(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 -------------------------------------------------
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:
cvs done.
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
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
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
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.
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.
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.