[Bug 453847] Review Request: gpt - The Grid Packaging Toolkit

[bugzilla at redhat.com]

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

Summary: Review Request: gpt - The Grid Packaging Toolkit


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





------- Additional Comments From mattias.ellert at fysast.uu.se  2008-07-07 04:02 EST -------
(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.


-- 
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, or are watching someone who is.