Bug 453848
Summary: | Review Request: globus-core - Globus Toolkit - Globus Core | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Mattias Ellert <mattias.ellert> |
Component: | Package Review | Assignee: | Hans de Goede <hdegoede> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | low | ||
Version: | rawhide | CC: | fedora-package-review, hdegoede, javiplx, notting, pertusus, waananen |
Target Milestone: | --- | Flags: | hdegoede:
fedora-review+
kevin: fedora-cvs+ |
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | 5.15-1.fc10 | Doc Type: | Bug Fix |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2009-03-23 15:50:36 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | 453847 | ||
Bug Blocks: | 453815, 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 |
Description
Mattias Ellert
2008-07-02 22:07:22 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. Mattias, any comment? 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. 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 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 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. (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. (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. 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 (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? 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 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. 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. (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 (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. I'm going to sponsor Mattias now, so I'm approving this. 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: cvs done. 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 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 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 The Bodhi bug was fixed, so this time it worked. https://fedorahosted.org/bodhi/ticket/308 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. 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. |