Spec URL: http://www.grid.tsl.uu.se/repos/globus/fedora/9/info/gpt.spec SRPM URL: http://www.grid.tsl.uu.se/repos/globus/fedora/9/src/SRPMS/gpt-3.2-8.fc9.src.rpm Description : GPT is a collection of packaging tools built around an XML based packaging data format. This format provides a straight forward way to define complex dependency and compatibility relationships between packages. The tools provide a means for developers to easily define the packaging data and include it as part of their source code distribution. Binary packages can be automatically generated from this data. The packages defined by GPT are compatible with other packages and can be easily converted. This is a build requirement for the Globus Toolkit
I'll have a look at all this globus stuff (as time permits...). It is a possible root dependency which has been submitted recently.
It seems strange to me to need gpt for globus in fedora, on pure rpm system it should not be useful. That being said, it doesn't mean that gpt cannot be in fedora. I think that the name is too short, it would be better to have something longer. You have a fair amount of patches. Is upstream aware of these? Why do you change _ in - in script file names? Reading the /usr/share/gpt/lib/perl/Grid/GPT/LocalEnv.pm file it seems that gtar/gzip is used, so a Requires should be needed, in stead of the perl(Archive::Tar) Requires. Also rpm and rpmbuild seems to be used so maybe some Requires are missing. Why aren't the dtds in some dtd directory? There are some automake duplicated files, this deserves a comment. Also gpt-bootstrap.sh seems to require all the autotools and it is not very clear where it is documented. the globus_core-src.tar.gz file seems also dubious to me. Do you really need to rerun the autotools? It doesn't seems clear to me based on the patches. Instead of populating %{docdir} yourseld, I wuold suggest using an in-source directory and %doc. This allows to better keep timestamps without much work. Like: rm -rf __dist_docs mkdir __dist_docs mv $RPM_BUILD_ROOT%{_sysconfdir}/%{name}/dtd __dist_docs/ mv $RPM_BUILD_ROOT%{_datadir}/%{name}/gpt_rpm.spec __dist_docs/ iconv -f iso-8859-1 -t utf-8 LICENSE > LICENSE.utf8 touch -c -r LICENSE LICENSE.utf8 mv LICENSE.utf8 LICENSE and in %files: %doc __dist_docs/* CHANGES INSTALL README For patch, 2 suggestions, use %patch0 -p1 -b .globus to be able to use gendiff more easily, and name patch like Patch0: %{name}-3.2-globus.patch to know in which version the patch was introduced.
(In reply to comment #2) Thank you for taking the time to review this. > It seems strange to me to need gpt for globus in fedora, on pure > rpm system it should not be useful. That being said, it doesn't mean > that gpt cannot be in fedora. You are right, gpt is quite useless for most things. Except for building globus. The globus build (makefiles and configure files) makes heavy use of gpt macros and gpt package dependency description files. Building globus without gpt would be possible, but would require a large amount of work to rewrite the build instructions. > I think that the name is too short, it would be better to have something > longer. I have made a new package where I use the full name, grid-packaging-tools: http://www.grid.tsl.uu.se/repos/globus/fedora/9/src/SRPMS/grid-packaging-tools-3.2-9.fc9.src.rpm http://www.grid.tsl.uu.se/repos/globus/fedora/9/info/grid-packaging-tools.spec It is still OK to use /usr/share/gpt as the install location, right? Or must I change that to /usr/share/grid-packaging-tools? > You have a fair amount of patches. Is upstream aware of these? The patch with the fixes from globus I consider to be upstream. The patches that adapt to the HFS file system layout used in Fedora are not really bug fixes but packaging adaptations, so not really appropriate to submit to upstream. The remaining patches are mainly one and two liners, that probably should be committed upstream. NCSA:s bugzilla is quite closed - you need to have an account to submit, and you need to apply for an account via e-mail - no online registration. I will submit the relevant patches upstream when I get a reply on my bugzilla account request (provided it is accepted). > Why do you change _ in - in script file names? The upstream code is inconsistent in its naming. It has 20 script names that have - and 8 that uses _, for no apparent good reason. Making the naming of the scripts consistent is more userfriendly. > Reading the /usr/share/gpt/lib/perl/Grid/GPT/LocalEnv.pm > file it seems that gtar/gzip is used, so a Requires should be needed, > in stead of the perl(Archive::Tar) Requires. Also rpm and rpmbuild > seems to be used so maybe some Requires are missing. I have added Requires for tar and gzip. (I did not do that originally since those were listed as exceptions that were not needed to be listed as build requirements. But thinking about it that is not quite the same thing.) The perl(Archive::Tar) requirement is automatically picked up by rpm, and not mentioned in the spec file. And I think it should be there. I did not add any requires for rpm and rpmbuild since their use inside gpt is a rather exotic use of gpt. Using gpt to produce rpms that way will create bad rpms, with no proper sources. > Why aren't the dtds in some dtd directory? In the new package I have moved them to /usr/share/gpt/dtd which seems consistent with how other Fedora packages install dtd files. > There are some automake duplicated files, this deserves a comment. The files in /usr/share/gpt/amdir are not simple copies of the automake versions. They are extended gpt versions with additional build instructions in them. > Also gpt-bootstrap.sh seems to require all the autotools and it is not > very clear where it is documented. Yes bootstrapping requires the autotools, but that is normally the case, so I can't see that it requires any special documentation. Please clarify what you meant by this comment, since I don't get what you are suggesting. > the globus_core-src.tar.gz file seems also dubious to me. Removed in the new package. > Do you really need to rerun the autotools? It doesn't seems clear > to me based on the patches. Patches change configure.in and Makefile.am, so yes. > Instead of populating %{docdir} yourseld, I wuold suggest using an > in-source directory and %doc. This allows to better keep timestamps > without much work. Like: > > rm -rf __dist_docs > mkdir __dist_docs > mv $RPM_BUILD_ROOT%{_sysconfdir}/%{name}/dtd __dist_docs/ > mv $RPM_BUILD_ROOT%{_datadir}/%{name}/gpt_rpm.spec __dist_docs/ > iconv -f iso-8859-1 -t utf-8 LICENSE > LICENSE.utf8 > touch -c -r LICENSE LICENSE.utf8 > mv LICENSE.utf8 LICENSE Done. (It was even simpler since I moved the dtds to its own directory). > and in %files: > %doc __dist_docs/* CHANGES INSTALL README > > For patch, 2 suggestions, use > %patch0 -p1 -b .globus > to be able to use gendiff more easily, and name patch like > Patch0: %{name}-3.2-globus.patch > to know in which version the patch was introduced. I have added -b options to the patch macros.
(In reply to comment #3) > I will submit the relevant patches upstream when I get a reply on > my bugzilla account request (provided it is accepted). The relevant patches have now been submitted to upstream bugzilla: grid-packaging-tools-globus-package-dtd.patch: http://bugzilla.ncsa.uiuc.edu/show_bug.cgi?id=377 grid-packaging-tools-bootstrap-shebang.patch: http://bugzilla.ncsa.uiuc.edu/show_bug.cgi?id=378 grid-packaging-tools-flavored-headers.patch: http://bugzilla.ncsa.uiuc.edu/show_bug.cgi?id=379 grid-packaging-tools-colocate-bugfix.patch: http://bugzilla.ncsa.uiuc.edu/show_bug.cgi?id=380 grid-packaging-tools-age-version.patch: http://bugzilla.ncsa.uiuc.edu/show_bug.cgi?id=381 grid-packaging-tools-xml.patch: http://bugzilla.ncsa.uiuc.edu/show_bug.cgi?id=382
(In reply to comment #3) > You are right, gpt is quite useless for most things. Except for building globus. > The globus build (makefiles and configure files) makes heavy use of gpt macros > and gpt package dependency description files. Building globus without gpt would > be possible, but would require a large amount of work to rewrite the build > instructions. Ok. > http://www.grid.tsl.uu.se/repos/globus/fedora/9/src/SRPMS/grid-packaging-tools-3.2-9.fc9.src.rpm Looks good. > http://www.grid.tsl.uu.se/repos/globus/fedora/9/info/grid-packaging-tools.spec > > It is still OK to use /usr/share/gpt as the install location, right? Or must I > change that to /usr/share/grid-packaging-tools? You'll need to change it only if another package really needs to use that name. > The patches that > adapt to the HFS file system layout used in Fedora are not really bug fixes but > packaging adaptations, so not really appropriate to submit to upstream. (It is FHS, not HFS). This patch too should be made acceptable by upstream. It uses configure, so the standard paths could be passed to the application. They should be used unless the environment variable is set, in which case the environment variable should be used. > > Why do you change _ in - in script file names? > > The upstream code is inconsistent in its naming. It has 20 script names that > have - and 8 that uses _, for no apparent good reason. Making the naming of the > scripts consistent is more userfriendly. Maybe, but this is something that should be changed upstream and not in fedora. > > Reading the /usr/share/gpt/lib/perl/Grid/GPT/LocalEnv.pm > > file it seems that gtar/gzip is used, so a Requires should be needed, > > in stead of the perl(Archive::Tar) Requires. Also rpm and rpmbuild > > seems to be used so maybe some Requires are missing. > > I have added Requires for tar and gzip. (I did not do that originally since > those were listed as exceptions that were not needed to be listed as build > requirements. But thinking about it that is not quite the same thing.) The > perl(Archive::Tar) requirement is automatically picked up by rpm, and not > mentioned in the spec file. And I think it should be there. I don't think that perl(Archive::Tar) is used when gtar/gzip is used, so it shouldn't be required. > I did not add any requires for rpm and rpmbuild since their use inside gpt is a > rather exotic use of gpt. Using gpt to produce rpms that way will create bad > rpms, with no proper sources. I don't think it is an issue. If it is what gpt user are used to. > > Why aren't the dtds in some dtd directory? > > In the new package I have moved them to /usr/share/gpt/dtd which seems > consistent with how other Fedora packages install dtd files. Ok. > > There are some automake duplicated files, this deserves a comment. > > The files in /usr/share/gpt/amdir are not simple copies of the automake > versions. They are extended gpt versions with additional build instructions in them. Ok. > > Also gpt-bootstrap.sh seems to require all the autotools and it is not > > very clear where it is documented. > > Yes bootstrapping requires the autotools, but that is normally the case, so I > can't see that it requires any special documentation. Please clarify what you > meant by this comment, since I don't get what you are suggesting. If gpt-bootstrap.sh is meant to be used by gpt users, Requires in gpt are needed for the autotools. > > the globus_core-src.tar.gz file seems also dubious to me. > > Removed in the new package. > > > Do you really need to rerun the autotools? It doesn't seems clear > > to me based on the patches. > > Patches change configure.in and Makefile.am, so yes. Ah, I missed them. Looking at them they seem simple enough that patching configure and Makefile.in additionally would allow not to rerun the autotools.
In the next iterations, it could be nice to have in the spec file the the links to the bugzilla you put in Comment #4 in comment near the patch. I have 2 additional comments regarding the changelog section * you should remove %{?dist} from the changelog * I personally prefer when there is a blank line between 2 changelog entries, but this is a amatter of preference. I also like to add a trailing / to directories in %files to mark visually that they are directories, once again it is a personal preference you may ignore. It would lead to this entry in %files: %{_datadir}/gpt/
In fact the renaming of the package cannot be done only in fedora, the naming guidelines mandate that the package is called gpt in that case. So upstream should be convinced to change the package name first. If it cannot be done it is not easy to know what to do, since gpt doesn't show up that high on a google search. I have put some thoughts about that issue on: https://fedoraproject.org/wiki/PackagingTricks#Use_of_common_namespace
Sorry for the delay of this iteration. Globus made a new release of the globus toolkit, so I had to update my packages accordingly. After communicating with the upstream GPT, I have been informed that the maintenance of GPT has been transferred from NCSA to the Globus Alliance, so I have based the new GPT SRPM on the latest tarfile from globus.org. (This transfer is not indicated anywhere on the www.gridpackagingtools.org website though.) (In reply to comment #7) > In fact the renaming of the package cannot be done only in fedora, > the naming guidelines mandate that the package is called gpt in > that case. So the name is back to gpt, new version is here: SRPM: http://www.grid.tsl.uu.se/repos/globus/fedora/9/src/SRPMS/gpt-3.2-10.fc9.src.rpm SPEC: http://www.grid.tsl.uu.se/repos/globus/fedora/9/info/gpt.spec (In reply to comment #5) > (It is FHS, not HFS). This patch too should be made acceptable by > upstream. It uses configure, so the standard paths could be passed to > the application. They should be used unless the environment variable > is set, in which case the environment variable should be used. I have split the large FHS patch to several smaller ones and submitted them upstream. > > > Why do you change _ in - in script file names? > > > > The upstream code is inconsistent in its naming. It has 20 script names that > > have - and 8 that uses _, for no apparent good reason. Making the naming of the > > scripts consistent is more userfriendly. > > Maybe, but this is something that should be changed upstream and not in > fedora. OK, I have removed this. > > > Reading the /usr/share/gpt/lib/perl/Grid/GPT/LocalEnv.pm > > > file it seems that gtar/gzip is used, so a Requires should be needed, > > > in stead of the perl(Archive::Tar) Requires. Also rpm and rpmbuild > > > seems to be used so maybe some Requires are missing. > > > > I have added Requires for tar and gzip. (I did not do that originally since > > those were listed as exceptions that were not needed to be listed as build > > requirements. But thinking about it that is not quite the same thing.) The > > perl(Archive::Tar) requirement is automatically picked up by rpm, and not > > mentioned in the spec file. And I think it should be there. > > I don't think that perl(Archive::Tar) is used when gtar/gzip is used, so > it shouldn't be required. The system tar/gzip is only used for packaging/unpackaging. The Archive::Tar perl module is still used for extracting metadata about tarfiles, like the number of files in the archive. (Yes, I did try to filter out the requirement and the globus build then failed). > > I did not add any requires for rpm and rpmbuild since their use inside gpt is a > > rather exotic use of gpt. Using gpt to produce rpms that way will create bad > > rpms, with no proper sources. > > I don't think it is an issue. If it is what gpt user are used to. I have added requires on rpm and rpm-build. > > > Also gpt-bootstrap.sh seems to require all the autotools and it is not > > > very clear where it is documented. > > > > Yes bootstrapping requires the autotools, but that is normally the case, so I > > can't see that it requires any special documentation. Please clarify what you > > meant by this comment, since I don't get what you are suggesting. > > If gpt-bootstrap.sh is meant to be used by gpt users, Requires in gpt > are needed for the autotools. I have added requires on autotools (automake, autoconf, libtool). (I hope you won't reject the package now due to rpmlint complaining about the libtool requirement.) > > > Do you really need to rerun the autotools? It doesn't seems clear > > > to me based on the patches. > > > > Patches change configure.in and Makefile.am, so yes. > > Ah, I missed them. Looking at them they seem simple enough that patching > configure and Makefile.in additionally would allow not to rerun the > autotools. OK, The patches grew quite a bit in order to propagate the changes, but it was not impossible, so I have done it this way in the new package. (In reply to comment #6) > In the next iterations, it could be nice to have in the spec file the > the links to the bugzilla you put in Comment #4 in comment near the > patch. Due to the change of maintenance to the Globus Alliance I was asked to resubmit the patches to the globus bugzilla by the current maintainer. I have put links to these new bugzilla reports in the spec file. > I have 2 additional comments regarding the changelog section > * you should remove %{?dist} from the changelog > * I personally prefer when there is a blank line between 2 changelog > entries, but this is a matter of preference. OK, done in the new package.
Sorry for being so long, but I am pretty busy right now. I'll try to progress during the week-end. Also I don't have internet at home anymore which is also slowing me.
The INSTALL file should not be included at %doc section
I still haven't progressed a bit, but hopefully I'll be less busy in the next days...
The package seems right from a strict packaging point of view, now regarding the patches, I find them a bit too important to have them in fedora and not upstream. And they are not perfect either since some hardcoded paths should be set from ./configure. But these are certainly already better than without them, and it would be much better if upstream already acked them as is before improving them. In the general case I would have tend to leave this review open and wait for upstream to adapt the software to be easier to package, with your help and patches, but this blocks many other packagaes, so I am not so sure about what to do. What's your opinion?
I have received the following message during my e-mail correspondence with the maintainer: "As you comment in one of the reports, we are not particularly active developers of GPT. We basically took control of it only because we depended on it and their funding ended. However, I think we will probably wind up applying some if not all of these patches, so I am glad you sent them to us." So there is acknowledgement of the patches from the maintainer, but no upstream integration yet.
Hi, this is the upstream maintainer. We'll need to verify that the patched source works for us; if it does, we'll incorporate these patches into the mainline. I believe we'll require the hardcoded paths to be configurable, but (without looking yet) I can't imagine that will be too hard.
I have created yet another version http://www.grid.tsl.uu.se/repos/globus/fedora/9/src/SRPMS/gpt-3.2-11.fc9.src.rpm based on the feedback from the upstream maintainer. The details for the reasons for the changes are discussed in the upstream bugzilla: http://bugzilla.globus.org/bugzilla/show_bug.cgi?id=6229
I have updated the package due to a compatibility issue with older versions of automake. SRPM: http://www.grid.tsl.uu.se/repos/globus/fedora/9/src/SRPMS/gpt-3.2-12.fc9.src.rpm SPEC: http://www.grid.tsl.uu.se/repos/globus/fedora/9/info/gpt.spec The new additional patch is reported to upstream bugzilla: http://bugzilla.globus.org/bugzilla/show_bug.cgi?id=6564
Due to a naming conflict with an existing package in the Ubuntu distribution the package has been renamed from gpt to grid-packaging-tools. The new package name was discussed in an e-mail exchange with the upstream maintainer. Quoting the upstream maintainer: "I think grid-packaging-tools is fine if that name is not considered too long. Grid Packaging Tools is the official name of the software, it just goes by GPT because it is a more convenient acronym." SRPM: http://www.grid.tsl.uu.se/repos/globus/fedora/10/src/SRPMS/grid-packaging-tools-3.2-13.fc10.src.rpm SPEC: http://www.grid.tsl.uu.se/repos/globus/info/grid-packaging-tools.spec
As discussed by mail, after I've reviewed 3 of your submission and deemed them approvable (the actual approving will happen when you get sponsored) I'll sponsor you. Full review done, looks good, I only have a few small nitpicks: MUST FIX -------- * You are missing: BuildRequires: perl, note that Requires will not get installed during the build, only BuildRequires (and visa versa during install) * Source code changes / conversions should be done in %prep, so please move these 3 lines: iconv -f iso-8859-1 -t utf-8 LICENSE > LICENSE.utf8 touch -c -r LICENSE LICENSE.utf8 mv LICENSE.utf8 LICENSE to %prep SHOULD FIX ---------- * Please add a comment as to why you are removing these files: rm $RPM_BUILD_ROOT%{_sbindir}/gpt-perl-version rm $RPM_BUILD_ROOT%{_datadir}/globus/globus_core-src.tar.gz rm $RPM_BUILD_ROOT%{_datadir}/globus/gpt_scripts_list * It is sort of standard to put %doc in %files the line below %defattr, instead of at the end * These 2 rpmlint messages: grid-packaging-tools.noarch: E: non-executable-script /usr/share/globus/aclocal/bootstrap.frg 0644 grid-packaging-tools.src: W: mixed-use-of-spaces-and-tabs (spaces: line 115, tab: line 1) Simple chmod 755 the bootstrap.frg file, and in your specfile either uses tabs everywhere to indent or spaces, do not mix the 2 note that the Fedora standard is sort of to use spaces. But you're free to use tabs if you prefer.
(In reply to comment #18) > > MUST FIX > -------- > * You are missing: BuildRequires: perl, note that Requires will not get > installed > during the build, only BuildRequires (and visa versa during install) I know the difference between Requires and BuildRequires, however perl always got dragged in for me in the default build environment by e.g. rpm-build or redhat-rpm-config. But an explicit BuildRequires on perl doesn't hurt - added. > * Source code changes / conversions should be done in %prep, so please move > these > 3 lines: > iconv -f iso-8859-1 -t utf-8 LICENSE > LICENSE.utf8 > touch -c -r LICENSE LICENSE.utf8 > mv LICENSE.utf8 LICENSE > to %prep Changed accordingly. > SHOULD FIX > ---------- > * Please add a comment as to why you are removing these files: > rm $RPM_BUILD_ROOT%{_sbindir}/gpt-perl-version > rm $RPM_BUILD_ROOT%{_datadir}/globus/globus_core-src.tar.gz > rm $RPM_BUILD_ROOT%{_datadir}/globus/gpt_scripts_list Done. > * It is sort of standard to put %doc in %files the line below %defattr, instead > of at the end Done. > * These 2 rpmlint messages: > grid-packaging-tools.noarch: E: non-executable-script > /usr/share/globus/aclocal/bootstrap.frg 0644 > grid-packaging-tools.src: W: mixed-use-of-spaces-and-tabs (spaces: line 115, > tab: line 1) The /usr/share/globus/aclocal/bootstrap.frg file is a script fragment - it is not intended to be executable standalone. It does have a shebang so that when put together with other pieces to create a complete script that script will be executable. Making the fragment executable would silence rpmlint, but it would contradict the intended usage of the file. The specfile uses tabs everywhere for indentation. However it is not possible to enter 1.375 tabs. For this you have do 1 tab + 3 spaces. Line 115 (now 120) starts with a tab, so I don't see why rpmlint complains about it using spaces for indentation. If there at some place in the file was a tab immediately following a space, or a set of 2 or more consecutive spaces crossing or ending at an even 8 column boundary then the warning would make sense. This is not the case. I consider this warning a "false positive" - at least for my understanding of what mixed-use-of-spaces-and-tabs means. New version available here: http://www.grid.tsl.uu.se/repos/globus/info/grid-packaging-tools.spec http://www.grid.tsl.uu.se/repos/globus/fedora/10/src/SRPMS/grid-packaging-tools-3.2-14.fc10.src.rpm
(In reply to comment #19) > (In reply to comment #18) > > <snip> > > * These 2 rpmlint messages: > > grid-packaging-tools.noarch: E: non-executable-script > > /usr/share/globus/aclocal/bootstrap.frg 0644 > > grid-packaging-tools.src: W: mixed-use-of-spaces-and-tabs (spaces: line 115, > > tab: line 1) > > The /usr/share/globus/aclocal/bootstrap.frg file is a script fragment - it is > not intended to be executable standalone. It does have a shebang so that when > put together with other pieces to create a complete script that script will be > executable. Making the fragment executable would silence rpmlint, but it would > contradict the intended usage of the file. > Ok, I'm fine with ignoring the rpmlint output then > The specfile uses tabs everywhere for indentation. However it is not possible > to enter 1.375 tabs. For this you have do 1 tab + 3 spaces. Line 115 (now 120) > starts with a tab, so I don't see why rpmlint complains about it using spaces > for indentation. If there at some place in the file was a tab immediately > following a space, or a set of 2 or more consecutive spaces crossing or ending > at an even 8 column boundary then the warning would make sense. This is not the > case. I consider this warning a "false positive" - at least for my > understanding of what mixed-use-of-spaces-and-tabs means. > I agree, one could even call it a bug, so ignore this one too. > New version available here: > > http://www.grid.tsl.uu.se/repos/globus/info/grid-packaging-tools.spec > http://www.grid.tsl.uu.se/repos/globus/fedora/10/src/SRPMS/grid-packaging-tools-3.2-14.fc10.src.rpm Looks good! Once 2 more packages are in good shape I'll sponsor you and approve all 3. I'm not doing the approving right now, as some people run scripts in the flags that sets, checking all approved packages have been imported in to CVS, etc.
I'm going to sponsor Mattias now, so I'm approving this.
New Package CVS Request ======================= Package Name: grid-packaging-tools Short Description: Grid Packaging Tools (GPT) Owners: ellert Branches: F-9 F-10 EL-4 EL-5 InitialCC:
cvs done.
grid-packaging-tools-3.2-16.fc10 has been submitted as an update for Fedora 10. http://admin.fedoraproject.org/updates/grid-packaging-tools-3.2-16.fc10
grid-packaging-tools-3.2-16.fc9 has been submitted as an update for Fedora 9. http://admin.fedoraproject.org/updates/grid-packaging-tools-3.2-16.fc9
grid-packaging-tools-3.2-16.fc10 has been pushed to the Fedora 10 stable repository. If problems still persist, please make note of it in this bug report.
grid-packaging-tools-3.2-16.fc9 has been pushed to the Fedora 9 stable repository. If problems still persist, please make note of it in this bug report.