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@xxxxxxxxxxxx> 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. _______________________________________________ Fedora-package-review mailing list Fedora-package-review@xxxxxxxxxx http://www.redhat.com/mailman/listinfo/fedora-package-review