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.
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
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.