Bug 453850 - Review Request: globus-openssl - Openssl Library (virtual GPT glue package)
Review Request: globus-openssl - Openssl Library (virtual GPT glue package)
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Orcan Ogetbil
Fedora Extras Quality Assurance
:
Depends On: gpt 453848
Blocks: globus 453853 453854 453855 453856 453857 453858 453861 453862 453865 467237
  Show dependency treegraph
 
Reported: 2008-07-02 18:09 EDT by Mattias Ellert
Modified: 2009-05-09 00:10 EDT (History)
3 users (show)

See Also:
Fixed In Version: 3.0-1.fc11
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-04-21 20:51:12 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
oget.fedora: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Mattias Ellert 2008-07-02 18:09:09 EDT
Spec URL: http://www.grid.tsl.uu.se/repos/globus/fedora/9/info/globus-openssl.spec
SRPM URL: http://www.grid.tsl.uu.se/repos/globus/fedora/9/src/SRPMS/globus-openssl-1.20-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-openssl package contains:
Openssl Library (virtual GPT glue package)

BuildRequires:  gpt
BuildRequires:  libtool
BuildRequires:  globus-core-devel >= 4
Comment 2 Mattias Ellert 2009-01-05 19:43:21 EST
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-openssl-3.0-0.3.fc10.src.rpm

SPEC: http://www.grid.tsl.uu.se/repos/globus/info/globus-openssl.spec
Comment 3 Mattias Ellert 2009-03-15 19:58:09 EDT
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-openssl-3.0-0.5.fc10.src.rpm
http://www.grid.tsl.uu.se/repos/globus/info/globus-openssl.spec
Comment 4 Orcan Ogetbil 2009-04-07 13:53:01 EDT
(In reply to comment #3)
> - Added comment documenting source

Sorry, but I don't see the comment documenting the source. Am I missing something?


Other than this, there are a few other things I'd like to clarify:

* Why is this package not noarch? (The .pc file can go to /usr/share/pkgconfig)

* The guidelines say:
    If (and only if) the source package includes the text of the license(s) in 
    its own file, then that file, containing the text of the license(s) for the 
    package must be included in %doc.

Here, the license file is supplied separately. Is there a reason that we should grant an exception to this package?
Comment 5 Orcan Ogetbil 2009-04-07 14:58:59 EDT
I see that there are arch dependent non-binary files. Then, why are they going to %{_datadir} instead of %{_libdir}?
Comment 6 Mattias Ellert 2009-04-07 19:24:23 EDT
(In reply to comment #4)
> (In reply to comment #3)
> > - Added comment documenting source
> 
> Sorry, but I don't see the comment documenting the source. Am I missing
> something?

Sorry for the confusion. I updated all the globus packages at the same time and pasted the same entry in the bugzilla comment for all of them (which was the right one for 28 of 30 packages). Since the source for this package is not extracted from upstream like for the other packages, the comment added to the other packages (which tells how the source was extracted from upstream) does not apply to this package - so I did not add that comment to this package.

I will add a different comment to this package explaining what the source file is and why there is no upstream source for this package.

> Other than this, there are a few other things I'd like to clarify:
> 
> * Why is this package not noarch? (The .pc file can go to /usr/shar/pkgconfig)

The content of the package differs for different architectures. (I guess you realised that based on your comment 5.)

> * The guidelines say:
>     If (and only if) the source package includes the text of the license(s) in 
>     its own file, then that file, containing the text of the license(s) for the 
>     package must be included in %doc.
> 
> Here, the license file is supplied separately. Is there a reason that we should
> grant an exception to this package?  

I should just drop the license file. The argument I had for including it was quite weak, and not really convincing, so I will not bother you with it.

(In reply to comment #5)
> I see that there are arch dependent non-binary files. Then, why are they going
> to %{_datadir} instead of %{_libdir}?  

The files in /usr/share/globus/packages are GPT metadata files. GPT expects all metadata files to be in the same directory. GPT supports installing packages from different architectures in parallel by using different filenames for different architectures for architecture dependent metadata, so there are no filename conflicts for a multi-arch installation. Even though the metadata describes architecture dependent information, the metadata itself is architecture independent. A query on the metadata can return information about multiple architectures on a multi-arch installation.


An updated specfile is available here:

http://www.grid.tsl.uu.se/repos/globus/info/new/globus-openssl.spec
Comment 7 Orcan Ogetbil 2009-04-07 20:53:52 EDT
OK, here are my comments, notes, questions etc.

- rpmlints
   globus-openssl.x86_64: E: no-binary
   globus-openssl-devel.x86_64: W: no-documentation
   globus-openssl-progs.x86_64: W: no-documentation
can be ignored.

? Where does the version number come from? and why is the release number 0.x?

* The license seems to be just ASL 2.0. Is there any non-trivial differences? If not, please use ASL 2.0:
   http://fedoraproject.org/wiki/Licensing#Good_Licenses

! No need to BR: pkgconfig on the main package. It will be picked up by openssl-devel.

! No need to Requires: openssl on the progs subpackage since it already requires the main package which requires openssl.

! No need to Requires: zlib-devel and pkgconfig on the devel subpackage since openssl-devel will pull them up.

* The new guidelines suggest that %global should be preferred over %define

! Could you collect all your "%global"s at one place?

! Please make the descriptions span 80 columns

? Why are you packaging the .filelist files?

? Packages must not own files or directories already owned by other packages. 
   /usr/share/globus and /usr/share/globus/packages is already owned by globus-core. Shouldn't you just put globus-core as a requirement to this package? Is this package useful without globus-core?
Comment 8 Orcan Ogetbil 2009-04-07 22:18:15 EDT
Additionally,

! I suggest you to use macros such as %{_sbindir} and %{_datadir}, instead of the hardcoded /usr/sbin and /usr/share
Comment 9 Orcan Ogetbil 2009-04-13 02:16:28 EDT
One more thing:

Shouldn't this package be called globus-gsi-openssl for consistency with other globus-gsi-openssl-* packages? All the source is in the 
   source-trees/gsi/openssl*
directories of the main tarball.

Or conversely, shouldn't the other packages be called globus-openssl-* ?
Comment 10 Mattias Ellert 2009-04-17 09:43:51 EDT
(In reply to comment #7)

> ? Where does the version number come from? and why is the release number 0.x?

The version is the same as for the upstream package (containing the copy of the openssl sources) this package replaces.

The release was 0.x because I wanted to have a clear upgrade path from my third party repository containing the packages and the official Fedora version once the packages are accepted. My plan was to change the release to 1 when importing it to CVS. My new package that I list below now gives the release number as 1 already, but I didn't import it to my third party repo.

> * The license seems to be just ASL 2.0. Is there any non-trivial differences?
> If not, please use ASL 2.0:
>    http://fedoraproject.org/wiki/Licensing#Good_Licenses

The licence is Apache-2.0 without any changes, and the specfile already states ASL 2.0 accordingly.

> ! No need to BR: pkgconfig on the main package. It will be picked up by
> openssl-devel.

Fixed.

> ! No need to Requires: openssl on the progs subpackage since it already
> requires the main package which requires openssl.

Fixed.

> ! No need to Requires: zlib-devel and pkgconfig on the devel subpackage since
> openssl-devel will pull them up.

This Requires was put in there to workaround the broken openssl-devel package in RHL9 which is missing the Requires on zlib-devel. I have now made the Requires conditional so that it will only be used in this case and not for releases where openssl-devel is not broken.

> * The new guidelines suggest that %global should be preferred over %define

Fixed.

> ! Could you collect all your "%global"s at one place?

The globals are now first in the file (which seems to be the custom nowadays). Except for the global that defines %_name which must come after the Name tag, since it uses %name in its definition and this is not defined before the Name tag is parsed. This is also the most logical place for it.

> ! Please make the descriptions span 80 columns

Fixed.

> ? Why are you packaging the .filelist files?

This is part of the package GPT metadata. They are needed by GPT.

> ? Packages must not own files or directories already owned by other packages. 
>    /usr/share/globus and /usr/share/globus/packages is already owned by
> globus-core. Shouldn't you just put globus-core as a requirement to this
> package? Is this package useful without globus-core?  

globus-core is a development only package. globus-openssl and globus-openssl-progs are runtime packages. Runtime packages must not Require development packages.

(In reply to comment #8)
> Additionally,
> 
> ! I suggest you to use macros such as %{_sbindir} and %{_datadir}, instead of
> the hardcoded /usr/sbin and /usr/share  

Fixed

(In reply to comment #9)
> One more thing:
> 
> Shouldn't this package be called globus-gsi-openssl for consistency with other
> globus-gsi-openssl-* packages? All the source is in the 
>    source-trees/gsi/openssl*
> directories of the main tarball.

The package names are taken from the GPT source package description file pkgdata/pkg_data_src.gpt.in (by replacing underscores with hyphens to comply with the naming guidelines). They are not derived from the directory structure of the installer tarball.

> Or conversely, shouldn't the other packages be called globus-openssl-* ?  

Same answer as above.

New version is available here:

http://www.grid.tsl.uu.se/repos/globus/info/new/globus-openssl-3.0-1.fc10.src.rpm
http://www.grid.tsl.uu.se/repos/globus/info/new/globus-openssl.spec
Comment 11 Orcan Ogetbil 2009-04-17 11:48:49 EDT
Thanks for the update.

(In reply to comment #10)
> (In reply to comment #7)
> 
> > ! Could you collect all your "%global"s at one place?
> 
> The globals are now first in the file (which seems to be the custom nowadays).
> Except for the global that defines %_name which must come after the Name tag,
> since it uses %name in its definition and this is not defined before the Name
> tag is parsed. This is also the most logical place for it.
> 

This shouldn't matter for rpm, i.e. rpm doesn't needs these to be in logical order. But it's a matter of taste. I won't say anything more.

I have one last question:

> 
> > ? Packages must not own files or directories already owned by other packages. 
> >    /usr/share/globus and /usr/share/globus/packages is already owned by
> > globus-core. Shouldn't you just put globus-core as a requirement to this
> > package? Is this package useful without globus-core?  
> 
> globus-core is a development only package. globus-openssl and
> globus-openssl-progs are runtime packages. Runtime packages must not Require
> development packages.
> 

How about globus-common? That one is a runtime package and "sounds" like the one all the other globus packages would require. Multiple ownership is not much desired that's why I'm asking. Is the globus-openssh package worth anything without having globus-common installed?
Comment 12 Mattias Ellert 2009-04-17 15:37:44 EDT
(In reply to comment #11)
> Thanks for the update.
> 
> (In reply to comment #10)
> > (In reply to comment #7)
> > 
> > > ! Could you collect all your "%global"s at one place?
> > 
> > The globals are now first in the file (which seems to be the custom nowadays).
> > Except for the global that defines %_name which must come after the Name tag,
> > since it uses %name in its definition and this is not defined before the Name
> > tag is parsed. This is also the most logical place for it.
> > 
> 
> This shouldn't matter for rpm, i.e. rpm doesn't needs these to be in logical
> order. But it's a matter of taste. I won't say anything more.

I did actually test it before writing it. The order does matter.

> I have one last question:
> 
> > 
> > > ? Packages must not own files or directories already owned by other packages. 
> > >    /usr/share/globus and /usr/share/globus/packages is already owned by
> > > globus-core. Shouldn't you just put globus-core as a requirement to this
> > > package? Is this package useful without globus-core?  
> > 
> > globus-core is a development only package. globus-openssl and
> > globus-openssl-progs are runtime packages. Runtime packages must not Require
> > development packages.
> > 
> 
> How about globus-common? That one is a runtime package and "sounds" like the
> one all the other globus packages would require. Multiple ownership is not much
> desired that's why I'm asking. Is the globus-openssh package worth anything
> without having globus-common installed?  

There is at least one usecase for having globus-openssl without globus-common. This is for building globus-gsi-proxy-ssl (see review reqest bug #453854). This requires globus-core and globus-openssl, but not globus-common.
Comment 13 Orcan Ogetbil 2009-04-17 16:04:53 EDT
(In reply to comment #12)
> (In reply to comment #11)
> > 
> > This shouldn't matter for rpm, i.e. rpm doesn't needs these to be in logical
> > order. But it's a matter of taste. I won't say anything more.
> 
> I did actually test it before writing it. The order does matter.
> 

That's interesting. It works either way here. Maybe the RPM version matters. I tested this on F-10 and F-11. Oh well... No big deal.

> There is at least one usecase for having globus-openssl without globus-common.
> This is for building globus-gsi-proxy-ssl (see review reqest bug #453854). 
> This requires globus-core and globus-openssl, but not globus-common.  

globus-common is not that common then :)

I was just asking about the usage, not for building packages. If you think there might be people, who will use globus-openssl, but don't want to have globus-common installed, then multiple ownership is okay. I trust your judgement.

-------------------------------------------------
This package (globus-openssl) is APPROVED by oget
-------------------------------------------------
Comment 14 Orcan Ogetbil 2009-04-17 16:08:06 EDT
Let's have these three packages (globus-openssl, globus-callout and the updated globus-common) in rawhide. Then I'll go on with other packages.
Comment 15 Mattias Ellert 2009-04-17 16:22:43 EDT
New Package CVS Request
=======================
Package Name: globus-openssl
Short Description: Globus Toolkit - Openssl Library
Owners: ellert
Branches: F-9 F-10 F-11 EL-4 EL-5
InitialCC:
Comment 16 Mattias Ellert 2009-04-17 16:26:40 EDT
(In reply to comment #14)
> Let's have these three packages (globus-openssl, globus-callout and the updated
> globus-common) in rawhide. Then I'll go on with other packages.  

Many thanks for your review comments. I hope that when you do more of the packages it will be "easy" since I updated all the other packages with your comments from the callout library in mind.
Comment 17 Kevin Fenzi 2009-04-17 17:25:05 EDT
cvs done.
Comment 18 Fedora Update System 2009-04-17 22:33:32 EDT
globus-openssl-3.0-1.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/globus-openssl-3.0-1.fc11
Comment 19 Fedora Update System 2009-04-17 22:33:37 EDT
globus-openssl-3.0-1.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/globus-openssl-3.0-1.fc10
Comment 20 Fedora Update System 2009-04-17 22:33:43 EDT
globus-openssl-3.0-1.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/globus-openssl-3.0-1.fc9
Comment 21 Orcan Ogetbil 2009-04-17 23:01:00 EDT
(In reply to comment #16)
> (In reply to comment #14)
> > Let's have these three packages (globus-openssl, globus-callout and the updated
> > globus-common) in rawhide. Then I'll go on with other packages.  
> 
> Many thanks for your review comments. I hope that when you do more of the
> packages it will be "easy" since I updated all the other packages with your
> comments from the callout library in mind.  

No problem. I admit that they look scary in the first sight, but once I got into them, I realized there is not much to worry about. They are all well organized.
Comment 22 Fedora Update System 2009-04-21 20:51:07 EDT
globus-openssl-3.0-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 23 Fedora Update System 2009-04-21 20:51:31 EDT
globus-openssl-3.0-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 24 Fedora Update System 2009-05-09 00:10:21 EDT
globus-openssl-3.0-1.fc11 has been pushed to the Fedora 11 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.