Bug 453851 - Review Request: globus-common - Globus Toolkit - Common Library
Summary: Review Request: globus-common - Globus Toolkit - Common Library
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Thomas Sailer
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 453852 (view as bug list)
Depends On: gpt 453848 453849
Blocks: globus 453853 453855 453856 453857 453858 453861 453862 453865 467235 467237 467239 478917 478918 478922 478923 478925 478926 478927 478930 478931
TreeView+ depends on / blocked
 
Reported: 2008-07-02 22:09 UTC by Mattias Ellert
Modified: 2009-04-22 06:17 UTC (History)
5 users (show)

Fixed In Version: 10.2-1.fc10
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-04-07 15:49:07 UTC
Type: ---
Embargoed:
fedora: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Mattias Ellert 2008-07-02 22:09:57 UTC
Spec URL: http://www.grid.tsl.uu.se/repos/globus/fedora/9/info/globus-common.spec
SRPM URL: http://www.grid.tsl.uu.se/repos/globus/fedora/9/src/SRPMS/globus-common-7.30-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-common package contains:
Common Library

BuildRequires:  gpt
BuildRequires:  libtool
BuildRequires:  globus-libtool-devel >= 1
BuildRequires:  globus-core-devel >= 4

Comment 1 Mattias Ellert 2008-10-14 05:33:05 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-common-10.2-0.2.fc9.src.rpm
SPEC: http://www.grid.tsl.uu.se/repos/globus/fedora/9/info/globus-common.spec

All applied patches submitted upstream:

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

Segfault fix patch:
http://bugzilla.globus.org/bugzilla/show_bug.cgi?id=6460

Allow plugins without the flavor extension in the name:
http://bugzilla.globus.org/bugzilla/show_bug.cgi?id=6461

Remove bogus inter-package dependency:
http://bugzilla.globus.org/bugzilla/show_bug.cgi?id=6462

Fixes a stupid typo in library/Makefile.am:
http://bugzilla.globus.org/bugzilla/show_bug.cgi?id=6463

The file globus_netos_dir.h does not exist:
http://bugzilla.globus.org/bugzilla/show_bug.cgi?id=6464

Doxygen warning fix:
http://bugzilla.globus.org/bugzilla/show_bug.cgi?id=6465

Bad version information:
http://bugzilla.globus.org/bugzilla/show_bug.cgi?id=6466

Configure options not propagated in globus common setup:
http://bugzilla.globus.org/bugzilla/show_bug.cgi?id=6467

Comment 2 Mattias Ellert 2009-01-06 00:45:24 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-common-10.2-0.3.fc10.src.rpm

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

Two additional patches submitted upstream:

Extradist should not be conditional:
http://bugzilla.globus.org/bugzilla/show_bug.cgi?id=6576

Check for function removed from newer versions of libltdl:
http://bugzilla.globus.org/bugzilla/show_bug.cgi?id=6578

Comment 3 Mattias Ellert 2009-01-06 00:45:52 UTC
*** Bug 453852 has been marked as a duplicate of this bug. ***

Comment 4 Mattias Ellert 2009-03-15 23:58:19 UTC
New version
- Added s390x as 64 bit arch
- Added comment documenting source
- Adapt to changes in the globus-core package

http://www.grid.tsl.uu.se/repos/globus/fedora/10/src/SRPMS/globus-common-10.2-0.5.fc10.src.rpm
http://www.grid.tsl.uu.se/repos/globus/info/globus-common.spec

Comment 5 Rex Dieter 2009-03-16 02:21:17 UTC
Small suggestion, instead of having to enumerate all known 64bit arches via
%ifarch alpha ia64 ppc64 s390x sparc64 x86_64
you could consider using a something like:
%if "%{_lib}" == "lib64"

Comment 6 Mattias Ellert 2009-03-20 18:32:31 UTC
(In reply to comment #5)
> Small suggestion, instead of having to enumerate all known 64bit arches via
> %ifarch alpha ia64 ppc64 s390x sparc64 x86_64
> you could consider using a something like:
> %if "%{_lib}" == "lib64"  

Not all 64 bit architectures use /lib64.

Comment 7 Thomas Sailer 2009-04-06 11:19:26 UTC
rpmlint:
globus-common.src: W: mixed-use-of-spaces-and-tabs (spaces: line 211, tab: line1)
globus-common-devel.i386: W: no-documentation
globus-common-devel.x86_64: W: no-documentation
globus-common-progs.i386: W: no-documentation
globus-common-progs.i386: W: dangerous-command-in-%post cp
globus-common-progs.x86_64: W: no-documentation
globus-common-progs.x86_64: W: dangerous-command-in-%post cp

scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1279217

Comment 8 Thomas Sailer 2009-04-06 11:33:35 UTC
Legend:
  * OK
  ! Needs attention

! rpmlint output - please fix the mixed-spaces-tabs issue. Also consider adding
  the license file to all packages as doc. I don't quite understand what the
  %post script for progs does, so please explain.

* Package is named according to Fedora packaging guidelines

* Spec file is named as the package

! Package follows the Fedora packaging guidelines
  why are the file lists packaged? (such as /usr/share/globus/packages/globus_common/gcc32pthr_rtl.filelist)

* The stated license (ASL 2.0) is a Fedora approved license

* The stated license is the same as the one for the corresponding
  Fedora package

* The package contains the license file (GLOBUS_LICENSE)

* The specfile is written in legible English

! Sources matches upstream, but...
  If you insist on chopping the source tarball, please do it according to
  http://fedoraproject.org/wiki/Packaging/SourceURL,
  section "When upstream uses prohibited code". This section IMO matches
  best what you want to achieve

* Package scratch builds

* BuildRequires look sane

* Owns the directories it creates

* No duplicate files

* %files has %defattr

* %clean clears %buildroot

* Specfile uses macros consistently

* Package does not own other's directories

* %install clears %buildroot

* Installed filenames are valid UTF8

Comment 9 Mattias Ellert 2009-04-06 12:51:30 UTC
(In reply to comment #8)

> ! rpmlint output - please fix the mixed-spaces-tabs issue. Also consider adding
>   the license file to all packages as doc. I don't quite understand what the
>   %post script for progs does, so please explain.

The mixed-spaces-tabs warning is due to a bug in rpmlint, if you update to
rpmlint-0.87-1 (currently in the testing repo) where the bug has been fixed, the warning goes away.

The %post script generates the two files labelled as %ghost in the filelist:
%ghost %{_datadir}/globus/setup/globus-sh-tools-vars.sh
%ghost %{_datadir}/globus/globus-sh-tools-vars.sh

> ! Package follows the Fedora packaging guidelines
>   why are the file lists packaged? (such as
> /usr/share/globus/packages/globus_common/gcc32pthr_rtl.filelist)

These are GPT (grid-packaging-tools) metadata files, used by e.g. gpt-verify and other GPT tools.

> ! Sources matches upstream, but...
>   If you insist on chopping the source tarball, please do it according to
>   http://fedoraproject.org/wiki/Packaging/SourceURL,
>   section "When upstream uses prohibited code". This section IMO matches
>   best what you want to achieve

I think a script is an overkill here, since the instructions are quite simple (extract a complete subdirectory) and can be stated in a comment. The reviewer of globus-core had a similar request (at the time there was no comment in the spec files), and when I added the comment as it is now he was satisfied with it.

Comment 10 Thomas Sailer 2009-04-06 16:32:08 UTC
(In reply to comment #9)

> The %post script generates the two files labelled as %ghost in the filelist:
> %ghost %{_datadir}/globus/setup/globus-sh-tools-vars.sh
> %ghost %{_datadir}/globus/globus-sh-tools-vars.sh

And what are these files used for?

> I think a script is an overkill here, since the instructions are quite simple
> (extract a complete subdirectory) and can be stated in a comment. The reviewer

I don't think "overkill" is the right description for a 3 line script. Everytime there is a globus update, you need to: unpack the original archive, pack the two new archives. Now if you write these 3 commands into a script file, I'm happy.

Furthermore, Fedora has guidelines for what to do if one has to repack the source archive. They are there for a reason. There has to be a good reason for not following them, and IMO calling a 3 line script "overkill" is not a good enough reason...

Thanks

Comment 11 Mattias Ellert 2009-04-06 21:06:13 UTC
(In reply to comment #10)
> (In reply to comment #9)
> 
> > The %post script generates the two files labelled as %ghost in the filelist:
> > %ghost %{_datadir}/globus/setup/globus-sh-tools-vars.sh
> > %ghost %{_datadir}/globus/globus-sh-tools-vars.sh
> 
> And what are these files used for?

These files are sourced by many scripts in this and other globus packages in order to find the location of common tools like cat, chmod, sed and so on. Quoting a few lines from the file as an example:

GLOBUS_SH_AWK="/bin/gawk"
GLOBUS_SH_AUTOCONF="/usr/bin/autoconf"
GLOBUS_SH_BASENAME="/bin/basename"
GLOBUS_SH_CAT="/bin/cat"
GLOBUS_SH_CHGRP="/bin/chgrp"
GLOBUS_SH_CHMOD="/bin/chmod"
GLOBUS_SH_CHOWN="/bin/chown"
GLOBUS_SH_CLEAR="/usr/bin/clear"

It doesn't really matter if all tools are found at installation time when the file is created or not, since the scripts, after having sourced this file, uses constructions like

${GLOBUS_SH_SED-sed}

so if e.g. GLOBUS_SH_SED is not defined in the file it will use sed from the path. This means there is no need to have Requires(post) conditions requiring every tool the %post script tries to find to ensure that all entries are filled. The important thing is that if the file is not there the scripts will fail when they try to source the file, so it must be there. I agree that doing things this way is a bit silly, but it is the way upstream does it.

> > I think a script is an overkill here, since the instructions are quite simple
> > (extract a complete subdirectory) and can be stated in a comment. The reviewer
> 
> I don't think "overkill" is the right description for a 3 line script.
> Everytime there is a globus update, you need to: unpack the original archive,
> pack the two new archives. Now if you write these 3 commands into a script
> file, I'm happy.
> 
> Furthermore, Fedora has guidelines for what to do if one has to repack the
> source archive. They are there for a reason. There has to be a good reason for
> not following them, and IMO calling a 3 line script "overkill" is not a good
> enough reason...

I was obviously not suggesting I should not follow the guidelines. I was trying to argue that what I had done follows the guidelines. The guidelines says:

"There are several cases where upstream is not providing the source to you in an upstream tarball. In these cases you must document how to generate the tarball used in the rpm either through a spec file comment OR a script included as a separate SourceX" (my capitalisation).

I was arguing that using a separate script file in this simple case is an overkill and that using a spec file comment is sufficient. The guideline page quoted above uses a subversion checkout as an example of a simple case where a specfile comment is used. Extracting a directory from a tarball is an operation of similar complexity. The example where a separate script is used is much more elaborate and involves removing specific files from a tarball as well as modifying other files using sed expressions. Extracting a directory from a tarball is no way nearly as complicated as that. The operations needed to extract a directory from a tarball are in my opinion simple enough not to warrant creating a separate script to explain them, nor the additional burden of maintaining such a script.

If on the other hand your objection really is that the comment was to terse to allow a reviewer or anyone else interested to reproduce the source tarball that is an other matter. I have created an updated specfile where I have made the comment more explicit. (The new comment is similar to the subversion checkout example - which I find more relevant in this case.) I hope this will be to your satisfaction.

http://www.grid.tsl.uu.se/repos/globus/info/new/globus-common.spec

Comment 12 Thomas Sailer 2009-04-06 21:17:51 UTC
Thanks for clarifying how to repack the source archive. Please keep the notes uptodate when you bump the version.

APPROVED by sailer.

Comment 13 Mattias Ellert 2009-04-06 21:45:17 UTC
New Package CVS Request
=======================
Package Name: globus-common
Short Description: Globus Toolkit - Common Library
Owners: ellert
Branches: F-9 F-10 EL-4 EL-5
InitialCC:

Comment 14 Kevin Fenzi 2009-04-07 03:15:30 UTC
cvs done.

Comment 15 Fedora Update System 2009-04-07 09:45:32 UTC
globus-common-10.2-1.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/globus-common-10.2-1.fc9

Comment 16 Fedora Update System 2009-04-07 09:45:39 UTC
globus-common-10.2-1.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/globus-common-10.2-1.fc10

Comment 17 Fedora Update System 2009-04-07 15:49:02 UTC
globus-common-10.2-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 18 Fedora Update System 2009-04-07 15:49:51 UTC
globus-common-10.2-1.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 19 Orcan Ogetbil 2009-04-08 03:19:45 UTC
There is a file conflict between globus-common-progs and grid-packaging-tools.

The file /usr/share/globus/config.guess exists in both packages. This must be sorted out. Is this file really needed in any of these packages?

Comment 20 Mattias Ellert 2009-04-19 05:20:51 UTC
(In reply to comment #19)
> There is a file conflict between globus-common-progs and grid-packaging-tools.
> 
> The file /usr/share/globus/config.guess exists in both packages. This must be
> sorted out. Is this file really needed in any of these packages?  

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.

This has been fixed in rawhide. F9 and F10 never saw the problem (since the files in the two conflicting packages are identical there). The update for F11 (and F9 and F10 too, though less critical) have been submitted but not yet pushed as an update.

Comment 21 Orcan Ogetbil 2009-04-19 22:50:50 UTC
When two packages contain the same file in the same path, there is always a problem, even if they are identical. The packages will not install because rpm will say that they conflict.

Am I missing something?

Comment 22 Mattias Ellert 2009-04-20 04:15:48 UTC
(In reply to comment #21)
> When two packages contain the same file in the same path, there is always a
> problem, even if they are identical. The packages will not install because rpm
> will say that they conflict.
> 
> Am I missing something?  

The packages will only conflict if the files are different. If they are the same there is no problem installing both packages (like on F-9 and F-10 in this case):

[ellert@ellert ~]$ rpm -q --whatprovides /usr/share/globus/config.guess 
globus-common-progs-10.2-1.fc9.i386
grid-packaging-tools-3.2-17.fc9.noarch

That it is possible does of course not mean this is a good thing to do in this case.

This feature is regularly used to allow multi-arch installations where e.g. header files are the same in the -devel package for i386 an x86_64, but can be installed simultaneously. But this is as far as I see the only really valid usecase for it.

Comment 23 Orcan Ogetbil 2009-04-20 04:44:00 UTC
(In reply to comment #22)
> This feature is regularly used to allow multi-arch installations where e.g.
> header files are the same in the -devel package for i386 an x86_64, but can be
> installed simultaneously. But this is as far as I see the only really valid
> usecase for it.  

Hmm, the last time I tried installing a -devel package for i386 and x86_64, rpm complained about conflicts for identical files. Probably there was a glitch in my setup or the particular rpm version. I always wondered why they made identical files conflict. Thanks for clearing it up.

Comment 24 Mattias Ellert 2009-04-22 06:17:04 UTC
(In reply to comment #19)
> There is a file conflict between globus-common-progs and grid-packaging-tools.
> 
> The file /usr/share/globus/config.guess exists in both packages. This must be
> sorted out. Is this file really needed in any of these packages?  

The issue with the duplicate files has been reported as a separate bug #496921. For further comments on this issue please add them to that bug report.


Note You need to log in before you can comment on or make changes to this bug.