Bug 609728 - Review Request: sparsehash - Extremely memory-efficient C++ hash_map implementation
Summary: Review Request: sparsehash - Extremely memory-efficient C++ hash_map implemen...
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Adam Miller
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 609738
TreeView+ depends on / blocked
 
Reported: 2010-06-30 22:35 UTC by Kalev Lember
Modified: 2014-01-06 13:11 UTC (History)
8 users (show)

Fixed In Version: sparsehash-1.7-3.fc14
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-07-03 10:26:08 UTC
Type: ---
maxamillion: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Kalev Lember 2010-06-30 22:35:12 UTC
Spec URL: http://kalev.fedorapeople.org/sparsehash.spec
SRPM URL: http://kalev.fedorapeople.org/sparsehash-1.7-1.fc14.src.rpm
Description:
The Google SparseHash project contains several C++ template hash-map
implementations with different performance characteristics, including
an implementation that optimizes for space and one that optimizes for
speed.

Comment 1 Chen Lei 2010-07-01 05:48:27 UTC
Two suggestions:
1. move all files too -devel subpackage
2. It'll better to leave this package as noarch, I think test in not very useful for development libs. You can add %check to packages which require sparsehash.

Comment 2 Andrew Himelstieb 2010-07-02 03:15:04 UTC
-Unsponsored reviewer-

[makerpm@localhost SPECS]$ rpmlint -v sparsehash.spec 
sparsehash.spec: I: checking-url http://google-sparsehash.googlecode.com/files/sparsehash-1.7.tar.gz (timeout 10 seconds)
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

* Compiled and Built successfully on 2.6.33.5-124.fc13.x86_64
* No %find_lang macros (none needed)
* All requirements met in https://fedoraproject.org/wiki/Packaging:ReviewGuidelines

Comment 3 Kalev Lember 2010-07-02 21:36:44 UTC
(In reply to comment #1)
> Two suggestions:
> 1. move all files too -devel subpackage

When I was creating the package, I couldn't make up my mind whether to put everything in -devel or not. If you think it's better that way, I'll move all files over to -devel.


> 2. It'll better to leave this package as noarch, I think test in not very
> useful for development libs. You can add %check to packages which require
> sparsehash.    

I respectfully disagree here.

Building the package as noarch means that it's built only once for every primary architecture we have. For F-12, it's only one build for i686, x86_64, ppc, and ppc64. That would mean not running self tests on 3/4 architectures.

If we have a package which comes with self tests, we should try to run them if possible. You are saying these tests should be moved to packages which require sparsehash; I disagree. Every component should test itself if it is possible. And besides that, a large part of sparsehash package consists of self tests; if they aren't necessary why do you think upstream wrote them in the first place?

The package is also less than 0.5 MB, which wouldn't be much of a saving on mirrors either.

Comment 4 Kalev Lember 2010-07-02 21:41:07 UTC
* Sat Jul 03 2010 Kalev Lember <kalev@smartlink.ee> - 1.7-2
- Move all files to -devel (#609728)

Spec URL: http://kalev.fedorapeople.org/sparsehash.spec
SRPM URL: http://kalev.fedorapeople.org/sparsehash-1.7-2.fc14.src.rpm

Comment 5 Adam Miller 2010-07-02 21:58:01 UTC
YES - Package meets naming and packaging guidelines
YES - Spec file matches base package name.
YES - Spec has consistant macro usage.
YES - Meets Packaging Guidelines.
YES - License
YES - License field in spec matches
YES - License file included in package
YES - Spec in American English
YES - Spec is legible.
YES - Sources match upstream md5sum:

NA - Package needs ExcludeArch
YES - BuildRequires correct
NA - Spec handles locales/find_lang
NA - Package is relocatable and has a reason to be.
YES - Package has %defattr and permissions on files is good.
YES - Package has a correct %clean section.
YES - Package has correct buildroot
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
YES - Package is code or permissible content.
NA - Doc subpackage needed/used.
YES - Packages %doc files don't affect runtime.

YES - Headers/static libs in -devel subpackage.
NA - Spec has needed ldconfig in post and postun
NA - .pc files in -devel subpackage/requires pkgconfig
NA - .so files in -devel subpackage.
YES - -devel package Requires: %{name} = %{version}-%{release}
NA - .la files are removed.

NA - Package is a GUI app and has a .desktop file

YES - Package compiles and builds on at least one arch.
YES - Package has no duplicate files in %files.
YES - Package doesn't own any directories other packages own.
YES - Package owns all the directories it creates.
NO - No rpmlint output.--> RPMLINT false W about invalid URL, verified with spectool
- final provides and requires are sane:
(include output of for i in *rpm; do echo $i; rpm -qp --provides $i; echo =; rpm -qp --requires $i; echo; done
manually indented after checking each line.  I also remove the rpmlib junk and anything provided by glibc.)
[16:56:18][adam@turnip][result]+ for i in *rpm; do echo $i; rpm -qp --provides $i; echo =; rpm -qp --requires $i; echo; done
sparsehash-1.7-2.fc13.src.rpm
=
rpmlib(FileDigests) <= 4.6.0-1
rpmlib(CompressedFileNames) <= 3.0.4-1

sparsehash-devel-1.7-2.fc13.x86_64.rpm
sparsehash-devel = 1.7-2.fc13
sparsehash-devel(x86-64) = 1.7-2.fc13
=
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(FileDigests) <= 4.6.0-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rpmlib(PayloadIsXz) <= 5.2-1


SHOULD Items:

YES - Should build in mock.
YES - Should build on all supported archs
NA - Should function as described.
NA - Should have sane scriptlets.
- Should have subpackages require base package with fully versioned depend.
YES - Should have dist tag
YES - Should package latest version
- check for outstanding bugs on package. (For core merge reviews)

No issues found.

APPROVED.

Comment 6 Kalev Lember 2010-07-02 22:03:38 UTC
Thanks for the quick review, Adam!

New Package CVS Request
=======================
Package Name: sparsehash
Short Description: Extremely memory-efficient C++ hash_map implementation
Owners: kalev
Branches: F-13
InitialCC:

Comment 7 Chen Lei 2010-07-03 01:39:16 UTC
(In reply to comment #3)
> (In reply to comment #1)
> > Two suggestions:
> > 1. move all files too -devel subpackage
> 
> When I was creating the package, I couldn't make up my mind whether to put
> everything in -devel or not. If you think it's better that way, I'll move all
> files over to -devel.
> 
It's a convension, see https://bugzilla.redhat.com/show_bug.cgi?id=609788
> 
> > 2. It'll better to leave this package as noarch, I think test in not very
> > useful for development libs. You can add %check to packages which require
> > sparsehash.    
> 
> I respectfully disagree here.
> 
> Building the package as noarch means that it's built only once for every
> primary architecture we have. For F-12, it's only one build for i686, x86_64,
> ppc, and ppc64. That would mean not running self tests on 3/4 architectures.
> 
> If we have a package which comes with self tests, we should try to run them if
> possible. You are saying these tests should be moved to packages which require
> sparsehash; I disagree. Every component should test itself if it is possible.
> And besides that, a large part of sparsehash package consists of self tests; if
> they aren't necessary why do you think upstream wrote them in the first place?
> 
> The package is also less than 0.5 MB, which wouldn't be much of a saving on
> mirrors either.    
You can add noarch to -devel subpackage, the test will still run on all arch.

Comment 8 Chen Lei 2010-07-03 01:49:36 UTC
s/conversion/convention

Comment 9 Jason Tibbitts 2010-07-03 03:28:23 UTC
CVS done (by process-cvs-requests.py).

Comment 10 Kalev Lember 2010-07-03 10:26:08 UTC
(In reply to comment #7)
> You can add noarch to -devel subpackage, the test will still run on all arch.    

Oh, this is really clever: keep main package archful but mark the -devel subpackage (where all files end up in) as noarch. The end result is that the package build + tests run on each separate architecture, but we only get a single, noarch rpm for the mirrors.

Thanks Chen.


I've imported and built the updated package, closing the ticket.

Comment 11 Fedora Update System 2010-07-03 10:34:32 UTC
sparsehash-1.7-3.fc13 has been submitted as an update for Fedora 13.
http://admin.fedoraproject.org/updates/sparsehash-1.7-3.fc13

Comment 12 Rakesh Pandit 2010-07-05 11:06:42 UTC
Added missing 'assigned to'

Comment 13 Fedora Update System 2010-07-06 17:10:04 UTC
sparsehash-1.7-3.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 14 Than Ngo 2013-06-25 09:24:36 UTC
Package Change Request
======================
Package Name: sparsehash
New Branches: el6
Owners: than

Comment 15 Gwyn Ciesla 2013-06-25 11:13:52 UTC
Git done (by process-git-requests).

Comment 16 Björn 'besser82' Esser 2014-01-06 12:59:30 UTC
Package Change Request
======================
Package Name: sparsehash
New Branches: el5
Owners: besser82

Please create the el5-branch based onto the el6-branch.  Many thanks!

Comment 17 Gwyn Ciesla 2014-01-06 13:11:28 UTC
Git done (by process-git-requests).


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