Bug 453848 - Review Request: globus-core - Globus Toolkit - Globus Core
Summary: Review Request: globus-core - Globus Toolkit - Globus Core
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Hans de Goede
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: gpt
Blocks: globus 453849 453850 453851 453852 453853 453854 453855 453856 453857 453858 453861 453862 453865 467235 467237 467239 478917 478918 478921 478922 478923 478925 478926 478927 478928 478929 478930 478931
TreeView+ depends on / blocked
 
Reported: 2008-07-02 22:07 UTC by Mattias Ellert
Modified: 2009-03-23 15:58 UTC (History)
6 users (show)

Fixed In Version: 5.15-1.fc10
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-03-23 15:50:36 UTC
Type: ---
Embargoed:
hdegoede: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Mattias Ellert 2008-07-02 22:07:22 UTC
Spec URL: http://www.grid.tsl.uu.se/repos/globus/fedora/9/info/globus-core.spec
SRPM URL: http://www.grid.tsl.uu.se/repos/globus/fedora/9/src/SRPMS/globus-core-4.34-0.1.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-core package contains:
Globus Core

BuildRequires:  gpt
BuildRequires:  libtool

Comment 1 Javier Palacios 2008-08-13 19:01:46 UTC
The Source should be a full URL pointing to the source.

I have seen that the reported URL for the SRPM has changed due to a new version. It might be a good idea to reflect that on bugzilla

Also, as I'm able to follow the package, the real source code (and probably for some other globus-* packages) is on a full globus-5.14.tar.gz file. I'm not sure if there is a policy about that, but having many packages sharing a main source tarball might cause problems. It's probably much better (and faster for building) to have a single source package for multiple globus things and handle it as subpackages.

Finally, the changelog entry says that the SPEC is automatically created. I'm not sure if people from Fedora might accept a program as packager, but even in that case you will very likely be required to ensure that the program implements all current (and future) policies.

Comment 2 Patrice Dumas 2008-09-04 14:26:01 UTC
Mattias, any comment?

Comment 3 Mattias Ellert 2008-09-22 07:23:50 UTC
The globus alliance no longer provides tar files for sources for individual packages in their releases on their website. Instead, the sources for all the packages in the release are bundled together in a big monolithic tar file:

http://www-unix.globus.org/ftppub/gt4/4.2.0/installers/src/gt4.2.0-all-source-installer.tar.bz2

This monolithic thing currently contains the sources for 294 packages.

In my source RPMs I do not include this big tarfile as source. I would find it to be an extreme waste of disk space and bandwidth if this tarfile would have to be included in every source package since only one 294th of it is actually used for the build. Instead I have extracted the relevant subdirectory for each package into a per-package tar file. Which of course means that there is no URL pointing to this exact file on the upstream website.

For between-release-updates the globus alliance do provide per-package tar files:

http://www-unix.globus.org/ftppub/gt4/4.2.0/updates/src/

For these updated packages the provided tar files can be used with no problem. For the non-updated packages I extract this kind of per-package tar files from the monolithic source tree.

The globus build for each package requires that the other globus packages that it depends on are already installed. So putting all the globus packages in one source RPM and build as sub packages is not really possible without reworking completely the way internal globus dependencies are handled.

(If you use the globus build script it pretends to first do "make" for all packages and then "make install" for all packages, but in this case the "make" is actually "make" followed by "make install" and "make install" is only post installation configuration.)

The "Automatically generated" is just a print statement in a perl script I have written to convert the globus XML build instructions to an RPM spec. If you just don't like the wording I can change it to whatever you like, including the customary "Initial build". For most packages the perl script generated spec does not require further polishing, but occasionally some manual tweaks are needed. I would still consider myself, and not the script, as being the packager.

Comment 4 Mattias Ellert 2008-10-14 05:32:33 UTC
New version based on the globus toolkit 4.2.1 release:

SRPM: http://www.grid.tsl.uu.se/repos/globus/fedora/9/src/SRPMS/globus-core-5.15-0.2.fc9.src.rpm
SPEC: http://www.grid.tsl.uu.se/repos/globus/fedora/9/info/globus-core.spec

All applied patches submitted upstream:

Make file locations configurable:
http://bugzilla.globus.org/bugzilla/show_bug.cgi?id=6451

Environment variable elimination patch:
http://bugzilla.globus.org/bugzilla/show_bug.cgi?id=6452

Missing functions in configure check:
http://bugzilla.globus.org/bugzilla/show_bug.cgi?id=6453

Detect IOV_MAX correctly (requires _GNU_SOURCE):
http://bugzilla.globus.org/bugzilla/show_bug.cgi?id=6454

Make GPT_AGE_VERSION available to autotools:
http://bugzilla.globus.org/bugzilla/show_bug.cgi?id=6455

Missing shebang:
http://bugzilla.globus.org/bugzilla/show_bug.cgi?id=6456

Make configure --help look nice:
http://bugzilla.globus.org/bugzilla/show_bug.cgi?id=6457

Fix doxygen documentation installation instructions:
http://bugzilla.globus.org/bugzilla/show_bug.cgi?id=6458

Comment 5 Mattias Ellert 2009-01-06 00:42:51 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-core-5.15-0.3.fc10.src.rpm

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

Two additional patches submitted upstream:

Make it work with older automake versions:
http://bugzilla.globus.org/bugzilla/show_bug.cgi?id=6565

Avoid race condition when renaming the libtool file:
http://bugzilla.globus.org/bugzilla/show_bug.cgi?id=6575

Comment 6 Hans de Goede 2009-02-16 12:44:01 UTC
Not a full review yet but a first round of remarks:

MUST FIX:
---------
* Add a comment explaining how to get the Source0 tarbal, so people who want
  to verify its contents against the original can do that
* s390x is a 64 bit arch also
* since the devel subpackage requires the main package there is no need for it
  to own directories which are also owned by the main package


SHOULD FIX:
-----------
* rpmlint warning:
globus-core.src: W: mixed-use-of-spaces-and-tabs (spaces: line 116, tab: line 1)

* How did you come to the devel / non-devel split. Atleast the aclocal and doxygen
  files look like devel files to me. Only files which are needed to *run* globus
  tk using applications should be in the main package, the rest should all be in
  the devel package

* Given the short list of files in the package I see no need for all the magic
  to generate filelists. Why not just add everything manually (with wildcards)
  to %files, that way it is much clearer what is going on

* Why do you filter out the requires on the gpt modules, the -devel package
  requiring gpt is fine, and if the main package gets auto requires for gpt
  that feels like a hint that the package is not split properly.

Comment 7 Mattias Ellert 2009-02-16 16:30:07 UTC
(In reply to comment #6)

> MUST FIX:
> ---------
> * Add a comment explaining how to get the Source0 tarbal, so people who want
>   to verify its contents against the original can do that

The source was extracted from the globus distribution tarball:

http://www-unix.globus.org/ftppub/gt4/4.2.1/installers/src/gt4.2.1-all-source-installer.tar.bz2

using this script:

http://www.grid.tsl.uu.se/repos/globus/scripts/split-release.sh

I should be able to think up a short comment along those lines (and reuse it for the other globus packages.)

> * s390x is a 64 bit arch also

I'll add it

> * since the devel subpackage requires the main package there is no need for it
>   to own directories which are also owned by the main package

OK I'll fix that. (I previously saw some problems with left-behind orphaned directories, but I can't seem the reproduce those problems now.)

> SHOULD FIX:
> -----------
> * rpmlint warning:
> globus-core.src: W: mixed-use-of-spaces-and-tabs (spaces: line 116, tab: line
> 1)

This is the same thing as in the grid-packaging-tools package - I still think this is a false positive.

> * How did you come to the devel / non-devel split. Atleast the aclocal and
> doxygen
>   files look like devel files to me. Only files which are needed to *run*
> globus
>   tk using applications should be in the main package, the rest should all be
> in
>   the devel package

The globus-core package is very different from the rest of the globus packages. All of globus-core is devel, none of it is needed at runtime. I did the split so that architecture independent files are in the main package and the architecture dependent files are in the devel-package. For a i386 on x86_64 installation you could then install main + devel from x86_64 and devel from i386. I found that to be the most natural split if a split should be done. Thinking about it, it might make more sense to just put all the files in main and not split it into subpackages. You could then still install both i386 and x86_64 together since the common files would be exactly the same. Is more sensible?

> * Given the short list of files in the package I see no need for all the magic
>   to generate filelists. Why not just add everything manually (with wildcards)
>   to %files, that way it is much clearer what is going on

There is no magic here. The split between packages is automatically defined by gpt. What is done is simply a format conversion form the gpt filelist format to the rpm filelist format. I agree that in the case of globus-core, which is not split in so many sub packages you don't gain a lot. The gain is much more noticeable in packages that generate four or five sub packages. From a package maintainability point of view it is however much easier to use the same packaging instructions for all globus packages, though it is a slight overkill for globus-core.

> * Why do you filter out the requires on the gpt modules, the -devel package
>   requiring gpt is fine, and if the main package gets auto requires for gpt
>   that feels like a hint that the package is not split properly.

As I said, all of globus-core is really devel. No non-devel package requires globus-core. However all globus-*-devel packages require globus-core and If you install a globus-*-devel package for anything else than building other globus packages you don't need gpt. I really don't like having gpt being dragged in by anything. I consider this a major feature of the packaging.

I didn't prepare a new package yet, since you indicated that you might have additional comments already. Let me know if you want me to create a new package version at this stage.

Comment 8 Hans de Goede 2009-02-23 13:38:56 UTC
(In reply to comment #7)
> (In reply to comment #6)
> 

<snip>

> > SHOULD FIX:
> > -----------
> > * rpmlint warning:
> > globus-core.src: W: mixed-use-of-spaces-and-tabs (spaces: line 116, tab: line
> > 1)
> 
> This is the same thing as in the grid-packaging-tools package - I still think
> this is a false positive.
> 

And I still agree :)

> > * How did you come to the devel / non-devel split. Atleast the aclocal and
> > doxygen
> >   files look like devel files to me. Only files which are needed to *run*
> > globus
> >   tk using applications should be in the main package, the rest should all be
> > in
> >   the devel package
> 
> The globus-core package is very different from the rest of the globus packages.
> All of globus-core is devel, none of it is needed at runtime. I did the split
> so that architecture independent files are in the main package and the
> architecture dependent files are in the devel-package. For a i386 on x86_64
> installation you could then install main + devel from x86_64 and devel from
> i386. I found that to be the most natural split if a split should be done.
> Thinking about it, it might make more sense to just put all the files in main
> and not split it into subpackages. You could then still install both i386 and
> x86_64 together since the common files would be exactly the same. Is more
> sensible?
> 

Yes please do it that way, but keep the -devel subpackage and put all the files in the %files of the devel subpackage. If you then also remove the %files line itself for the main package, only the subpackage will be build.

This way we have a srpm and specfile name matching upstream, and still have a -devel extension to make clear this is a devel package

> > * Given the short list of files in the package I see no need for all the magic
> >   to generate filelists. Why not just add everything manually (with wildcards)
> >   to %files, that way it is much clearer what is going on
> 
> There is no magic here. The split between packages is automatically defined by
> gpt. What is done is simply a format conversion form the gpt filelist format to
> the rpm filelist format. I agree that in the case of globus-core, which is not
> split in so many sub packages you don't gain a lot. The gain is much more
> noticeable in packages that generate four or five sub packages. From a package
> maintainability point of view it is however much easier to use the same
> packaging instructions for all globus packages, though it is a slight overkill
> for globus-core.
> 

Ok.

> > * Why do you filter out the requires on the gpt modules, the -devel package
> >   requiring gpt is fine, and if the main package gets auto requires for gpt
> >   that feels like a hint that the package is not split properly.
> 
> As I said, all of globus-core is really devel. No non-devel package requires
> globus-core. However all globus-*-devel packages require globus-core and If you
> install a globus-*-devel package for anything else than building other globus
> packages you don't need gpt. I really don't like having gpt being dragged in by
> anything. I consider this a major feature of the packaging.
> 

Ok.

> I didn't prepare a new package yet, since you indicated that you might have
> additional comments already. Let me know if you want me to create a new package
> version at this stage.

Please do a new version without the package split, then I'll do a full review of that one.

Comment 9 Mattias Ellert 2009-02-28 21:11:04 UTC
New version available:

SRPM: http://www.grid.tsl.uu.se/repos/globus/fedora/10/src/SRPMS/globus-core-5.15-0.4.fc10.src.rpm
SPEC: http://www.grid.tsl.uu.se/repos/globus/info/globus-core.spec

- main merged with devel as requested
- added s390x to the list of 64 bit platforms

Comment 10 Hans de Goede 2009-03-09 10:03:38 UTC
(In reply to comment #9)
> New version available:
> 
> SRPM:
> http://www.grid.tsl.uu.se/repos/globus/fedora/10/src/SRPMS/globus-core-5.15-0.4.fc10.src.rpm
> SPEC: http://www.grid.tsl.uu.se/repos/globus/info/globus-core.spec
> 
> - main merged with devel as requested
> - added s390x to the list of 64 bit platforms  

Hi,

sorry for the slow response a wall of work hit me. However atm I cannot get to your srpm / spec file, can you check this please?

Comment 11 Mattias Ellert 2009-03-09 13:34:54 UTC
Hi,

the server went down yesterday due to a faulty fan. It is up again, but it is not quite stable because the fan has not been replaced yet. I have put copies on a different server in case the server goes down again:

http://www3.tsl.uu.se/~ellert/globus/globus-core.spec
http://www3.tsl.uu.se/~ellert/globus/globus-core-5.15-0.4.fc10.src.rpm

Comment 12 Hans de Goede 2009-03-13 13:08:22 UTC
Full review done, looks good, so I'll approve when I'm ready to sponsor you.

2 small things you could change:

1) Put %doc directly under %files
2) Put:
rm -rf autom4te.cache
./bootstrap
   Under %prep, that is where *most* people do autofoo regeneration.

Neither are blockers though.

Comment 13 Hans de Goede 2009-03-13 13:36:20 UTC
Ok, so I see now reviewing other globus packages that quite a few are for devel use only. I think that doing empty main packages for all those is a bad idea,
in retrospect it was a bad idea for this one too. It seemed like a clever
trick, but it contradicts Fedora's normal way of handling this, which is
if a package has only development use, its ok to have files which would
normally be part of a -devel package in the main package.

So please drop the "devel" from the %files list and remove the -devel subpackage,
that will also yield a nice simplification (lines reduction) of the spec file.

Comment 14 Mattias Ellert 2009-03-15 09:58:04 UTC
(In reply to comment #13)
> Ok, so I see now reviewing other globus packages that quite a few are for devel
> use only.

No, globus-core is the only one.

> I think that doing empty main packages for all those is a bad idea,
> in retrospect it was a bad idea for this one too. It seemed like a clever
> trick, but it contradicts Fedora's normal way of handling this, which is
> if a package has only development use, its ok to have files which would
> normally be part of a -devel package in the main package.
> 
> So please drop the "devel" from the %files list and remove the -devel
> subpackage, that will also yield a nice simplification (lines reduction)
> of the spec file.  

New iteration implementing this request available here:

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

Comment 15 Hans de Goede 2009-03-16 09:58:01 UTC
(In reply to comment #14)
> (In reply to comment #13)
> > Ok, so I see now reviewing other globus packages that quite a few are for devel
> > use only.
> 
> No, globus-core is the only one.
> 

Ah, my mistake.

> > So please drop the "devel" from the %files list and remove the -devel
> > subpackage, that will also yield a nice simplification (lines reduction)
> > of the spec file.  
> 
> New iteration implementing this request available here:
> 
> SRPM:
> http://www.grid.tsl.uu.se/repos/globus/fedora/10/src/SRPMS/globus-core-5.15-0.5.fc10.src.rpm
> SPEC: http://www.grid.tsl.uu.se/repos/globus/info/globus-core.spec  

Thanks! I really think this is the best way forward, sorry for the change of mind.
When importing into CVS (once sponsored) please use this version.

Comment 16 Hans de Goede 2009-03-16 10:08:53 UTC
I'm going to sponsor Mattias now, so I'm approving this.

Comment 17 Mattias Ellert 2009-03-18 15:28:14 UTC
New Package CVS Request
=======================
Package Name: globus-core
Short Description: Globus Toolkit - Globus Core
Owners: ellert
Branches: F-9 F-10 EL-4 EL-5
InitialCC:

Comment 18 Kevin Fenzi 2009-03-20 03:10:01 UTC
cvs done.

Comment 19 Mattias Ellert 2009-03-20 16:30:27 UTC
For some reason bodhi fails to add these comments automatically.
(The web interface says "Unable to access one or more bugs".)
So I add these myself...

globus-core-5.15-1.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/globus-core-5.15-1.fc9

globus-core-5.15-1.fc9 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/globus-core-5.15-1.fc10

Comment 20 Fedora Update System 2009-03-21 02:39:16 UTC
globus-core-5.15-1.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/globus-core-5.15-1.fc10

Comment 21 Fedora Update System 2009-03-21 02:39:23 UTC
globus-core-5.15-1.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/globus-core-5.15-1.fc9

Comment 22 Mattias Ellert 2009-03-21 02:44:18 UTC
The Bodhi bug was fixed, so this time it worked.
https://fedorahosted.org/bodhi/ticket/308

Comment 23 Fedora Update System 2009-03-23 15:50:30 UTC
globus-core-5.15-1.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 24 Fedora Update System 2009-03-23 15:58:03 UTC
globus-core-5.15-1.fc10 has been pushed to the Fedora 10 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.