Created attachment 415033 [details]
spec file

I've done a scratch build, SRPM is here:

http://koji.fedoraproject.org/koji/getfile?taskID=2196219&name=protobuf-c-0.13-1.fc14.src.rpm

Some initial comments

 - use version macro in Source url:
   Source0: http://protobuf-c.googlecode.com/files/protobuf-c-%{version}.tar.gz

Ref: https://fedoraproject.org/wiki/Packaging:SourceURL#Using_.25.7Bversion.7D

 - remove %check section if its empty or better: enable tests

 - don't ship static libs (.a)  if not strictly needed. Remove .la files.

Ref: https://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries

  - devel package need deps on protobuf-devel (for ownership of %{_includedir}/google

  - don't include explicit req. on protobuf, rpmbuild picks it up itself.

  - include ChangeLog and TODO by using the %doc macro.

- %{version} added (not sure how I missed that!)
- %check now calls make check
- added --disable-static option
- removed .la file
- added dep on protobuf-devel to devel subpackage
- removed requires on protobuf... I didn't realise rpmbuild picked up dependencies like this. How do I know when requires is not explicitly needed?
- added %doc's

new package is here:
http://koji.fedoraproject.org/koji/getfile?taskID=2196577&name=protobuf-c-0.13-1.fc14.src.rpm

I haven't bumped %{release} - not sure whether I need to during review.

I prefer you update release *and* changelog on every change. 

After all, that's what changelog is for.

Patch to include TODO and ChangeLog a bit simpler:

--- SPECS/protobuf-c.spec~      2010-05-19 11:58:39.000000000 +0200
+++ SPECS/protobuf-c.spec       2010-05-19 12:39:41.447810201 +0200
@@ -41,8 +41,6 @@
 %install
 rm -rf $RPM_BUILD_ROOT
 make install DESTDIR=$RPM_BUILD_ROOT
-install -p -m 644 -D TODO $RPM_BUILD_ROOT/%{_docdir}/TODO
-install -p -m 644 -D ChangeLog $RPM_BUILD_ROOT/%{_docdir}/ChangeLog
 rm -f $RPM_BUILD_ROOT/%{_libdir}/libprotobuf-c.la
 
 %post -p /sbin/ldconfig
@@ -55,8 +53,7 @@
 %defattr(-,root,root,-)
 %{_bindir}/protoc-c
 %{_libdir}/libprotobuf-c.so.*
-%doc %{_docdir}/TODO
-%doc %{_docdir}/ChangeLog
+%doc ChangeLog TODO
 
 %files devel
 %defattr(-,root,root,-)

rpmlint warning:

 W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 21)

Seems like tags in -devel subpackage use tabs, remove those.

updated:

http://koji.fedoraproject.org/koji/getfile?taskID=2198635&name=protobuf-c-0.13-2.fc14.src.rpm

Created attachment 418863 [details]
updated SRPM

Looks like the SRPM has been removed. I've attached it here. It fixes the whitespace issue and %doc.

Hi David,

your package looks fine now and could be approved. However, since you have to be sponsored first, some additional tasks are required. :)

If you're still interested in becoming a member of the packager group, you should show that you're familiar with the packaging guidelines. This is usually done by providing at least one further package, and by informally reviewing some packages submitted by other packagers.

Martin, if you are going to review this package formally,
would you assign this bug to yourself?

David, do you have your another review request, or have you done
a pre-review of other person's review request?

(In reply to comment #9)
> Martin, if you are going to review this package formally,
> would you assign this bug to yourself?

Yes, sure. My previous comment was just intended as a general note. But if David is still interested in joining the packager group and has no sponsor yet, I'm willing to take this review request.

David, would you answer the question from Martin (comment 10 and
comment 8)?

Hi Mamoru, Martin,

I'll do some package reviews over the next few days and list them here as I go. Here's one:

https://bugzilla.redhat.com/show_bug.cgi?id=633104

Hi David,

thanks for the feedback. If you don't have a sponsor yet, I can sponsor you. But before, you should do two or three informal reviews in order to familiarize yourself with the packaging guidelines and the reviewing process. 

As a first step, please have a look at the reviewing guidelines [1]. Then pick an uncommented package from the review request queue, e.g. bug #626458, and carefully check whether the package satisfies all MUST and SHOULD items listed in the reviewing guidelines. Finally, post your results to the corresponding ticket. If you have any questions, feel free to ask here or write me an email.

[1] https://fedoraproject.org/wiki/Packaging:ReviewGuidelines

Another informal review - bug #640455. More to come :-)

OK, thanks. This was a good start. Please choose another uncommented package and do a further informal review to practice a bit more. :)

Created attachment 473912 [details]
updated srpm

I've updated and minimally tested the attached srpm (built with mock for i686 and x86_64), as updated.  The patch is below, not sure which is more useful.

--- protobuf-c.spec~	2011-01-17 10:47:33.196403047 -0800
+++ protobuf-c.spec	2011-01-17 10:34:00.965444275 -0800
@@ -1,6 +1,6 @@
 Name:           protobuf-c
-Version:        0.13
-Release:        2%{?dist}
+Version:        0.14
+Release:        1%{?dist}
 Summary:        C bindings for Google's Protocol Buffers
 
 Group:          Development/Libraries
@@ -21,7 +21,6 @@
 Summary:        Protocol Buffers C headers and libraries
 Group:          Development/Libraries
 Requires:       %{name} = %{version}-%{release}
-Requires:       protobuf-devel
 
 %description devel
 This package contains protobuf-c headers and libraries
@@ -31,9 +30,7 @@
 
 %build
 %configure --disable-static
-# Causes build to fail
-#make %{?_smp_mflags}
-make
+make %{?_smp_mflags}
 
 %check
 make check
@@ -59,8 +56,14 @@
 %defattr(-,root,root,-)
 %{_includedir}/google/protobuf-c
 %{_libdir}/libprotobuf-c.so
+%{_libdir}/pkgconfig/libprotobuf-c.pc
 
 %changelog
+* Mon Jan 17 2011 Bobby Powers <bobby> 0.14-1
+- New upstream release
+- Removed -devel dependency on protobuf-devel
+- Small specfile cleanups
+
 * Wed May 19 2010 David Robinson <zxvdr.au> 0.13-2
 - Spec file cleanup

bump.  v 0.15 has been released.  It would be sweet to get this in f16.  Let me know if I can help.

Sorry, this review request went out of my radar. David, are you still interested in maintaining this package? If so, please let me know and we could finish two more informal reviews of uncommented package submissions in the next few days. Afterwards, I'll approve you.

Bobby, since this is David's first package submission, we have to finish the sponsoring process first.

Yep, I'm still interested. I've done another unofficial review, bug #691114.

OK, the package looks almost good so far. Just a couple of minor notes:

- Change the Group of the base package to "System Environment/Libraries".

- Please add a short info about protobuf to the %description, e.g. 
  "Protocol Buffers are a way of encoding structured data in an efficient yet 
  extensible format." Even if this is a C language extension of protobuf, it 
  might help the user to get a clue about what this is all about.

- The %description of the devel package should be a complete sentence too 
  (with final dot).

- As already suggested by Bobby, I recommend to update the package to the 
  latest upstream version. Check whether the new version build with 
  make %{?_smp_mflags}.

- As far as I can see, protobuf-devel is not required to develop applications
  with protobuf-c. Thus, you can drop Requires: protobuf-devel from the 
  devel package.

- If you don't want to maintain this package for EPEL < 6, you can drop all 
  the buildroot stuff (BuildRoot tag, rm -rf $RPM_BUILD_ROOT in %install, and 
  the %clean section). 

- File LICENSE is present in the SVN repo but missing in the tarball. Please 
  ask upstream to add it to future releases. Until then, grab the file from
  the repo and add it to the base package:
  Source1: http://code.google.com/p/protobuf-c/source/browse/tags/0.15/LICENSE
  in %prep: cp %{SOURCE1} .
  in %files: %doc LICENSE

- Caution: Upstream has changed the license of the current development 
  version to BSD. So keep in mind to adapt the License field and file LICENSE 
  once you update the package in the future.

Updated spec and srpm are here:

Spec URL: http://zxvdr.fedorapeople.org/RPMS/protobuf-c/protobuf-c.spec
SRPM URL: http://zxvdr.fedorapeople.org/RPMS/protobuf-c/protobuf-c-0.15-1.fc14.src.rpm

I've asked upstream if they can include the license in the tarball:
http://groups.google.com/group/protobuf-c/t/27fabd55539f1a54

make %{?_smp_mflags} seems to work when the package is built in mock.

The changelog changes are to make the format consistent w/ rpmdev-bumpspec.

--- protobuf-c.spec.orig	2011-04-20 06:50:12.181661465 +1000
+++ protobuf-c.spec	2011-04-20 07:54:39.366528303 +1000
@@ -1,19 +1,20 @@
 Name:           protobuf-c
-Version:        0.14
+Version:        0.15
 Release:        1%{?dist}
 Summary:        C bindings for Google's Protocol Buffers
 
-Group:          Development/Libraries
+Group:          System Environment/Libraries
 License:        ASL 2.0
 URL:            http://code.google.com/p/protobuf-c/
 Source0:        http://protobuf-c.googlecode.com/files/protobuf-c-%{version}.tar.gz
-BuildRoot:      %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
+Source1:        http://protobuf-c.googlecode.com/svn/tags/%{version}/LICENSE
 
 BuildRequires:  protobuf-devel
 
 %description
-This package provides a code generator and runtime libraries to use Protocol
-Buffers from pure C (not C++).
+Protocol Buffers are a way of encoding structured data in an efficient yet 
+extensible format. This package provides a code generator and run-time
+libraries to use Protocol Buffers from pure C (not C++).
 
 It uses a modified version of protoc called protoc-c. 
 
@@ -23,10 +24,11 @@
 Requires:       %{name} = %{version}-%{release}
 
 %description devel
-This package contains protobuf-c headers and libraries
+This package contains protobuf-c headers and libraries.
 
 %prep
 %setup -q
+cp %{SOURCE1} .
 
 %build
 %configure --disable-static
@@ -36,21 +38,17 @@
 make check
 
 %install
-rm -rf $RPM_BUILD_ROOT
 make install DESTDIR=$RPM_BUILD_ROOT
 rm -f $RPM_BUILD_ROOT/%{_libdir}/libprotobuf-c.la
 
 %post -p /sbin/ldconfig
 %postun -p /sbin/ldconfig
 
-%clean
-rm -rf $RPM_BUILD_ROOT
-
 %files
 %defattr(-,root,root,-)
 %{_bindir}/protoc-c
 %{_libdir}/libprotobuf-c.so.*
-%doc TODO ChangeLog
+%doc TODO LICENSE ChangeLog
 
 %files devel
 %defattr(-,root,root,-)
@@ -59,13 +57,17 @@
 %{_libdir}/pkgconfig/libprotobuf-c.pc
 
 %changelog
-* Mon Jan 17 2011 Bobby Powers <bobby> 0.14-1
+* Wed Apr 20 2011 David Robinson <zxvdr.au> - 0.15-1
+- New upstream release
+- Spec file cleanup
+
+* Mon Jan 17 2011 Bobby Powers <bobby> - 0.14-1
 - New upstream release
 - Removed -devel dependency on protobuf-devel
 - Small specfile cleanups
 
-* Wed May 19 2010 David Robinson <zxvdr.au> 0.13-2
+* Wed May 19 2010 David Robinson <zxvdr.au> - 0.13-2
 - Spec file cleanup
 
-* Wed May 19 2010 David Robinson <zxvdr.au> 0.13-1
+* Wed May 19 2010 David Robinson <zxvdr.au> - 0.13-1
 - Initial packaging

Here's the formal review. There's just one thing left to be fixed:
The devel package owns directory %{_includedir}/google/protobuf-c/ but not its parent %{_includedir}/google/. You can fix this by adding
  %dir %{_includedir}/google/
to the devel package. %{_includedir}/google/ is also co-owned by several unrelated packages.


$ rpmlint /var/lib/mock/fedora-14-x86_64/result/*.rpm
protobuf-c.src: W: spelling-error %description -l en_US protoc -> proton, protocol, protochordate
protobuf-c.src: W: invalid-url Source0: http://protobuf-c.googlecode.com/files/protobuf-c-0.15.tar.gz HTTP Error 404: Not Found
protobuf-c.x86_64: W: spelling-error %description -l en_US protoc -> proton, protocol, protochordate
protobuf-c.x86_64: W: no-manual-page-for-binary protoc-c
protobuf-c-devel.x86_64: W: no-documentation
4 packages and 0 specfiles checked; 0 errors, 5 warnings.

All the warnings are expected or false positive and can be ignored.


---------------------------------
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.
    - ASL 2.0 according to source file headers

[+] 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 protobuf-c-0.15.tar.gz*
    73ff0c8df50d2eee75269ad8f8c07dc8  protobuf-c-0.15.tar.gz
    73ff0c8df50d2eee75269ad8f8c07dc8  protobuf-c-0.15.tar.gz.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, ...
[X] MUST: A package must own all directories that it creates.
    - the devel package currently doesn't own %{_includedir}/google/

[+] MUST: A Fedora package must not list a file more than once in %files.
[+] MUST: Permissions on files must be set properly.
[+] 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: If a package contains library files with a suffix, .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.

[+] 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: The reviewer should test that the package builds in mock.
[+] 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.

Parallel builds of this package fail in koji, so drop %{?_smp_mflags} again:
https://koji.fedoraproject.org/koji/taskinfo?taskID=3018026

Fixed.

Spec URL: http://zxvdr.fedorapeople.org/RPMS/protobuf-c/protobuf-c.spec
SRPM URL:
http://zxvdr.fedorapeople.org/RPMS/protobuf-c/protobuf-c-0.15-2.fc14.src.rpm

--- protobuf-c.spec.orig	2011-04-21 19:23:06.000000000 +1000
+++ protobuf-c.spec	2011-04-24 15:29:36.060990432 +1000
@@ -1,6 +1,6 @@
 Name:           protobuf-c
 Version:        0.15
-Release:        1%{?dist}
+Release:        2%{?dist}
 Summary:        C bindings for Google's Protocol Buffers
 
 Group:          System Environment/Libraries
@@ -32,7 +32,9 @@
 
 %build
 %configure --disable-static
-make %{?_smp_mflags}
+# Causes build to fail
+#make %{?_smp_mflags}
+make
 
 %check
 make check
@@ -52,11 +54,15 @@
 
 %files devel
 %defattr(-,root,root,-)
+%dir %{_includedir}/google
 %{_includedir}/google/protobuf-c
 %{_libdir}/libprotobuf-c.so
 %{_libdir}/pkgconfig/libprotobuf-c.pc
 
 %changelog
+* Sun Apr 24 2011 David Robinson <zxvdr.au> - 0.15-2
+- Spec file cleanup
+
 * Wed Apr 20 2011 David Robinson <zxvdr.au> - 0.15-1
 - New upstream release
 - Spec file cleanup

OK, the package is ready now. 
The next step is to request a Git repository with the distro branches you're planning to maintain. See here for further information:
https://fedoraproject.org/wiki/PackageMaintainers/CVSAdminProcedure

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

New Package SCM Request
=======================
Package Name: protobuf-c
Short Description: C bindings for Google's Protocol Buffers
Owners: zxvdr
Branches: f14 f15 el6
InitialCC:

great, thank you folks :)

Git done (by process-git-requests).

protobuf-c-0.15-2.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/protobuf-c-0.15-2.fc15

protobuf-c-0.15-2.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/protobuf-c-0.15-2.fc14

protobuf-c-0.15-2.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/protobuf-c-0.15-2.el6

protobuf-c-0.15-2.fc15 has been pushed to the Fedora 15 testing repository.

protobuf-c-0.15-2.fc15 has been pushed to the Fedora 15 stable repository.

protobuf-c-0.15-2.fc14 has been pushed to the Fedora 14 stable repository.

protobuf-c-0.15-2.el6 has been pushed to the Fedora EPEL 6 stable repository.

Package Change Request
======================
Package Name: protobuf-c
New Branches: el5

No owner specified.

Package Change Request
======================
Package Name: protobuf-c
New Branches: el5
Owner: zxvdr

Git done (by process-git-requests).

protobuf-c-0.15-6.el5 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/protobuf-c-0.15-6.el5

protobuf-c-0.15-6.el5 has been pushed to the Fedora EPEL 5 stable repository.

Package Change Request
======================
Package Name: protobuf-c
New Branches: epel7
Owners: nmav

I've not received a reply from the original owner, so I took the liberty of applying for that branch as I need it in a dependency. If the original owner wants to keep that branch please assign it to him.

Git done (by process-git-requests).