> I'm not a packager yet, but I have a sponsor

Really? This package needs a lot of love, since it isn't ready yet and doesn't pass the guidelines

Btw, your sponsor must be the one to do the final package review:
https://fedoraproject.org/wiki/Package_Review_Process#Reviewer


> rpmlint is a bit scary

Well, it's more scary that you don't comment on those rpmlint warnings and errors at all. Some of the errors found by rpmlint are obvious packaging mistakes. You don't even ask any questions about that. Also run rpmlint on the src.rpm.

Please become familiar with the Packaging Guidelines:
https://fedoraproject.org/wiki/Packaging:Guidelines

In particular take a look at
https://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries

But that's not the only problem. At the top of the list are misplaced files, such as

-rw-r--r--  /usr/share/doc/collection.html
-rw-r--r--  /usr/share/doc/default.css
-rw-r--r--  /usr/share/doc/index.html
-rw-r--r--  /usr/share/doc/misc.html
-rw-r--r--  /usr/share/doc/mixin.html
-rw-r--r--  /usr/share/doc/setup-teardown.html
-rw-r--r--  /usr/share/doc/test-advanced.html
-rw-r--r--  /usr/share/doc/test.html

and

drwxr-xr-x  /usr/share/doc/unittest-devel-0.50/test
-rw-r--r--  /usr/share/doc/unittest-devel-0.50/test/ExistingBaseTest.cpp
-rw-r--r--  /usr/share/doc/unittest-devel-0.50/test/ExistingBaseTest.d
-rw-r--r--  /usr/share/doc/unittest-devel-0.50/test/ExistingBaseTest.o
-rw-r--r--  /usr/share/doc/unittest-devel-0.50/test/Helper.cpp
-rw-r--r--  /usr/share/doc/unittest-devel-0.50/test/Helper.d
-rw-r--r--  /usr/share/doc/unittest-devel-0.50/test/Helper.hpp
-rw-r--r--  /usr/share/doc/unittest-devel-0.50/test/Helper.o
-rw-r--r--  /usr/share/doc/unittest-devel-0.50/test/Makefile
-rw-r--r--  /usr/share/doc/unittest-devel-0.50/test/Makefile.in
-rw-r--r--  /usr/share/doc/unittest-devel-0.50/test/RegistryTest.cpp
-rw-r--r--  /usr/share/doc/unittest-devel-0.50/test/RegistryTest.d
-rw-r--r--  /usr/share/doc/unittest-devel-0.50/test/RegistryTest.o
-rw-r--r--  /usr/share/doc/unittest-devel-0.50/test/SuiteTest.cpp
-rw-r--r--  /usr/share/doc/unittest-devel-0.50/test/SuiteTest.d
-rw-r--r--  /usr/share/doc/unittest-devel-0.50/test/SuiteTest.o
-rw-r--r--  /usr/share/doc/unittest-devel-0.50/test/TestHolderTest.cpp
-rw-r--r--  /usr/share/doc/unittest-devel-0.50/test/TestHolderTest.d
-rw-r--r--  /usr/share/doc/unittest-devel-0.50/test/TestHolderTest.o
-rw-r--r--  /usr/share/doc/unittest-devel-0.50/test/TestPtrTest.cpp
-rw-r--r--  /usr/share/doc/unittest-devel-0.50/test/TestPtrTest.d
-rw-r--r--  /usr/share/doc/unittest-devel-0.50/test/TestPtrTest.o
-rw-r--r--  /usr/share/doc/unittest-devel-0.50/test/TestTest.cpp
-rw-r--r--  /usr/share/doc/unittest-devel-0.50/test/TestTest.d
-rw-r--r--  /usr/share/doc/unittest-devel-0.50/test/TestTest.o
-rw-r--r--  /usr/share/doc/unittest-devel-0.50/test/unittesttest
-rw-r--r--  /usr/share/doc/unittest-devel-0.50/test/unittesttest.cpp
-rw-r--r--  /usr/share/doc/unittest-devel-0.50/test/unittesttest.d
-rw-r--r--  /usr/share/doc/unittest-devel-0.50/test/unittesttest.hpp
-rw-r--r--  /usr/share/doc/unittest-devel-0.50/test/unittesttest.o

That's why rpmlint prints the arch-dependent-file-in-usr-share errors. /usr/share is for arch-independent data.


-rw-r--r--  /usr/share/doc/unittest-devel-0.50/INSTALL

That one is irrelevant to RPM package end-users.


> %{__sed} -i 's|/usr/lib|%{buildroot}%{_libdir}|g' Makefile

That transformation breaks the build on 64-bit platforms where libdir is /usr/lib64.

Thanks for the review, Michael!

> > I'm not a packager yet, but I have a sponsor

> Really? This package needs a lot of love, since it isn't ready yet and doesn't
> pass the guidelines

> Btw, your sponsor must be the one to do the final package review:
> https://fedoraproject.org/wiki/Package_Review_Process#Reviewer

It's gotten a bit more complicated I think, since I've submitted more packages
after I've found a sponsor for the calibre package. As I understood from my
sponsor, he will watch all my submissions and reviews and help me get
sponsored.

> rpmlint is a bit scary

> Well, it's more scary that you don't comment on those rpmlint warnings and
> errors at all. Some of the errors found by rpmlint are obvious packaging
> mistakes. You don't even ask any questions about that. Also run rpmlint on the
> src.rpm.

> Please become familiar with the Packaging Guidelines:
> https://fedoraproject.org/wiki/Packaging:Guidelines

> In particular take a look at
> https://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries

Sorry for not commenting on the rpmlint errors. I was actually looking for
feedback as I didn't know what to do about all those errors. I knew what they
meant, but I also falsely assumed that all tests provided by the package must
be included in the final rpm.
The package now removes all the tests after they are run.

> But that's not the only problem. At the top of the list are misplaced files,
> such as

> -rw-r--r--  /usr/share/doc/collection.html
> -rw-r--r--  /usr/share/doc/default.css
> -rw-r--r--  /usr/share/doc/index.html
> -rw-r--r--  /usr/share/doc/misc.html
> -rw-r--r--  /usr/share/doc/mixin.html
> -rw-r--r--  /usr/share/doc/setup-teardown.html
> -rw-r--r--  /usr/share/doc/test-advanced.html
> -rw-r--r--  /usr/share/doc/test.html

Fixed.


> -rw-r--r--  /usr/share/doc/unittest-devel-0.50/INSTALL

> That one is irrelevant to RPM package end-users.

I found nothing in the Guidelines saying that irrelevant files should be
excluded, though I found a lot of INSTALL files that are written in a similar
manner and included. Originally I followed the example of the eject.spec
described in:
https://fedoraproject.org/wiki/PackageMaintainers/CreatingPackageHowTo

# yum provides */INSTALL|echo $((`wc -l`/7))
553

> > %{__sed} -i 's|/usr/lib|%{buildroot}%{_libdir}|g' Makefile

> That transformation breaks the build on 64-bit platforms where libdir is
> /usr/lib64.  

Fixed.

SPEC: http://mapleoin.fedorapeople.org/pkgs/unittest/unittest.spec.1
SRPM: http://mapleoin.fedorapeople.org/pkgs/unittest/unittest-0.50-62.2.fc11.src.rpm

* Thu Oct  1 2009 Ionuț Arțăriși <mapleoin> - 0.50-62.2
- don't include tests
- move html docs to the right dir
- add Provides: -static
- fixed Group:
- fixed /usr/lib problem for 64-bit systems in Makefile

Aha, I see you've found somebody who has started the sponsoring process but has not sponsored you yet (and hasn't approved the first package yet).

[...]

> I found nothing in the Guidelines saying that irrelevant files
> should be excluded,

There is nothing in the guidelines which says you should explicitly include useless files which aren't installed automatically by "make install". ;-P

And there is one guideline above all other guidelines. It's called "common sense". ;-)

The trick is to not include such files as %doc. Simpy write

  %doc COPYING

instead of:

  %doc COPYING INSTALL

:)

> though I found a lot of INSTALL files that are written in a similar
> manner and included.

Packaging mistakes. Not good examples. Especially not if it's the "standard" FSF Installation Instructions "INSTALL" file.

Ok. :)

http://mapleoin.fedorapeople.org/pkgs/unittest/unittest.spec.2
http://mapleoin.fedorapeople.org/pkgs/unittest/unittest-0.50-62.3.fc11.src.rpm

* Fri Oct  2 2009 Ionuț Arțăriși <mapleoin> - 0.50-62.3
- removed INSTALL file
- moved all doc files to the same dir

I've found the section, so actually it is covered by the guidelines:

| Irrelevant documentation include build instructions,
| the omnipresent INSTALL file containing generic build instructions,
| for example,

https://fedoraproject.org/wiki/Packaging:Guidelines#Documentation

The COPYING file is excecutable, but should not be.

Please add '-m 644' to install.

I'm not sure, if it's allowed to include docs like this.

How about 
make install prefix=%{buildroot} \
             HTMLDIR=$(pwd)/tmp \
             INCDIR=%{buildroot}%{_includedir}/%{name} \

and instead %doc COPYING docs/*

This way, the HTMLDIR would be in the BUILD folder, but now used anyway and the docs proberly installed.
This also solves the executable bit problem mentioned above ;)

Thanks Thomas!

I left the HTMLDIR the way it was since we're not touching it anyway and used %doc instead.

http://mapleoin.fedorapeople.org/pkgs/unittest/unittest.spec
http://mapleoin.fedorapeople.org/pkgs/unittest/unittest-0.50-62.4.fc12.src.rpm


* Sat Oct 31 2009 Ionuț Arțăriși <mapleoin> - 0.50-62.4
- use %doc macro

Package Review
==============

Key:
 - = N/A
 x = Check
 ! = Problem
 ? = Not evaluated

=== REQUIRED ITEMS ===
 [x] Package is named according to the Package Naming Guidelines.
 [x] Spec file name must match the base package %{name}, in the format %{name}.spec.
 [x] Package meets the Packaging Guidelines
 [x] Package successfully compiles and builds into binary rpms on at least one supported architecture.
     Tested on: 
       [] devel/i386 
       [] devel/x86_64
       [] F11/i386 
       [x] F11/x86_64

_______________________

 [!] Rpmlint output:
     $ rpmlint unittest.spec unittest-0.50-62.4.fc11.src.rpm x86_64/*
unittest.spec:40: W: rpm-buildroot-usage %prep %{__sed} -i 's|@libdir@|%{buildroot}%{_libdir}|g' Makefile.in
unittest.spec:44: W: rpm-buildroot-usage %build make %{?_smp_mflags} libdir=%{buildroot}%{_libdir}
unittest.spec:69: W: macro-in-%changelog %doc
unittest.src:40: W: rpm-buildroot-usage %prep %{__sed} -i 's|@libdir@|%{buildroot}%{_libdir}|g' Makefile.in
unittest.src:44: W: rpm-buildroot-usage %build make %{?_smp_mflags} libdir=%{buildroot}%{_libdir}
unittest.src:69: W: macro-in-%changelog %doc
unittest-debuginfo.x86_64: E: empty-debuginfo-package
3 packages and 1 specfiles checked; 1 errors, 6 warnings.

- For the empty-debuginfo-package, please add a
  %global debug_package %{nil}
- for the macro-in-%changelog: replace %doc with %%doc
- The other ones are ok.
_____________________

 [x] Buildroot is correct
     (%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n))
 [x] Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines.
 [x] License field in the package spec file matches the actual license.
     License type: BSD
 [x] If (and only if) the source package includes the text of the license(s) in
its own file, then that file, containing the text of the license(s) for the
package is included in %doc.
 [x] Spec file is legible and written in American English.
 [x] Sources used to build the package matches the upstream source, as provided in the spec URL.
     Upstream source: 6eaa2823620c2e21fc745bd8da6a26b2
     Build source:    6eaa2823620c2e21fc745bd8da6a26b2
 [x] Package is not known to require ExcludeArch
 [x] All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines.
 [-] The spec file handles locales properly.
 [-] ldconfig called in %post and %postun if required.
 [x] Package must own all directories that it creates.
 [-] Package requires other packages for directories it uses.
 [x] Package does not contain duplicates in %files.
 [x] Permissions on files are set properly.
 [x] Package has a %clean section, which contains rm -rf %{buildroot}.
 [x] Package consistently uses macros.
 [x] Package contains code, or permissable content.
 [-] Large documentation files are in a -doc subpackage, if required.
 [x] Package uses nothing in %doc for runtime.
 [x] Header files in -devel subpackage, if present.
 [x] Static libraries in -devel subpackage, if present.
 [-] Package requires pkgconfig, if .pc files are present.
 [-] Development .so files in -devel subpackage, if present.

______________________________

 [!] Fully versioned dependency in subpackages, if present.
     -devel should require : %{name} = %{version}-%{release}
     I believe, it's usefull to also add a 
     Requires: %{name}-devel = %{version}-%{release}
     to the main package, because most users would just run a
     'yum install unittest'

______________________________

 [-] Package does not contain any libtool archives (.la).
 [-] Package contains a properly installed %{name}.desktop file if it is a GUI application.
 [x] Package does not own files or directories owned by other packages.

=== SUGGESTED ITEMS ===
 [x] Latest version is packaged.
 [-] Description and summary sections in the package spec file contains translations for supported Non-English languages, if available.
 [x] Package should compile and build into binary rpms on all supported architectures.
http://koji.fedoraproject.org/koji/taskinfo?taskID=1786671 

 [x] Package functions as described (no hardware to test with).
     There is a testsuite for this.
 

_______________________________

TODO:
- use %global instead %define in first line

- rpmlint:
  * For the empty-debuginfo-package, please add a
    %global debug_package %{nil}
    to avoid building a debuginfo package
  * for the macro-in-%changelog: replace %doc with %%doc

- Fully versioned dependency in subpackage

- add a %check section:
  %check
  ./test/unittesttest

Not much to do :)

Thanks for picking this up, Thomas!

I added all of your changes, however:

> Requires: %{name}-devel = %{version}-%{release}
>      to the main package, because most users would just run a
>      'yum install unittest'

How does this work if there is no main package?

http://mapleoin.fedorapeople.org/pkgs/unittest/unittest-0.50-62.5.fc12.src.rpm
http://mapleoin.fedorapeople.org/pkgs/unittest/unittest.spec

* Wed Nov  4 2009 Ionuț Arțăriși <mapleoin> - 0.50-62.5
- use %%global instead of %%define
- comment escape doc macro
- added subpackage requires
- declare that there is no debug package
- add %%check

(In reply to comment #9)
> Thanks for picking this up, Thomas!

You are welcome. :)
 
> I added all of your changes, however:
> 
> > Requires: %{name}-devel = %{version}-%{release}
> >      to the main package, because most users would just run a
> >      'yum install unittest'
> 
> How does this work if there is no main package?

Hmm, good question ;)
I read something similar here:
http://cvs.fedoraproject.org/viewvc/rpms/boost/devel/boost.spec?view=markup

# boost is an "umbrella" package that pulls in all other boost components
Requires: ......

and no %files section.

So there will be a 'unittest' package laterly in the repos, which install nothing, without the Requires: unittest-devel. Now it does.



Why did you a "Requires: %{name}-devel = %{version}-%{release}" in the devel package? It should be "%{name} = %{version}-%{release}".

It's just needed in the main package. If you require it here, it won't be installable (I believe ;)). This way the devel package requires itself and on the first install, it will fail.

Please remove the -devel.

All other issues are done.

I don't want to wait now for another release, to see if you can delete a word :-D -> approving this now.

________________________

APPROVED

After talking a bit more with folks on #fedora-devel concerning the "umbrella" package issue, it seems there's no need for a -devel subpackage, since the whole unittest package is a developmental one (another good example is gcc).

So I removed it altogether and now everything is in the main unittest package.

The obligatory rpmlint warnings are there, of course, but they should be ok:

$ rpmlint unittest-0.50-62.6.fc12.x86_64.rpm 
unittest.x86_64: W: devel-file-in-non-devel-package /usr/lib64/libunittest.a
unittest.x86_64: W: devel-file-in-non-devel-package /usr/include/unittest/ExistingBase.hpp
unittest.x86_64: W: devel-file-in-non-devel-package /usr/include/unittest/Registry.hpp
unittest.x86_64: W: devel-file-in-non-devel-package /usr/include/unittest/UnitTest.hpp
1 packages and 0 specfiles checked; 0 errors, 4 warnings.

http://mapleoin.fedorapeople.org/pkgs/unittest/unittest.spec
http://mapleoin.fedorapeople.org/pkgs/unittest/unittest-0.50-62.6.fc12.src.rpm


I'll wait for another go ahead from you before making the CVS request since this is probably not the little change you had in mind when approving :).

(In reply to comment #11)
> I'll wait for another go ahead from you before making the CVS request since
> this is probably not the little change you had in mind when approving :).  

Hehe, true^^

You added the provides %{name}-static, so it's completely ok...



'Go ahead' ;)

New Package CVS Request
=======================
Package Name: unittest
Short Description: C++ unit testing framework
Owners: mapleoin
Branches: F-11 F-12
InitialCC:

cvs done.

unittest-0.50-62.6.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/unittest-0.50-62.6.fc11

unittest-0.50-62.6.fc11 has been pushed to the Fedora 11 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 unittest'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F11/FEDORA-2009-11254

unittest-0.50-62.6.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

Package Change Request
======================
Package Name: unittest
New Branches: el5 el6
Owners: mapleoin

I want to maintain this package in EPEL as well as it's needed by mongodb.

Git done (by process-git-requests).