Bug 545046

Summary: Review Request: CVector - ANSI C API for Dynamic Arrays
Product: [Fedora] Fedora Reporter: Takanori MATSUURA <t.matsuu>
Component: Package ReviewAssignee: Susi Lehtola <susi.lehtola>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: 12CC: fedora-package-review, kryzhev, notting, supercyper1, susi.lehtola
Target Milestone: ---Flags: susi.lehtola: fedora-review+
kevin: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: CVector-1.0.3-1.5Aug09.el5 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2010-09-11 03:32:56 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:
Bug Depends On:    
Bug Blocks: 541462, 545047    

Description Takanori MATSUURA 2009-12-07 12:41:13 UTC
Spec URL: http://t-matsuu.sakura.ne.jp/mock/cvector/cvector.spec
SRPM URL: http://t-matsuu.sakura.ne.jp/mock/cvector/cvector-1.0.3-1.20090805.fc12.src.rpm
Description: 
CVector is an ANSI C implementation of dynamic arrays to provide a
crude approximation to the C++ vector class.

Comment 1 Susi Lehtola 2009-12-08 10:53:38 UTC
A few notes:

- you should have a comment about the necessity of the patch in the spec file, for example
 # Make install target libdir multilib compatible
 Patch0: cvector-1.0.3-libdir.patch
I'm not sure why you change the libtool commands. Can you please clarify what is the intent of adding --tag=CC?

- Please fix the tabbing of the subpackages in the spec file.

- You have misspelled "vector" as "vecrot" in the %description of -devel.

- Send the Makefile patch upstream. The makefile also lacks DESTDIR support, which should be included.

Comment 2 Takanori MATSUURA 2009-12-08 14:19:22 UTC
Spec URL: http://t-matsuu.sakura.ne.jp/mock/CVector.spec
SRPM URL: http://t-matsuu.sakura.ne.jp/mock/CVector-1.0.3-2.20090805.fc12.src.rpm

Update based on the comment for another in-parallel reviewing package.

* Rename to CVector to keep upstream package name.
* Move static library to separated subpackage.


Thank you for reviewing, Jussi.

(In reply to comment #1)
> - you should have a comment about the necessity of the patch in the spec file,
> for example
>  # Make install target libdir multilib compatible
>  Patch0: cvector-1.0.3-libdir.patch
> I'm not sure why you change the libtool commands. Can you please clarify what
> is the intent of adding --tag=CC?
 
"--tag=" is now removed because it's not needed for Fedora package.
Other fix is for changing destination directories and implement as a command at the beginning of %build.  Therefore the patch is removed now.


> - You have misspelled "vector" as "vecrot" in the %description of -devel.

Fixed.  Thanks.


> - Send the Makefile patch upstream. The makefile also lacks DESTDIR support,
> which should be included.  

Yes, upstream source lacks DESTDIR support.  If DESTDIR support is included in upstream source, it would be better for our packaging.  But I don't know upstream want it.

Comment 4 Susi Lehtola 2009-12-08 15:29:45 UTC
More comments:

- The Summary, Group and Requires fields of the subpackages are still not tabbed correctly.

- LIBDIR should have no effect in %build, since nothing is installed yet. You can safely change
 make all CC="%{__cc}" CFLAGS="%{optflags}" LIBDIR="%{_libdir}"
to
 make CFLAGS="%{optflags}" %{?_smp_mflags}

where %{?_smp_mflags} enables parallel compilation.

- You are mixing macros: %{buildroot} vs. $RPM_BUILD_ROOT, %{optflags} vs. $RPM_OPT_FLAGS. Choose a style and stick with it.

- You don't have to ship the static library at all - you can just remove it at the end of %install.

- Please do not use wildcards where they are not necessary: change
 %{_libdir}/*.so.*
to
 %{_libdir}/libCVector.so.*
and the same thing with *.so, and
 %{_includedir}/*
to
 %{_includedir}/CVector.h

Comment 5 Takanori MATSUURA 2009-12-09 06:00:58 UTC
Spec URL: http://t-matsuu.sakura.ne.jp/mock/CVector/CVector.spec
SRPM URL: http://t-matsuu.sakura.ne.jp/mock/CVector/CVector-1.0.3-3.20090805.fc12.src.rpm  

(In reply to comment #4)
> - The Summary, Group and Requires fields of the subpackages are still not
> tabbed correctly.

It's not clear for me.
Does the latest spec file refrain what you mean?


> - LIBDIR should have no effect in %build, since nothing is installed yet. You
> can safely change
>  make all CC="%{__cc}" CFLAGS="%{optflags}" LIBDIR="%{_libdir}"
> to
>  make CFLAGS="%{optflags}" %{?_smp_mflags}
> 
> where %{?_smp_mflags} enables parallel compilation.

Fixed.


> - You are mixing macros: %{buildroot} vs. $RPM_BUILD_ROOT, %{optflags} vs.
> $RPM_OPT_FLAGS. Choose a style and stick with it.

I choose "macros" policy.


> - You don't have to ship the static library at all - you can just remove it at
> the end of %install.

This package may be used by scientific programmer which have the computer with huge memory area.

They sometimes link libraries as static for the "performance" with their own risk.  Other scientific libraries such as blas/lapack or fftw also have static libraries in their package.  So I suppose there is no problem that I pack static libraries as a separate subpackage.


> - Please do not use wildcards where they are not necessary: change
>  %{_libdir}/*.so.*
> to
>  %{_libdir}/libCVector.so.*
> and the same thing with *.so, and
>  %{_includedir}/*
> to
>  %{_includedir}/CVector.h  

Fixed.


Thank you for continuous reviewing.

Comment 6 Susi Lehtola 2009-12-09 10:47:51 UTC
(In reply to comment #5)
> (In reply to comment #4)
> > - The Summary, Group and Requires fields of the subpackages are still not
> > tabbed correctly.
> 
> It's not clear for me.
> Does the latest spec file refrain what you mean?

Perfect.

> > - You don't have to ship the static library at all - you can just remove it at
> > the end of %install.
> 
> This package may be used by scientific programmer which have the computer with
> huge memory area.
> 
> They sometimes link libraries as static for the "performance" with their own
> risk.  Other scientific libraries such as blas/lapack or fftw also have static
> libraries in their package.  So I suppose there is no problem that I pack
> static libraries as a separate subpackage.

Uhm, I don't think the overhead of dynamic linkage is THAT great. Dynamic memory allocation, on the other hand, is slow.

Static linking is most helpful a) on systems that don't support dynamic linking and b) if you want to run the same binary on a lot of different systems without having to recompile it.

http://fedoraproject.org/wiki/Packaging/Guidelines#Packaging_Static_Libraries
"Packages including libraries should exclude static libs as far as possible (eg by configuring with --disable-static). Static libraries should only be included in exceptional circumstances. Applications linking against libraries should as far as possible link against shared libraries not static versions."

If you *really* think having the static library is necessary, then it's OK to ship it. I wouldn't, though.

**

An aesthetical comment: you don't need to put so much empty lines within sections in the spec file. A few lines between sections (such as %build and %install) is enough, when there's nothing long and complicated happening. %post and %postun are also usually grouped together for symmetry.

**

I am willing to sponsor you if you show me your knowing of the Fedora guidelines, most importantly
 http://fedoraproject.org/wiki/Packaging/Guidelines
 http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
Additionally to the Packaging Guidelines, there are a bunch of language / application specific guidelines that are linked to in the Packaging Guidelines.

Here are some tricks of the trade:
http://fedoraproject.org/wiki/Packaging_tricks
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
http://fedoraproject.org/wiki/Common_Rpmlint_issues

I will sponsor you if you have at least one other submission and perform a couple of informal reviews of packages of other people.

Please review only packages *not* marked with FE-NEEDSPONSOR. I will have to do the full formal review after you to check that you have got everything correctly. Once I have sponsored you you will be able to do formal reviews of your own.

Comment 7 Takanori MATSUURA 2009-12-09 11:52:48 UTC
(In reply to comment #6)
> Uhm, I don't think the overhead of dynamic linkage is THAT great. Dynamic
> memory allocation, on the other hand, is slow.
> 
> Static linking is most helpful a) on systems that don't support dynamic linking
> and b) if you want to run the same binary on a lot of different systems without
> having to recompile it.
> 
> http://fedoraproject.org/wiki/Packaging/Guidelines#Packaging_Static_Libraries
> "Packages including libraries should exclude static libs as far as possible (eg
> by configuring with --disable-static). Static libraries should only be included
> in exceptional circumstances. Applications linking against libraries should as
> far as possible link against shared libraries not static versions."
> 
> If you *really* think having the static library is necessary, then it's OK to
> ship it. I wouldn't, though.

Yes, I have read Guideline and keep it.
Hmm...
OK. static library is now removed.


> An aesthetical comment: you don't need to put so much empty lines within
> sections in the spec file. A few lines between sections (such as %build and
> %install) is enough, when there's nothing long and complicated happening. %post
> and %postun are also usually grouped together for symmetry.

Thanks.
I now get that the second empty line which is generated by rpm-spec mode in emacs or /etc/rpmdevtools/*-template.spec means "add more command or so".


> I am willing to sponsor you if you show me your knowing of the Fedora
> guidelines, most importantly
>  http://fedoraproject.org/wiki/Packaging/Guidelines
>  http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
> Additionally to the Packaging Guidelines, there are a bunch of language /
> application specific guidelines that are linked to in the Packaging Guidelines.
> 
> Here are some tricks of the trade:
> http://fedoraproject.org/wiki/Packaging_tricks
> http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
> http://fedoraproject.org/wiki/Common_Rpmlint_issues

Thanks a lot for the information and expressing sponsorship.
I'm collaborating with Jindrich (a maintainer of texlive packages) for adding Japanese TeX support into TeXLive (and formally teTeX) for many years.  And this process is a good time for me to re-check my packaging skill.


> I will sponsor you if you have at least one other submission and perform a
> couple of informal reviews of packages of other people.
> 
> Please review only packages *not* marked with FE-NEEDSPONSOR. I will have to do
> the full formal review after you to check that you have got everything
> correctly. Once I have sponsored you you will be able to do formal reviews of
> your own.  

Okay. I'll try.
Do I have to select the review request which don't have any reviewer comments yet?

Comment 8 Susi Lehtola 2009-12-09 12:06:11 UTC
(In reply to comment #7)
> > Please review only packages *not* marked with FE-NEEDSPONSOR. I will have to do
> > the full formal review after you to check that you have got everything
> > correctly. Once I have sponsored you you will be able to do formal reviews of
> > your own.  
> 
> Okay. I'll try.
> Do I have to select the review request which don't have any reviewer comments
> yet?  

Yes, please.

Comment 9 Takanori MATSUURA 2010-05-06 04:14:07 UTC
(In reply to comment #8)
I've selected bug 587315 as informal review.

Comment 10 Susi Lehtola 2010-05-06 05:33:07 UTC
Well, that took you a while..

Anyway, you will still need to do another submission and another informal review.

Comment 11 Takanori MATSUURA 2010-06-01 09:36:58 UTC
I'll try bug 597307 for informal review.

Comment 12 Takanori MATSUURA 2010-06-02 05:37:41 UTC
(In reply to comment #11)
> I'll try bug 597307 for informal review.
and bug 598511.

Comment 13 Mamoru TASAKA 2010-08-24 15:48:14 UTC
So Jussi, would you make it clear if you are going to
sponsor Matsuura-san or if you are asking him to do
some other things before sponsoring him?

Comment 14 Susi Lehtola 2010-08-24 15:55:30 UTC
Oh, I'm terribly sorry - this one totally slipped under my radar. Review coming right up.

Comment 15 Susi Lehtola 2010-08-24 17:28:35 UTC
rpmlint output:
CVector-devel.x86_64: W: no-documentation
4 packages and 0 specfiles checked; 0 errors, 1 warnings.

This is OK.


- Normally it's better to use a patch than sed, since sed can fail silently. YMMV.

- There are test binaries that you could run in the build.
 %check
 make tests
although "make install" already seems to run them.


MUST: The package does not yet exist in Fedora. The Review Request is not a duplicate. OK
MUST: The spec file for the package is legible and macros are used consistently. OK

MUST: The package must be named according to the Package Naming Guidelines. ~OK
- You should keep the original naming scheme. This means
 Release:        4.20090805%{?dist}
should be
 Release:        4.5Aug09%{?dist}
and so on. So the first number is the release number, the second is the date from the tarball.

MUST: The spec file name must match the base package %{name}. OK
MUST: The package must be licensed with a Fedora approved license and meet the  Licensing Guidelines. OK
MUST: The License field in the package spec file must match the actual license. OK
MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. OK
00d4399e74d9c7bc6e0bb72804990c3e  CVector-1.0.3-5Aug09.tar.gz
00d4399e74d9c7bc6e0bb72804990c3e  ../SOURCES/CVector-1.0.3-5Aug09.tar.gz

MUST: The package MUST successfully compile and build into binary rpms. OK
MUST: The spec file MUST handle locales properly. N/A
MUST: Optflags are used and time stamps preserved. OK
MUST: Packages containing shared library files must call ldconfig. OK
MUST: A package must own all directories that it creates or require the package that owns the directory. OK
MUST: Files only listed once in %files listings. OK
MUST: Debuginfo package is complete. OK
MUST: Permissions on files must be set properly. OK
MUST: Large documentation files must go in a -doc subpackage. N/A
MUST: All relevant items are included in %doc. Items in %doc do not affect runtime of application. OK
MUST: Header files must be in a -devel package. OK
MUST: Static libraries must be in a -static package. N/A (removed)
MUST: If a package contains library files with a suffix then library files ending in .so must go in a -devel package. OK
MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency. OK
MUST: Packages does not contain any .la libtool archives. OK
MUST: Desktop files are installed properly. N/A
MUST: No file conflicts with other packages and no general names. OK
SHOULD: %{?dist} tag is used in release. OK
SHOULD: If the package does not include license text(s) as separate files from upstream, the packager should query upstream to include it. OK
SHOULD: The package builds in mock. OK
EPEL: Clean section exists. OK
EPEL: Buildroot cleaned before install. OK
EPEL: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'. N/A

Comment 16 Susi Lehtola 2010-08-24 17:36:08 UTC
Please change the versioning scheme before import to git. This package has been

APPROVED.


Also, I've just sponsored you in FAS. You seem to have a pretty good hang on packaging. We are short on reviewers, so you're most warmly invited to take on formal reviews of your own. Just pay a bit more attention to the exact words of the licenses, and the honoring of build flags.

Reviewing is a one-time task, whereas maintaining a package is an ongoing process. Reviewing is the way of least resistance to improve Fedora to your liking!

If you have any problems feel free to ask me for help.

Comment 17 Chen Lei 2010-08-25 06:21:35 UTC
sed -e 's,-dynamic,-rdynamic,g' -i Makefile is useless at all, (grep dynamic Makefile) find nothing.

Comment 18 Takanori MATSUURA 2010-08-30 10:09:56 UTC
New Package SCM Request
=======================
Package Name: CVector
Short Description: ANSI C API for Dynamic Arrays
Owners: tmatsuu
Branches: f12 f13 f14 el5 el6
InitialCC:


(In reply to comment #15)
Versioning and "patch rather than sed" issues are fixed.

(In reply to comment #17)
Useless sed command has been removed.

Comment 19 Kevin Fenzi 2010-08-30 17:29:45 UTC
Git done (by process-git-requests).

Comment 20 Fedora Update System 2010-09-01 10:05:15 UTC
CVector-1.0.3-1.5Aug09.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/CVector-1.0.3-1.5Aug09.fc14

Comment 21 Fedora Update System 2010-09-01 10:08:49 UTC
CVector-1.0.3-1.5Aug09.fc12 has been submitted as an update for Fedora 12.
https://admin.fedoraproject.org/updates/CVector-1.0.3-1.5Aug09.fc12

Comment 22 Fedora Update System 2010-09-01 10:10:10 UTC
CVector-1.0.3-1.5Aug09.el5 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/CVector-1.0.3-1.5Aug09.el5

Comment 23 Fedora Update System 2010-09-01 10:11:40 UTC
CVector-1.0.3-1.5Aug09.fc13 has been submitted as an update for Fedora 13.
https://admin.fedoraproject.org/updates/CVector-1.0.3-1.5Aug09.fc13

Comment 24 Fedora Update System 2010-09-02 02:28:18 UTC
CVector-1.0.3-1.5Aug09.el5 has been pushed to the Fedora EPEL 5 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update CVector'.  You can provide feedback for this update here: https://admin.fedoraproject.org/updates/CVector-1.0.3-1.5Aug09.el5

Comment 25 Fedora Update System 2010-09-11 03:32:49 UTC
CVector-1.0.3-1.5Aug09.fc14 has been pushed to the Fedora 14 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 26 Fedora Update System 2010-09-11 08:58:33 UTC
CVector-1.0.3-1.5Aug09.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 27 Fedora Update System 2010-09-11 09:03:08 UTC
CVector-1.0.3-1.5Aug09.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 28 Fedora Update System 2010-09-16 16:30:51 UTC
CVector-1.0.3-1.5Aug09.el5 has been pushed to the Fedora EPEL 5 stable repository.  If problems still persist, please make note of it in this bug report.