Bug 663244 - Review Request: CUnit - A unit testing framework for C
Summary: Review Request: CUnit - A unit testing framework for C
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Martin Gieseking
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Keywords:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-12-15 05:31 UTC by Shakthi Kannan
Modified: 2011-04-08 20:30 UTC (History)
7 users (show)

(edit)
Clone Of:
(edit)
Last Closed: 2011-02-08 22:54:07 UTC
martin.gieseking: fedora-review+
dennis: fedora-cvs+


Attachments (Terms of Use)

Description Shakthi Kannan 2010-12-15 05:31:44 UTC
Spec URL: http://shakthimaan.fedorapeople.org/SPECS/CUnit.spec
SRPM URL: http://shakthimaan.fedorapeople.org/SRPMS/CUnit-2.1_2-1.fc14.src.rpm
Description: CUnit is a lightweight system for writing, administering,
and running unit tests in C.  It provides C programmers a basic
testing functionality with a flexible variety of user interfaces.

Comment 1 Susi Lehtola 2010-12-15 08:02:08 UTC
A few notes:

Please use
 --datarootdir=%{_datadir}
instead of
 --datarootdir=/usr/share/
and %{_datadir} in place of %{_datarootdir} in %files, just to stick to convention.

Is there any reason why SMP make is not used?

Comment 2 Shakthi Kannan 2010-12-15 16:09:05 UTC
* Updated datarootdir, datadir macros.
* Updated postun, post ldconfig instructions.
* Added smp flags for make.

SPEC: http://shakthimaan.fedorapeople.org/SPECS/CUnit.spec
SRPM: http://shakthimaan.fedorapeople.org/SRPMS/CUnit-2.1_2-2.fc14.src.rpm

Successful Koji builds for F-13, F-14 and EL-6:

http://koji.fedoraproject.org/koji/taskinfo?taskID=2666721
http://koji.fedoraproject.org/koji/taskinfo?taskID=2666726
http://koji.fedoraproject.org/koji/taskinfo?taskID=2666731

$ rpmlint CUnit.spec
CUnit.spec: W: invalid-url Source0: http://sourceforge.net/projects/cunit/files/CUnit/2.1-2/CUnit-2.1-2-src.tar.bz2 <urlopen error timed out>
0 packages and 1 specfiles checked; 0 errors, 1 warnings.

$ rpmlint CUnit-2.1_2-2.fc14.src.rpm 

CUnit.src: W: invalid-url Source0: http://sourceforge.net/projects/cunit/files/CUnit/2.1-2/CUnit-2.1-2-src.tar.bz2 <urlopen error timed out>
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

$ rpmlint CUnit-2.1_2-2.fc14.i686.rpm 
CUnit.i686: W: shared-lib-calls-exit /usr/lib/libcunit.so.1.0.1 exit@GLIBC_2.0
CUnit.i686: W: manual-page-warning /usr/share/man/man3/CUnit.3.gz 476: warning: macro `TP5' not defined (possibly missing space after `TP')
CUnit.i686: W: manual-page-warning /usr/share/man/man3/CUnit.3.gz 484: warning: macro `BpTest->pName' not defined
CUnit.i686: W: manual-page-warning /usr/share/man/man3/CUnit.3.gz 521: warning: macro `TP5' not defined (possibly missing space after `TP')
CUnit.i686: W: manual-page-warning /usr/share/man/man3/CUnit.3.gz 523: warning: macro `TP5' not defined (possibly missing space after `TP')
CUnit.i686: W: manual-page-warning /usr/share/man/man3/CUnit.3.gz 525: warning: macro `TP5' not defined (possibly missing space after `TP')
CUnit.i686: W: manual-page-warning /usr/share/man/man3/CUnit.3.gz 527: warning: macro `TP5' not defined (possibly missing space after `TP')
CUnit.i686: W: manual-page-warning /usr/share/man/man3/CUnit.3.gz 542: warning: macro `TP5' not defined (possibly missing space after `TP')
CUnit.i686: W: manual-page-warning /usr/share/man/man3/CUnit.3.gz 544: warning: macro `TP5' not defined (possibly missing space after `TP')
CUnit.i686: W: manual-page-warning /usr/share/man/man3/CUnit.3.gz 546: warning: macro `TP5' not defined (possibly missing space after `TP')
CUnit.i686: W: manual-page-warning /usr/share/man/man3/CUnit.3.gz 548: warning: macro `TP5' not defined (possibly missing space after `TP')
1 packages and 0 specfiles checked; 0 errors, 11 warnings.

$ rpmlint CUnit-devel-2.1_2-2.fc14.i686.rpm 
CUnit-devel.i686: W: no-documentation
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

Comment 3 Michael Schwendt 2010-12-25 08:48:20 UTC
> Please use
>  --datarootdir=%{_datadir}
> instead of
> --datarootdir=/usr/share/
> and %{_datadir} in place of %{_datarootdir} in %files, just to stick to
> convention.

Why set --datarootdir at all?

Its default is ${prefix}/share (also see rpm --eval %_datarootdir). ${datadir} is set to ${datarootdir}, and lots of other autoconf variables are derived from that value. And %configure already sets --datadir=/usr/share, too.

Then, where you use a fragile sed pattern to replace "prefix" with "datarootdir", you could use the default datadir instead. Or even better, replace the sed substitution (which may fail silently or cause damage) with a clean patch that would fail to apply if the patch target has changed.

Comment 4 Shakthi Kannan 2010-12-25 13:50:55 UTC
---
| Why set --datarootdir at all?
\--

Without it in doc/Makefile, you get:

docdir = $(prefix)/doc/CUnit
prefix = /usr

and doc gets installed in /usr/doc/CUnit which is not what we want.

Comment 5 Shakthi Kannan 2010-12-26 06:17:22 UTC
* Removed use of sed by adding a patch to fix docdir path.
* Added patch to remove exit calls from library.

SPEC: http://shakthimaan.fedorapeople.org/SPECS/CUnit.spec
SRPM: http://shakthimaan.fedorapeople.org/SRPMS/CUnit-2.1_2-3.fc14.src.rpm

Successful Koji builds for F-13, F-14 and EL-6 respectively:

http://koji.fedoraproject.org/koji/taskinfo?taskID=2689210
http://koji.fedoraproject.org/koji/taskinfo?taskID=2689194
http://koji.fedoraproject.org/koji/taskinfo?taskID=2689205

Comment 6 Martin Gieseking 2011-01-19 11:10:03 UTC
Hi Shakthi,

here are some notes on your latest spec:

- the license of CUnit is LGPLv2+

- BUILDROOT => BuildRoot

- add short comments above Patch0 and Patch1 telling what the patches do

- Don't change the behavior of the library. If a library calls exit() you 
  should ask upstream to fix it but don't remove the function call with a 
  local patch

- adapt Source0 according to
  http://fedoraproject.org/wiki/Packaging/SourceURL#Sourceforge.net

- you should use macros consistently (don't mix rm and %{__rm} for example)
  I recommend to prefer the plain commands (rm, make etc.) and not to use 
  macros here

- the man3 manpage contains the API docs and thus belongs to the devel package

- the html documentation should also go to the devel package

- add AUTHORS, COPYING, README, TODO to the base package (with %doc)

- replace 
    %{_libdir}/libcunit.so.1
    %{_libdir}/libcunit.so.1.0.1
  with
    %{_libdir}/libcunit.so.*
  to simplify future updates

- replace CUnit.3.gz with CUnit.3*

Comment 7 Shakthi Kannan 2011-01-19 23:54:40 UTC
Thanks for your wondeful feedback!

- Updated to license LGPLv2+. I checked the COPYING file for LGPLv2 and hence had used it. The code mentions also higher versions.
- Changed to use BuildRoot.
- Added comments for inclusion of patches.
- Removed inconsistent macro usage.
- Moved man page, HTML documentation to devel package.
- Added AUTHORS, COPYING, README, TODO to doc in base package.
- Used * in man, library inclusion.

I haven't changed Source0 because I get a 404 error if I use Source0 as:

  http://downloads.sourceforge.net/%{name}/%{name}-2.1-2-src.tar.bz2

I have pushed the patches upstream for inclusion in the project repo, and retaining it in the .spec file since rpmlint warns about calling exit() function in a library.

SPEC: http://shakthimaan.fedorapeople.org/SPECS/CUnit.spec
SRPM: http://shakthimaan.fedorapeople.org/SRPMS/CUnit-2.1_2-4.fc14.src.rpm

$ rpmlint CUnit.spec
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

$ rpmlint CUnit-2.1_2-4.fc14.i686.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

$ rpmlint CUnit-devel-2.1_2-4.fc14.i686.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

Successful Koji builds for F-13, F-14, EL-6 respectively:
http://koji.fedoraproject.org/koji/taskinfo?taskID=2732327
http://koji.fedoraproject.org/koji/taskinfo?taskID=2732331
http://koji.fedoraproject.org/koji/taskinfo?taskID=2732336

Comment 8 Shakthi Kannan 2011-01-19 23:56:11 UTC
$ rpmlint ../SRPMS/CUnit-2.1_2-4.fc14.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

Comment 9 Martin Gieseking 2011-01-20 09:30:43 UTC
(In reply to comment #7)
> Thanks for your wondeful feedback!

You're welcome. :)

> I haven't changed Source0 because I get a 404 error if I use Source0 as: 
>   http://downloads.sourceforge.net/%{name}/%{name}-2.1-2-src.tar.bz2

This is because upstream doesn't use camel case in the sourceforge project name.
http://downloads.sourceforge.net/cunit/%{name}-2.1-2-src.tar.bz2
should work.

 
> I have pushed the patches upstream for inclusion in the project repo, and
> retaining it in the .spec file since rpmlint warns about calling exit()
> function in a library.

OK, but please don't apply the exit() patch. The rpmlint warning should encourage you to convince the upstream developer to fix the issue. But don't do it yourself with a local patch because it changes the behavior of the library.

I also suggest to ask upstream what's the intention of the dash in the version number. Does the versioning scheme always follow pattern A.B-C or could there also be an A.B.C-D release? If A.B-C is guaranteed, I recommend to replace the dash with a dot in the Fedora package (rather than an underscore). Maybe upstream should also think about using only dots as separators.

Comment 10 Shakthi Kannan 2011-01-20 17:15:47 UTC
I haven't got a reply from upstream yet.

- Updated Source0 to use Fedora sourceforge.net URL.
- Removed exit from library patch.
- Used A.B.C version.

SPEC: http://shakthimaan.fedorapeople.org/SPECS/CUnit.spec
SRPM: http://shakthimaan.fedorapeople.org/SRPMS/CUnit-2.1.2-5.fc14.src.rpm

$ rpmlint CUnit.spec
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

$ rpmlint CUnit-2.1.2-5.fc14.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

$ rpmlint CUnit-2.1.2-5.fc14.i686.rpm 
CUnit.i686: W: shared-lib-calls-exit /usr/lib/libcunit.so.1.0.1 exit@GLIBC_2.0
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

$ rpmlint CUnit-devel-2.1.2-5.fc14.i686.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

Successful Koji builds for F-14 and EL-6 respectively:
http://koji.fedoraproject.org/koji/taskinfo?taskID=2733597
http://koji.fedoraproject.org/koji/taskinfo?taskID=2733600

Comment 11 Martin Gieseking 2011-01-29 16:18:22 UTC
The package is almost ready. Here are a few things you should consider:

- The Group of the base package should be "System Environment/Libraries".

- Some of the C files have executable permissions. Please get rid of them in
  %prep, e.g. with
  find -name *.c -exec chmod -x {} \;

- Please provide two separate patches: one for the docdir stuff and one for 
  the manpage. That way it's easier to deal with future upstream updates.

- As Michael already mentioned, you can drop --datarootdir=%{_datadir} from
  %configure because %{_datarootdir} = %{_datadir} = /usr/share

$ rpmlint /var/lib/mock/fedora-14-x86_64/result/*.rpm
CUnit.x86_64: W: shared-lib-calls-exit /usr/lib64/libcunit.so.1.0.1 exit@GLIBC_2.2.5
CUnit-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/CUnit-2.1-2/CUnit/Sources/Framework/Util.c
CUnit-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/CUnit-2.1-2/CUnit/Sources/Framework/TestRun.c
CUnit-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/CUnit-2.1-2/CUnit/Sources/Console/Console.c
CUnit-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/CUnit-2.1-2/CUnit/Sources/Automated/Automated.c
CUnit-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/CUnit-2.1-2/CUnit/Sources/Framework/TestDB.c
4 packages and 0 specfiles checked; 0 errors, 6 warnings.


---------------------------------
key:

[+] OK
[.] OK, not applicable
[X] needs work
---------------------------------

[+] MUST: The package must be named according to the Package Naming Guidelines.
[+] MUST: The spec file name must match the base package %{name}.
[+] MUST: The package must meet the Packaging Guidelines.
[+] MUST: The package must be licensed with a Fedora approved license.
    - LGPLv2+

[+] MUST: The License field in the package spec file must match the actual license.
[+] MUST: The file containing the text of the license(s) for the package must be included in %doc.
[+] MUST: The spec file must be written in American English.
[+] MUST: The spec file for the package MUST be legible.
[+] MUST: The sources used to build the package must match the upstream source.
    $ md5sum CUnit-2.1-2-src.tar.bz2*
    31c62bd7a65007737ba28b7aafc44d3a  CUnit-2.1-2-src.tar.bz2
    31c62bd7a65007737ba28b7aafc44d3a  CUnit-2.1-2-src.tar.bz2.1

[+] MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture.
[.] MUST: If the package does not successfully compile, build or work on an architecture, ...
[+] MUST: All build dependencies must be listed in BuildRequires.
[+] MUST: When compiling C, C++, or Fortran files, %{optflags} must be applied.
[.] MUST: The spec file MUST handle locales properly.
[.] MUST: If a package installs files below %{_datadir}/icons, the icon cache must be updated.
[+] MUST: Packages storing shared library files (not just symlinks) must call ldconfig in %post and %postun.
[+] MUST: Packages must NOT bundle copies of system libraries.
[.] MUST: If the package is designed to be relocatable, ...
[+] MUST: A package must own all directories that it creates. 
[+] MUST: A Fedora package must not list a file more than once in %files.
[X] MUST: Permissions on files must be set properly.
    - please fix the file permissions of the .c files

[.] MUST: Packages must not provide RPM dependency information when that information is not global in nature, or are otherwise handled.
[.] MUST: When filtering automatically generated RPM dependency information, the filtering system implemented by Fedora must be used.
[+] MUST: Each package must consistently use macros.
[+] MUST: The package must contain code, or permissable content.
[.] MUST: Large documentation files must go in a -doc subpackage.
[+] MUST: Files in %doc must not affect the runtime of the application.
[+] MUST: Header files must be in a -devel package.
[.] MUST: Static libraries must be in a -static package.
[+] MUST: library files that end in .so (without suffix) must go in a -devel package.
[+] MUST: devel packages must require the base package using a fully versioned dependency.
[+] MUST: Packages must NOT contain any .la libtool archives.
[.] MUST: Packages containing GUI applications must include a %{name}.desktop file.
[+] MUST: Packages must not own files or directories already owned by other packages.
[+] MUST: All filenames in rpm packages must be valid UTF-8.

EPEL <= 5 only:
[+] MUST: The spec file must contain a valid BuildRoot field.
[+] MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot}.
[+] MUST: Each package must have a %clean section, which contains rm -rf %{buildroot}.
[+] MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'

[.] SHOULD: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it.
[+] SHOULD: Timestamps of files should be preserved.
[+] SHOULD: Patch files should be prefixed with %{name}-
[+] SHOULD: All patches should be commented in the spec file
[+] SHOULD: The reviewer should test that the package builds in mock.
[+] SHOULD: The package should compile and build into binary rpms on all supported architectures.
[+] SHOULD: The reviewer should test that the package functions as described.
[+] SHOULD: If scriptlets are used, those scriptlets must be sane.
[.] SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency.
[+] SHOULD: pkgconfig(.pc) files should be placed in a -devel pkg.
[+] SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself.
[.] SHOULD: Your package should contain man pages for binaries/scripts.

Comment 12 Shakthi Kannan 2011-01-29 17:31:59 UTC
- Changed Group to use System Environment/Libraries.
- Removed executable permission from C files.
- Created two separate patches for Makefile and manpage fixes.
- Removed passing datarootdir from configure.

SPEC: http://shakthimaan.fedorapeople.org/SPECS/CUnit.spec
SRPM: http://shakthimaan.fedorapeople.org/SRPMS/CUnit-2.1.2-6.fc14.src.rpmm

$ rpmlint CUnit.spec
CUnit.spec: W: invalid-url Source0: http://downloads.sourceforge.net/cunit/CUnit-2.1-2-src.tar.bz2 <urlopen error timed out>
0 packages and 1 specfiles checked; 0 errors, 1 warnings.

$ rpmlint CUnit-2.1.2-6.fc14.i686.rpm 
CUnit.i686: W: shared-lib-calls-exit /usr/lib/libcunit.so.1.0.1 exit@GLIBC_2.0
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

$ rpmlint CUnit-devel-2.1.2-6.fc14.i686.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

$  $ rpmlint CUnit-2.1.2-6.fc14.src.rpm 
CUnit.src: W: invalid-url Source0: http://downloads.sourceforge.net/cunit/CUnit-2.1-2-src.tar.bz2 <urlopen error timed out>
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

Successful Koji builds for F-13, F-14, EL-6 respectively:
http://koji.fedoraproject.org/koji/taskinfo?taskID=2749372
http://koji.fedoraproject.org/koji/taskinfo?taskID=2749363
http://koji.fedoraproject.org/koji/taskinfo?taskID=2749366

Comment 13 Martin Gieseking 2011-01-29 18:11:18 UTC
The spec file and the patches look good now, so we can finish here.

----------------
Package APPROVED
----------------

Comment 14 Shakthi Kannan 2011-01-29 18:17:10 UTC
New Package SCM Request
=======================
Package Name: CUnit
Short Description: A unit testing framework for C
Owners: shakthimaan chitlesh
Branches: F-13 F-14 EL-6
InitialCC: shakthimaan

Comment 15 Jeroen van Meeuwen 2011-01-29 18:34:40 UTC
May I request a EL-5 branch too please?

Comment 16 Shakthi Kannan 2011-01-30 04:43:20 UTC
The build fails with:

  automake-1.11: command not found

for EL-5:

  http://koji.fedoraproject.org/koji/getfile?taskID=2750004&name=build.log

Comment 17 Ralf Corsepius 2011-01-30 05:06:07 UTC
(In reply to comment #16)
> The build fails with:
> 
>   automake-1.11: command not found

<grin/> That's a classical autotool beginner's mistake:
For portability reasons, the autotools should not be run during builts, but be integrated into the sources.

Comment 18 Dennis Gilmore 2011-01-31 00:04:59 UTC
Git done (by process-git-requests).

Comment 19 Fedora Update System 2011-01-31 14:43:01 UTC
CUnit-2.1.2-6.fc13 has been submitted as an update for Fedora 13.
https://admin.fedoraproject.org/updates/CUnit-2.1.2-6.fc13

Comment 20 Fedora Update System 2011-01-31 14:43:09 UTC
CUnit-2.1.2-6.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/CUnit-2.1.2-6.fc14

Comment 21 Fedora Update System 2011-01-31 19:56:23 UTC
CUnit-2.1.2-6.fc13 has been pushed to the Fedora 13 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 CUnit'.  You can provide feedback for this update here: https://admin.fedoraproject.org/updates/CUnit-2.1.2-6.fc13

Comment 22 Fedora Update System 2011-02-08 22:54:02 UTC
CUnit-2.1.2-6.fc14 has been pushed to the Fedora 14 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 23 Fedora Update System 2011-02-08 22:54:15 UTC
CUnit-2.1.2-6.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 24 Fedora Update System 2011-03-24 07:33:14 UTC
CUnit-2.1.2-6.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/CUnit-2.1.2-6.el6

Comment 25 Fedora Update System 2011-04-08 20:30:51 UTC
CUnit-2.1.2-6.el6 has been pushed to the Fedora EPEL 6 stable repository.


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