Bug 487421
Summary: | Review Request: libyaml - YAML 1.1 parser and emitter written in C | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | John Eckersberg <jeckersb> |
Component: | Package Review | Assignee: | Michael DeHaan <mdehaan> |
Status: | CLOSED CURRENTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | 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
You can drop files README LICENSE as %doc from -devel as libyaml already installs them. Any reason to include -static package? 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! rpmlint is exceptionally clean, 0 errors, 0 warnings. Commencing manual review checklist. 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. 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. This review hereby passes. 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: cvs done. (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. 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. (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? I have pulled the .a file out of the -devel package and rebuilt. (In reply to comment #12) > I have pulled the .a file out of the -devel package and rebuilt. Better use: %configure --disable-static If this package is built for all requested branches then can we have this review closed now? Yes certainly, I thought I had already closed it. Closing it now. Thanks for all your help! |