Bug 593559 - Review Request: protobuf-c - C bindings for Google's Protocol Buffers
Summary: Review Request: protobuf-c - C bindings for Google's Protocol Buffers
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Martin Gieseking
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-05-19 06:49 UTC by David Robinson
Modified: 2014-08-04 12:02 UTC (History)
7 users (show)

Fixed In Version: protobuf-c-0.15-6.el5
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-05-01 03:30:57 UTC
martin.gieseking: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)
spec file (1.39 KB, application/octet-stream)
2010-05-19 06:50 UTC, David Robinson
no flags Details
updated SRPM (478.92 KB, application/x-rpm)
2010-06-02 00:18 UTC, David Robinson
no flags Details
updated srpm (497.33 KB, application/x-rpm)
2011-01-17 18:52 UTC, Bobby Powers
no flags Details

Description David Robinson 2010-05-19 06:49:51 UTC
Spec URL: Need somewhere to host it
SRPM URL: ditto
Description: This package provides a code generator and runtime libraries to use Protocol Buffers from pure C (not C++).

It uses a modified version of protoc called protoc-c.

This is my first Fedora package - I need a sponsor.

Comment 1 David Robinson 2010-05-19 06:50:38 UTC
Created attachment 415033 [details]
spec file

Comment 2 David Robinson 2010-05-19 08:08:47 UTC
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

Comment 3 Terje Røsten 2010-05-19 08:52:23 UTC
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.

Comment 4 David Robinson 2010-05-19 10:29:45 UTC
- %{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.

Comment 5 Terje Røsten 2010-05-19 10:46:11 UTC
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.

Comment 7 David Robinson 2010-06-02 00:18:45 UTC
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.

Comment 8 Martin Gieseking 2010-08-24 20:08:45 UTC
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.

Comment 9 Mamoru TASAKA 2010-09-11 16:46:28 UTC
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?

Comment 10 Martin Gieseking 2010-09-12 14:01:58 UTC
(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.

Comment 11 Mamoru TASAKA 2010-10-03 19:41:52 UTC
David, would you answer the question from Martin (comment 10 and
comment 8)?

Comment 12 David Robinson 2010-10-05 12:06:43 UTC
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

Comment 13 Martin Gieseking 2010-10-05 17:59:57 UTC
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

Comment 14 David Robinson 2010-10-11 11:36:55 UTC
Another informal review - bug #640455. More to come :-)

Comment 15 Martin Gieseking 2010-10-11 12:30:09 UTC
OK, thanks. This was a good start. Please choose another uncommented package and do a further informal review to practice a bit more. :)

Comment 16 Bobby Powers 2011-01-17 18:52:56 UTC
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@laptop.org> 0.14-1
+- New upstream release
+- Removed -devel dependency on protobuf-devel
+- Small specfile cleanups
+
 * Wed May 19 2010 David Robinson <zxvdr.au@gmail.com> 0.13-2
 - Spec file cleanup

Comment 17 Bobby Powers 2011-04-12 03:06:16 UTC
bump.  v 0.15 has been released.  It would be sweet to get this in f16.  Let me know if I can help.

Comment 18 Martin Gieseking 2011-04-12 07:17:34 UTC
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.

Comment 19 David Robinson 2011-04-13 11:38:46 UTC
Yep, I'm still interested. I've done another unofficial review, bug #691114.

Comment 20 Martin Gieseking 2011-04-19 13:09:16 UTC
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.

Comment 21 David Robinson 2011-04-21 10:09:56 UTC
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@laptop.org> 0.14-1
+* Wed Apr 20 2011 David Robinson <zxvdr.au@gmail.com> - 0.15-1
+- New upstream release
+- Spec file cleanup
+
+* Mon Jan 17 2011 Bobby Powers <bobby@laptop.org> - 0.14-1
 - New upstream release
 - Removed -devel dependency on protobuf-devel
 - Small specfile cleanups
 
-* Wed May 19 2010 David Robinson <zxvdr.au@gmail.com> 0.13-2
+* Wed May 19 2010 David Robinson <zxvdr.au@gmail.com> - 0.13-2
 - Spec file cleanup
 
-* Wed May 19 2010 David Robinson <zxvdr.au@gmail.com> 0.13-1
+* Wed May 19 2010 David Robinson <zxvdr.au@gmail.com> - 0.13-1
 - Initial packaging

Comment 22 Martin Gieseking 2011-04-21 13:01:23 UTC
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.

Comment 23 Martin Gieseking 2011-04-22 06:11:12 UTC
Parallel builds of this package fail in koji, so drop %{?_smp_mflags} again:
https://koji.fedoraproject.org/koji/taskinfo?taskID=3018026

Comment 24 David Robinson 2011-04-24 05:38:50 UTC
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@gmail.com> - 0.15-2
+- Spec file cleanup
+
 * Wed Apr 20 2011 David Robinson <zxvdr.au@gmail.com> - 0.15-1
 - New upstream release
 - Spec file cleanup

Comment 25 Martin Gieseking 2011-04-25 06:30:03 UTC
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
----------------

Comment 26 David Robinson 2011-04-25 22:29:46 UTC
New Package SCM Request
=======================
Package Name: protobuf-c
Short Description: C bindings for Google's Protocol Buffers
Owners: zxvdr
Branches: f14 f15 el6
InitialCC:

Comment 27 Bobby Powers 2011-04-26 00:36:15 UTC
great, thank you folks :)

Comment 28 Jason Tibbitts 2011-04-26 02:36:10 UTC
Git done (by process-git-requests).

Comment 29 Fedora Update System 2011-04-26 11:19:48 UTC
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

Comment 30 Fedora Update System 2011-04-26 11:22:54 UTC
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

Comment 31 Fedora Update System 2011-04-26 11:25:09 UTC
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

Comment 32 Fedora Update System 2011-04-26 15:32:56 UTC
protobuf-c-0.15-2.fc15 has been pushed to the Fedora 15 testing repository.

Comment 33 Fedora Update System 2011-05-01 03:30:47 UTC
protobuf-c-0.15-2.fc15 has been pushed to the Fedora 15 stable repository.

Comment 34 Fedora Update System 2011-05-19 22:03:14 UTC
protobuf-c-0.15-2.fc14 has been pushed to the Fedora 14 stable repository.

Comment 35 Fedora Update System 2011-05-20 17:54:58 UTC
protobuf-c-0.15-2.el6 has been pushed to the Fedora EPEL 6 stable repository.

Comment 36 David Robinson 2013-01-14 06:16:50 UTC
Package Change Request
======================
Package Name: protobuf-c
New Branches: el5

Comment 37 Gwyn Ciesla 2013-01-14 11:46:55 UTC
No owner specified.

Comment 38 David Robinson 2013-01-14 13:38:20 UTC
Package Change Request
======================
Package Name: protobuf-c
New Branches: el5
Owner: zxvdr

Comment 39 Gwyn Ciesla 2013-01-14 13:58:22 UTC
Git done (by process-git-requests).

Comment 40 Fedora Update System 2013-02-26 01:11:11 UTC
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

Comment 41 Fedora Update System 2013-06-18 20:43:50 UTC
protobuf-c-0.15-6.el5 has been pushed to the Fedora EPEL 5 stable repository.

Comment 42 Nikos Mavrogiannopoulos 2014-08-04 07:39:18 UTC
Package Change Request
======================
Package Name: protobuf-c
New Branches: epel7
Owners: nmav

Comment 43 Nikos Mavrogiannopoulos 2014-08-04 07:45:36 UTC
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.

Comment 44 Gwyn Ciesla 2014-08-04 12:02:49 UTC
Git done (by process-git-requests).


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