Bug 487421

Summary: Review Request: libyaml - YAML 1.1 parser and emitter written in C
Product: [Fedora] Fedora Reporter: John Eckersberg <jeckersb>
Component: Package ReviewAssignee: Michael DeHaan <mdehaan>
Status: CLOSED CURRENTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: fedora-package-review, mdehaan, notting, panemade, rc040203
Target Milestone: ---Flags: mdehaan: fedora-review+
kevin: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2009-03-23 12:31:37 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:

Description John Eckersberg 2009-02-25 21:43:26 UTC
Spec URL: http://jeckersb.fedorapeople.org/libyaml/libyaml.spec
SRPM URL: http://jeckersb.fedorapeople.org/libyaml/libyaml-0.1.2-1.fc10.src.rpm
Description:

This package is needed as a build requirement for PyYAML's C bindings (I also maintain PyYAML).

YAML is a data serialization format designed for human readability and
interaction with scripting languages.  LibYAML is a YAML parser and
emitter written in C.

Comment 1 Parag AN(पराग) 2009-02-26 04:03:35 UTC
You can drop files README LICENSE as %doc from -devel as libyaml already installs them.

Any reason to include -static package?

Comment 2 John Eckersberg 2009-02-26 13:15:23 UTC
I modelled my spec after libxml2, which split static out into it's own package.  It really doesn't matter to me, so I'll just move it into the -devel package.

Updated:

Spec URL: http://jeckersb.fedorapeople.org/libyaml/libyaml.spec
SRPM URL: http://jeckersb.fedorapeople.org/libyaml/libyaml-0.1.2-2.fc10.src.rpm

Thanks for looking at this!

Comment 3 Michael DeHaan 2009-03-02 15:45:00 UTC
rpmlint is exceptionally clean, 0 errors, 0 warnings.

Commencing manual review checklist.

Comment 4 Michael DeHaan 2009-03-02 15:58:50 UTC
The following items were listed as "musts" and all pass:

+ rpmlint ok
+ package name ok
+ spec name ok
+ guidelines ok
+ license: ok, matched
+ license in %doc, ok
+ specfile is English
+ specfile is legible

(TARBALL MUST MATCH UPSTREAM)

+ compiles/builds ok on my i686 install, satisfying the "at least one primary arch rule"
+ no ExcludeArch required
+ no BuildRequires required, basic C library
+ no use of locales required, basic C library
+ calls ldconfig in %post and %postun as required
+ not relocatable
+ owns all directories since it creates zero of them
+ no duplicate files
+ defattr is correct
+ contains proper clean section
+ macro usage ok
+ package is code, not content, ok
+ no large docs, ok
+ docs do not affect runtime, ok
+ devel package created, ok
+ static package created, ok
+ pkgconfig N/A, ok
+ libraries ok
+ devel requires base as N-V-R, ok
+ no libtool archives, ok
+ no GUI apps, ok
+ no reownership, ok
+ cleans buildroot at install, ok
+ filenames are valid UTF-8, ok

All MUST items has passed, now reviewing SHOULD items.

Comment 5 Michael DeHaan 2009-03-02 16:01:38 UTC
SHOULD items:

+ does include LICENSE, good.
+ translations of description not included, ok.
+ mock test skipped
+ all arch builds skipped, build system can figure this out, simple C library
+ package DOES function as described, ok
+ no magic scriptlets, good
+ subpackage requirements are fine
+ no .pc files, fine
+ no dependency problems, fine

All SHOULD items are passed to my satisfaction, and were optional anyway.

Comment 6 Michael DeHaan 2009-03-02 16:05:27 UTC
This review hereby passes.

Comment 7 John Eckersberg 2009-03-02 16:16:47 UTC
New Package CVS Request
=======================
Package Name: libyaml
Short Description: LibYAML is a YAML parser and emitter written in C.
Owners: jeckersb
Branches: F-9 F-10 EL-4 EL-5
InitialCC:

Comment 8 Kevin Fenzi 2009-03-03 00:30:42 UTC
cvs done.

Comment 9 Parag AN(पराग) 2009-03-03 02:05:40 UTC
(In reply to comment #2)
> I modelled my spec after libxml2, which split static out into it's own package.
>  It really doesn't matter to me, so I'll just move it into the -devel package.
> 
> Updated:
> 
> Spec URL: http://jeckersb.fedorapeople.org/libyaml/libyaml.spec
> SRPM URL: http://jeckersb.fedorapeople.org/libyaml/libyaml-0.1.2-2.fc10.src.rpm
> 
> Thanks for looking at this!

I mean to say, static libraries generally are not allowed to be packaged. see http://fedoraproject.org/wiki/Packaging/Guidelines#StaticLibraries

I think this package is not following http://fedoraproject.org/wiki/Packaging/Guidelines#Packaging_Static_Libraries_2 guidelines.

I guess you need virtual provides.

Comment 10 Michael DeHaan 2009-03-03 03:53:44 UTC
This package has already passed review, so if you want you can bring this up in a seperate defect.  This all being said, correctness is good, so I'll quote from the page as I read it:

"""
    *  In general, packagers are strongly encouraged not to ship static libs unless a compelling reason exists. 
"""

The word was encouraged, not required.  It also says 

"""
There are two scenarios in which static libraries are packaged:
"""

This package follows scenario 1 of 2:

"""
   1. Static libraries and shared libraries. In this case, the static libraries must be placed in a *-static subpackage.
"""

This is what the RPM does.  I would also say that when doing the final spec, John could, if he wanted, choose to not build the static package and everything would be ok, it is not highly important to have the static package there.

Comment 11 Parag AN(पराग) 2009-03-03 04:40:42 UTC
(In reply to comment #10)
> This package has already passed review, so if you want you can bring this up in
> a seperate defect.  This all being said, correctness is good, so I'll quote
> from the page as I read it:
 sure I will.

> 
> """
>     *  In general, packagers are strongly encouraged not to ship static libs
> unless a compelling reason exists. 
> """
> 
> The word was encouraged, not required.  It also says 
>
  So I was interested to know that "compelling reason" which made static libraries to be shipped.

> """
> There are two scenarios in which static libraries are packaged:
> """
> 
> This package follows scenario 1 of 2:
> 
> """
>    1. Static libraries and shared libraries. In this case, the static libraries
> must be placed in a *-static subpackage.
> """
> 
> This is what the RPM does.  I would also say that when doing the final spec,
> John could, if he wanted, choose to not build the static package and everything
> would be ok, it is not highly important to have the static package there.

So you are asking to remove .a files from -devel?

Comment 12 John Eckersberg 2009-03-03 19:00:48 UTC
I have pulled the .a file out of the -devel package and rebuilt.

Comment 13 Ralf Corsepius 2009-03-04 03:01:11 UTC
(In reply to comment #12)
> I have pulled the .a file out of the -devel package and rebuilt.
Better use:

%configure --disable-static

Comment 14 Parag AN(पराग) 2009-03-23 04:57:38 UTC
If this package is built for all requested branches then can we have this review closed now?

Comment 15 John Eckersberg 2009-03-23 12:31:37 UTC
Yes certainly, I thought I had already closed it.  Closing it now.  Thanks for all your help!