Bug 545046
Summary: | Review Request: CVector - ANSI C API for Dynamic Arrays | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Takanori MATSUURA <t.matsuu> |
Component: | Package Review | Assignee: | Susi Lehtola <susi.lehtola> |
Status: | CLOSED ERRATA | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | low | ||
Version: | 12 | CC: | 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
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. 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. (In reply to comment #2) > 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 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-2.20090805.fc12.src.rpm 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 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. (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. (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? (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. (In reply to comment #8) I've selected bug 587315 as informal review. Well, that took you a while.. Anyway, you will still need to do another submission and another informal review. I'll try bug 597307 for informal review. (In reply to comment #11) > I'll try bug 597307 for informal review. and bug 598511. 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? Oh, I'm terribly sorry - this one totally slipped under my radar. Review coming right up. 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 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. sed -e 's,-dynamic,-rdynamic,g' -i Makefile is useless at all, (grep dynamic Makefile) find nothing. 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. Git done (by process-git-requests). 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 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 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 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 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 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. 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. 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. 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. |