Bug 459088 (google-protobuf) - Review Request: protobuf - Protocol Buffers - Google's data interchange format
Summary: Review Request: protobuf - Protocol Buffers - Google's data interchange format
Keywords:
Status: CLOSED NEXTRELEASE
Alias: google-protobuf
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 459110 (view as bug list)
Depends On: 469491
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-08-14 12:51 UTC by Lev Shamardin
Modified: 2010-01-26 21:29 UTC (History)
9 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-12-02 13:41:35 UTC
Type: ---
Embargoed:
mtasaka: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)
pkgconfig for 2.0.2 (1.47 KB, text/plain)
2008-10-06 09:02 UTC, Kaj J. Niemi
no flags Details
protobuf-pkgconfig-autotools.patch updated for 2.0.2 (663 bytes, text/plain)
2008-10-06 09:03 UTC, Kaj J. Niemi
no flags Details
protobuf 2.0.2 patch diff to 2.0.1 (4.11 KB, patch)
2008-10-06 09:05 UTC, Kaj J. Niemi
no flags Details | Diff
protobuf 2.0.2 patch diff to 2.0.1 (4.11 KB, patch)
2008-10-06 09:13 UTC, Kaj J. Niemi
no flags Details | Diff

Description Lev Shamardin 2008-08-14 12:51:52 UTC
Spec URL: http://shamardin.googlepages.com/protobuf.spec
SRPM URL: http://shamardin.googlepages.com/protobuf-2.0.0-0.1.beta.fc8.src.rpm
Description:

Protocol Buffers are a way of encoding structured data in an efficient
yet extensible format. Google uses Protocol Buffers for almost all of
its internal RPC protocols and file formats.

Protocol buffers are a flexible, efficient, automated mechanism for
serializing structured data – think XML, but smaller, faster, and
simpler. You define how you want your data to be structured once, then
you can use special generated source code to easily write and read
your structured data to and from a variety of data streams and using a
variety of languages. You can even update your data structure without
breaking deployed programs that are compiled against the "old" format.

Project homepage: http://code.google.com/p/protobuf/

This is my first package for Fedora and I am seeking for a sponsor.

Comment 1 Jason Tibbitts 2008-08-14 14:39:03 UTC
*** Bug 459110 has been marked as a duplicate of this bug. ***

Comment 2 Rick L Vinyard Jr 2008-08-14 19:55:16 UTC
I didn't realize someone else was working on this. Feel free to take any pieces from this version that you need:

Spec URL: http://miskatonic.cs.nmsu.edu/pub/protobuf.spec
SRPM URL: http://miskatonic.cs.nmsu.edu/pub/protobuf-2.0.0-0.1.beta.fc9.src.rpm

I didn't include the python stuff, but I did include a vim package and a pkgconfig file, which is isn't in the protocol buffers release, but probably should be. You might want to contact upstream (Google) about adding it.

Comment 3 Lev Shamardin 2008-08-15 09:46:52 UTC
I've added pkgconfig to -devel and vim subpackage to my version of the package and updated the spec & srpm on the web. Will be waiting for any comments.

Comment 4 Rick L Vinyard Jr 2008-08-15 21:25:32 UTC
I haven't done a formal review, but is there any API exposed from libprotoc... perhaps even run-time compiling? If so, libprotoc.so.* might need to be in the main package.

Otherwise, from a cursory glance it looks good.

Comment 5 Lev Shamardin 2008-08-16 08:08:33 UTC
To my best knowledge, libprotoc is used only by the protoc compiler. I'm using this library from python, but all sample C++ code is linked only against libprotobuf, so I conclude that libprotoc is required only by the compiler. 

According to the documentation on the project site, all sources included in libprotoc are parts of the compiler. If you think that someone can try to extend the compiler, or to use the compiler at run time, I can move the libprotoc to the main package, but I doubt that because libprotoc exposes only C++ API and compiler generates source code in C++ which cannot be used at run time without recompiling.

I wonder if it is possible to generate noarch subpackages from the same spec as the main arch package? Strictly speaking, -vim and -python subpackages should be noarch.

Comment 6 Rahul Sundaram 2008-08-23 03:32:31 UTC
RPM doesn't yet allow noarch sub packages when the main package is arch specific but refer

http://wiki.rpm.org/Problems_of_Building

Comment 7 Rick L Vinyard Jr 2008-09-04 14:43:26 UTC
Just an FYI... there is a new release.

In particular, what I'm interested in is this:
  * Fix bytes type setter to work with byte sequences with embedded NULLs

Comment 8 Lev Shamardin 2008-09-04 17:44:47 UTC
I've updated the spec file on my webpage for version 2.0.1, and here is the new 

SRPM URL: http://shamardin.googlepages.com/protobuf-2.0.1-1.fc8.src.rpm
Spec URL: http://shamardin.googlepages.com/protobuf.spec

Comment 9 Rick L Vinyard Jr 2008-09-05 17:15:19 UTC
I'm not a sponsor, but perhaps this review will help speed things up:

===== MUST Items =====
  UNKNOWN - rpmlint results
    protobuf-python.i386: E: non-executable-script /usr/lib/python2.5/site-
        packages/google/protobuf/descriptor_pb2.py 0644
    protobuf-static.i386: W: no-documentation
    protobuf-vim.i386: W: no-documentation
    8 packages and 0 specfiles checked; 1 errors, 2 warnings.

    rpmlint gives an error for 0644 permissions, but other python packages use
    the same permissions. False positive in rpmlint?
  PASSED - package is named according to package guidelines
  PASSED - spec file name matches the base package
  PASSED - package meets guidelines
  PASSED - ASL 2.0 license is acceptable
  PASSED - spec license field matches actual license
  PASSED - license in package included in %doc
  PASSED - spec is written in American English
  PASSED - spec file is legible
  PASSED - sources match upstream sources
           Upstream md5sum: add533032c5abffa378306fb580a18a4  protobuf-2.0.1.tar.bz2
           srpm md5sum:     add533032c5abffa378306fb580a18a4  protobuf-2.0.1.tar.bz2
  PASSED - package successfully builds on i386 and x86_64
  PASSED - all dependencies listed in BuildRequires
  PASSED - no BuildRequires duplicates
  PASSED - no BuildRequires listed exceptions
  PASSED - specfile does not have locales
  PASSED - ldconfig correctly called for all shared lib packages
  PASSED - relocatable
  PASSED - owns all directories it creates
  PASSED - doesn't own files or directories already owned by other packages
  PASSED - no duplicate files
  PASSED - permissions set properly
  PASSED - each %files section has a proper %defattr
  PASSED - %clean present and proper
  PASSED - macro usage consistent
  PASSED - package contains code, not content
  PASSED - Documentation not large... no need for separate %doc
  PASSED - %doc section does not effect runtime
  PASSED - devel files are in a separate -devel subpackage
  PASSED - static libraries present, but in separate -static subpackage
           given the nature of this library, presence of statics are beneficial
  FAILED - The -devel also needs a Requires: pkgconfig
  PASSED - -devel requires the base using a fully versioned dependency
  FAILED - libtool archives are included in -devel
           Add this line after make in %install to fix and remove .la from -devel
             find %{buildroot} -type f -name "*.la" -exec rm -f {} ';'
  PASSED - no GUI applications, no need for desktop files
  PASSED - package doesn't own directories owned by other packages
  PASSED - all filenames are UTF-8

===== SHOULD Items =====
  GOOD    - Package builds in mock; Fedora 9 i386 and x86_64
  GOOD    - Package functionality tested:
           main library functions properly with app built against libprotobuf
           -compiler functions properly
           -devel functions properly including pkgconfig
           -vim functions properly
  UNKNOWN - shouldn't -python depend on the base package for libprotobuf? 
  GOOD    - package uses disttag


===== OTHER Items =====
I'd prefer to see the make line look like this to preserve permissions:
  %{__make} %{?_smp_mflags} INSTALL="%{__install} -p" DESTDIR=%{buildroot} STRIPBINARIES=no

Comment 10 Rick L Vinyard Jr 2008-09-05 18:43:11 UTC
Sorry, minor correction. The modified make line preserves timestamps, not permissions.

Comment 11 Rick L Vinyard Jr 2008-09-09 22:13:47 UTC
Another fix I noticed...

The protobuf.pc file needs to remove -lprotoc from the Libs line.

Comment 12 Lev Shamardin 2008-09-15 08:07:59 UTC
That was a busy week, sorry for a delay.

The new spec and srpm are as usual:

SRPM URL: http://shamardin.googlepages.com/protobuf-2.0.1-2.fc8.src.rpm
Spec URL: http://shamardin.googlepages.com/protobuf.spec

From %changelog:
* Mon Sep 15 2008 Lev Shamardin <shamardin> - 2.0.1-2
- Added -p switch to install commands to preserve timestamps.
- Fixed Version and Libs in pkgconfig script.
- Added pkgconfig requires for -devel package.
- Removed libtool archives from -devel package.

Comment 13 Lev Shamardin 2008-09-15 08:13:59 UTC
Additional remark about python subpackage:
The -python subpackage should not depend on the base package or any other packages because it is a pure python implementation. It requires nothing but itself to run, but still requires protobuf-compiler for development. May be it is a good idea to split the protobuf SRPM into to SRPMS: protobuf and python-protobuf. The first one will contain everything except python subpackage, and the latter will contain only python part and will have BuildArch: noarch. Still I'm not sure it is a good idea, because these SRPMS will contain exactly the same sources.

Comment 14 David Fraser 2008-09-30 07:37:22 UTC
(In reply to comment #13)
> Additional remark about python subpackage:
> The -python subpackage should not depend on the base package or any other
> packages because it is a pure python implementation. It requires nothing but
> itself to run, but still requires protobuf-compiler for development. May be it
> is a good idea to split the protobuf SRPM into to SRPMS: protobuf and
> python-protobuf. The first one will contain everything except python
> subpackage, and the latter will contain only python part and will have
> BuildArch: noarch. Still I'm not sure it is a good idea, because these SRPMS
> will contain exactly the same sources.

I don't think it makes sense to have two SRPMS. Can't you just change the -python subpackage to not depend on the base package?

Comment 15 David Fraser 2008-09-30 08:05:24 UTC
Sorry, just saw that it doesn't depend on the base package :-)

Comment 16 Kaj J. Niemi 2008-10-06 09:02:49 UTC
Created attachment 319527 [details]
pkgconfig for 2.0.2

protobuf.pc.in with library version 2.0.2

Comment 17 Kaj J. Niemi 2008-10-06 09:03:46 UTC
Created attachment 319528 [details]
protobuf-pkgconfig-autotools.patch updated for 2.0.2

Comment 18 Kaj J. Niemi 2008-10-06 09:05:29 UTC
Created attachment 319530 [details]
protobuf 2.0.2 patch diff to 2.0.1

- make compiling with python optional (rhel4 has 2.3 which isn't good enough or something)
- build java implementation
- build java implementation with gcj

feel free to change the defaults or make them nicer, thanks ;-)

Comment 19 Kaj J. Niemi 2008-10-06 09:13:20 UTC
Created attachment 319531 [details]
protobuf 2.0.2 patch diff to 2.0.1

working patch, previous was missing .jar at the end of symlink creation

Comment 20 Lev Shamardin 2008-10-12 11:40:47 UTC
Release of protobuf-2.0.2-1.fc8.src.rpm

SRPM URL: http://shamardin.googlepages.com/protobuf-2.0.2-1.fc8.src.rpm
Spec URL: http://shamardin.googlepages.com/protobuf.spec

Changes:
* Sun Oct 12 2008 Lev Shamardin <shamardin> - 2.0.2-1
- Update to version 2.0.2
- New -java and -javadoc subpackages.
- Options to disable building of -python and -java* subpackages

Additional comments:
- This release adds possibility to disable building of python and java* subpackages, but they are enabled by default.
- Java subpackages are built with maven2 according to Fedora packaging guidelines.
- Original java sources depend on easymock for the tests which is not yet packaged for Fedora, so I had to remove all self testing stuff from java, but eventually it should be placed back. Any volunteers to package easymock for Fedora? 
- Requires java-devel >= 1.6.0 to build.

Waiting for the formal review...

Comment 21 Mamoru TASAKA 2008-10-19 18:52:35 UTC
Some notes for 2.0.2-1:

A. %description stage
* License
  - is actually BSD (also see CHANGES.txt)
    Perhaps you want to change the license of .pc.in
    file.

* (Build)Requires
  * For -devel subpackage
    - "Requires: protobuf" is redundant because protobuf-compiler
      already requires protobuf.

  * For -python subpackage
    - Would you explain why -python subpackage has "Requires
      python-setuptools"?

------------------------------------------------------
Additional remark about python subpackage:
The -python subpackage should not depend on the base package or any other
packages because it is a pure python implementation.
------------------------------------------------------
    - Well, for technical discussion, does this mean that there will
      be no problem even if the installed version of protobuf and
      protobuf-python differ? (if you don't write Requires this
      can happen).
      This discussion can be applied for -java subpackage.

  * For -java subpackage
    - About "BuildRequires: java-devel >= 1.6.0"
      -- If this means that Java binding needs OpenJDK to
         build, then this line must be
--------------------------------------------------------
BuildRequires: java-devel >= 1:1.6.0
--------------------------------------------------------
         java-devel vitrual Provides by java-1.6.0-openjdk-devel
         has Epoch 1 for historical reason (see:
         https://fedoraproject.org/wiki/Packaging/Java#BuildRequires_and_Requires )

      -- If either OpenJDK or icedtea can be used to build,
         then "BuildRequires: java-devel >= 1.7" is sufficient
         (Note that 1.7 < 1:1.6)

   * For -javadoc subpackage
     - "Requires: %{name}-%{version}-%{release}-java" is perhaps a typo.

* Shipping -static subpackage
  - Please explain why this package is needed where -devel
    subpackage is provided which includes .so symlink libraries.
    Usually static archives must be removed unless
    the package does not provide shared libraries.

B. %prep, %build, %instll, scriptlets stage:
* Timestamps
  - When using "cp" or "install" commands, add "-p" option to keep timestamps
    on installed files

  - Also this package uses "install-sh" when installing files. In such case
--------------------------------------------------------
make install DESTDIR=$RPM_BUILD_ROOT CPPROG="cp -p"
--------------------------------------------------------
    is useful to keep timestamps on installed files as much as possible.
    ('INSTALL="install -p"' option is usually tricks for Makefiles based
     on Makefiles generated by more recent Makefiles and not using install-sh)

* Internal libraries
  - This package uses "gtest" library to build. 
    This is already provided as "gtest" rpm on Fedora system-widely so
    this package must be patched to use system-wide gtest library.

* Unneeded /sbin/ldconfig call
  - There is no need that -devel subpackage should call /sbin/ldconfig on
    post/postun scriptlets.

C. %files entry
* Macros
  - Please refer to
    https://fedoraproject.org/wiki/Packaging/RPMMacros
    %_prefix/include must be %_includedir.

* For -devel subpackage
--------------------------------------------------------
[tasaka1@localhost ~]$ rpm -qf /usr/include/google/protobuf/
protobuf-devel-2.0.2-1.fc10.i386
[tasaka1@localhost ~]$ rpm -qf /usr/include/google/         
file /usr/include/google is not owned by any package
--------------------------------------------------------
  - Well, it seems that some other packages also install files
    under %_includedir/google, however when installing this -devel
    subpackage (and its dependency rpms), %_includedir/google
    is not owned by any packages.
    So for now %_includedir/google must also be owned by 
    this rpm.

* For -python subpackage
--------------------------------------------------------
%files python -f python/INSTALLED_FILES
--------------------------------------------------------
  - This method (i.e. listing %files entry by using INSTALLED_FILES
    created by setup.py) is forbidden on Fedora.
    With this method directory ownership issue is frequently ignored
    (and actually for this case):

    https://fedoraproject.org/wiki/Packaging/Python#System_Architecture
    https://fedoraproject.org/wiki/Packaging/UnownedDirectories

!!For -vim subpackage
  ! Neither of %_datadir/vim/vimfiles/{ftdetect,syntax} are owned
    by any packages, however I will ask vim maintainer about this.

* For -java subpackage
  - %{_datadir}/maven2/poms, %{_mavendepmapfragdir} are already
    owned by jpackage-utils and this package should not own
    these directories 
    (in this point packaging guidelines is a bit wrong).

Comment 22 Mamoru TASAKA 2008-10-30 16:55:36 UTC
ping?

Comment 23 Lev Shamardin 2008-10-31 14:07:22 UTC
(In reply to comment #21)
> Some notes for 2.0.2-1:
> 
> A. %description stage
> * License
>   - is actually BSD (also see CHANGES.txt)
>     Perhaps you want to change the license of .pc.in
>     file.
> 
> * (Build)Requires
>   * For -devel subpackage
>     - "Requires: protobuf" is redundant because protobuf-compiler
>       already requires protobuf.

Fixed this.

> 
>   * For -python subpackage
>     - Would you explain why -python subpackage has "Requires
>       python-setuptools"?
> 

This was a typo. Fixed.

> ------------------------------------------------------
> Additional remark about python subpackage:
> The -python subpackage should not depend on the base package or any other
> packages because it is a pure python implementation.
> ------------------------------------------------------
>     - Well, for technical discussion, does this mean that there will
>       be no problem even if the installed version of protobuf and
>       protobuf-python differ? (if you don't write Requires this
>       can happen).
>       This discussion can be applied for -java subpackage.

From my point of view, the only possible problem is that someone can
finish using newer protobuf-compiler with older python/java
bindings. Both java and python implementations are usable as a runtime
without any C++ code, you only need corresponding version of
protobuf-compiler for development.

> 
>   * For -java subpackage
>     - About "BuildRequires: java-devel >= 1.6.0"
>       -- If this means that Java binding needs OpenJDK to
>          build, then this line must be
> --------------------------------------------------------
> BuildRequires: java-devel >= 1:1.6.0
> --------------------------------------------------------
>          java-devel vitrual Provides by java-1.6.0-openjdk-devel
>          has Epoch 1 for historical reason (see:

Unfortunately I'm not that familiar with java development and
packaging. The problem is that if I just add 'BuildRequires:
java-devel' the build fails at some point with errors like:

-------------------------------------------------------
1. ERROR in /builddir/build/BUILD/protobuf-2.0.2/java/src/main/java/com/google/p
rotobuf/TextFormat.java (at line 438)
        while (pos < matcher.regionStart()) {
                             ^^^^^^^^^^^
The method regionStart() is undefined for the type Matcher
-------------------------------------------------------

On my Fedora 8 system BuildRequires: java-devel brings 
java-1.5.0-gcj-devel-1.5.0.0-17.fc8.i386 in mock build.

I can successfully build the package with
java-1.6.0-sun-devel-1.6.0.3-1jpp without mock on my system, and
BuildRequires: java-devel >= 1.6 brings
java-1.7.0-icedtea-devel-1.7.0.0-0.19.b21.snapshot.fc8.i586 which
builds the package succesfully.

>         
> https://fedoraproject.org/wiki/Packaging/Java#BuildRequires_and_Requires )
> 
>       -- If either OpenJDK or icedtea can be used to build,
>          then "BuildRequires: java-devel >= 1.7" is sufficient
>          (Note that 1.7 < 1:1.6)

I'm not sure if this is a good idea. This requirement in general makes
it impossible to build the package with
java-1.6.0-sun-devel-1.6.0.3-1jpp, while java-devel >= 1.6 brings
correct dependencies in mock and allows to build the package with sun
java 1.6

> 
>    * For -javadoc subpackage
>      - "Requires: %{name}-%{version}-%{release}-java" is perhaps a typo.

Fixed.

> 
> * Shipping -static subpackage
>   - Please explain why this package is needed where -devel
>     subpackage is provided which includes .so symlink libraries.
>     Usually static archives must be removed unless
>     the package does not provide shared libraries.

I just think this can be handy, and I generally disagree with the
policy of packaging only shared libraries for development. There are
some cases when you need static libraries, and for these cases having
static libraries packaged saves you from rebuilding the required
libraries yourself. I see no problems here, since normally developer
will install -devel and not -static subpackage.

> B. %prep, %build, %instll, scriptlets stage:
> * Timestamps
>   - When using "cp" or "install" commands, add "-p" option to keep timestamps
>     on installed files
> 
>   - Also this package uses "install-sh" when installing files. In such case
> --------------------------------------------------------
> make install DESTDIR=$RPM_BUILD_ROOT CPPROG="cp -p"
> --------------------------------------------------------
>     is useful to keep timestamps on installed files as much as possible.
>     ('INSTALL="install -p"' option is usually tricks for Makefiles based
>      on Makefiles generated by more recent Makefiles and not using install-sh)

Fixed.

> 
> * Internal libraries
>   - This package uses "gtest" library to build. 
>     This is already provided as "gtest" rpm on Fedora system-widely so
>     this package must be patched to use system-wide gtest library.

Working on this. Will update the ticket as soon as it is ready.

> 
> * Unneeded /sbin/ldconfig call
>   - There is no need that -devel subpackage should call /sbin/ldconfig on
>     post/postun scriptlets.

This was a typo. Fixed.

> 
> C. %files entry
> * Macros
>   - Please refer to
>     https://fedoraproject.org/wiki/Packaging/RPMMacros
>     %_prefix/include must be %_includedir.

Fixed.

> 
> * For -devel subpackage
> --------------------------------------------------------
> [tasaka1@localhost ~]$ rpm -qf /usr/include/google/protobuf/
> protobuf-devel-2.0.2-1.fc10.i386
> [tasaka1@localhost ~]$ rpm -qf /usr/include/google/         
> file /usr/include/google is not owned by any package
> --------------------------------------------------------
>   - Well, it seems that some other packages also install files
>     under %_includedir/google, however when installing this -devel
>     subpackage (and its dependency rpms), %_includedir/google
>     is not owned by any packages.
>     So for now %_includedir/google must also be owned by 
>     this rpm.

Done.

> 
> * For -python subpackage
> --------------------------------------------------------
> %files python -f python/INSTALLED_FILES
> --------------------------------------------------------
>   - This method (i.e. listing %files entry by using INSTALLED_FILES
>     created by setup.py) is forbidden on Fedora.
>     With this method directory ownership issue is frequently ignored
>     (and actually for this case):
> 
>     https://fedoraproject.org/wiki/Packaging/Python#System_Architecture
>     https://fedoraproject.org/wiki/Packaging/UnownedDirectories

Fixed.

> 
> !!For -vim subpackage
>   ! Neither of %_datadir/vim/vimfiles/{ftdetect,syntax} are owned
>     by any packages, however I will ask vim maintainer about this.
> 

Any news on this item?

> * For -java subpackage
>   - %{_datadir}/maven2/poms, %{_mavendepmapfragdir} are already
>     owned by jpackage-utils and this package should not own
>     these directories 
>     (in this point packaging guidelines is a bit wrong).

Fixed.

So finally,

Updated SPEC: http://shamardin.googlepages.com/protobuf.spec
New SRPM: http://shamardin.googlepages.com/protobuf-2.0.2-2.fc8.src.rpm

Changes:
* Fri Oct 31 2008 Lev Shamardin <shamardin> - 2.0.2-2
- Use python_sitelib macro instead of INSTALLED_FILES.
- Fix the license.
- Fix redundant requirement for -devel subpackage.
- Fix wrong dependences for -python subpackage.
- Fix typo in requirements for -javadoc subpackage.
- Use -p option for cp and install to preserve timestamps.
- Remove unneeded ldconfig call for post scripts of -devel subpackage.
- Fix directories ownership.

I will update the SPEC to use the packaged gtest in the next few days, could you please check for any other errors left meanwhile?

Comment 24 Rick L Vinyard Jr 2008-10-31 16:23:33 UTC
> > 
> > * Shipping -static subpackage
> >   - Please explain why this package is needed where -devel
> >     subpackage is provided which includes .so symlink libraries.
> >     Usually static archives must be removed unless
> >     the package does not provide shared libraries.
> 
> I just think this can be handy, and I generally disagree with the
> policy of packaging only shared libraries for development. There are
> some cases when you need static libraries, and for these cases having
> static libraries packaged saves you from rebuilding the required
> libraries yourself. I see no problems here, since normally developer
> will install -devel and not -static subpackage.

I would second that. They're particularly useful for libraries that are likely to be used in embedded environments.

Comment 25 Mamoru TASAKA 2008-11-01 13:22:35 UTC
(In reply to comment #23)
> (In reply to comment #21)
> > * Shipping -static subpackage
> >   - Please explain why this package is needed where -devel
> >     subpackage is provided which includes .so symlink libraries.
> >     Usually static archives must be removed unless
> >     the package does not provide shared libraries.
> 
> There are
> some cases when you need static libraries, 

Unless you provide the concrete case for this package I strongly
disagree (packaging guidelines say that the "compelling reason"
must be provided)
(If you still want I probably have to ask for FESCo:
https://fedoraproject.org/wiki/Packaging/Guidelines#Staticly_Linking_Executables
)

> and for these cases having
> static libraries packaged saves you from rebuilding the required
> libraries yourself. 

This is exactly why we think we must _not_ provide static archives unless
avoided.
Using static archives will cause problem when some security
issues or so are found in protobuf and people forget that they are
using old static protobuf archive, for example.

> > !!For -vim subpackage
> >   ! Neither of %_datadir/vim/vimfiles/{ftdetect,syntax} are owned
> >     by any packages, however I will ask vim maintainer about this.
> > 
> 
> Any news on this item?
Oops, completely forgotton, I will surely ask later...

> > ------------------------------------------------------
> > Additional remark about python subpackage:
> > The -python subpackage should not depend on the base package or any other
> > packages because it is a pure python implementation.
> > ------------------------------------------------------
> >     - Well, for technical discussion, does this mean that there will
> >       be no problem even if the installed version of protobuf and
> >       protobuf-python differ? (if you don't write Requires this
> >       can happen).
> >       This discussion can be applied for -java subpackage.
> 
> From my point of view, the only possible problem is that someone can
> finish using newer protobuf-compiler with older python/java
> bindings. Both java and python implementations are usable as a runtime
> without any C++ code, you only need corresponding version of
> protobuf-compiler for development.

Then you should ensure that the trouble you mentioned here won't happen.
* One method is to make -compiler subpackage have:
-----------------------------------------------------
Conflicts: %{name}-java < %{version}
Conflicts: %{name}-java > %{version}
-----------------------------------------------------
or so.
 
> >   * For -java subpackage
> >     - About "BuildRequires: java-devel >= 1.6.0"
> >       -- If this means that Java binding needs OpenJDK to
> >          build, then this line must be
> > --------------------------------------------------------
> > BuildRequires: java-devel >= 1:1.6.0
> > --------------------------------------------------------
> >          java-devel vitrual Provides by java-1.6.0-openjdk-devel
> >          has Epoch 1 for historical reason (see:
> 
 
> I can successfully build the package with
> java-1.6.0-sun-devel-1.6.0.3-1jpp without mock on my system, and
> BuildRequires: java-devel >= 1.6 brings
> java-1.7.0-icedtea-devel-1.7.0.0-0.19.b21.snapshot.fc8.i586 which
> builds the package succesfully.

Well, actually I don't care about RHEL4, however for this case
I can allow "java-devel > 1.6".

Comment 26 Mamoru TASAKA 2008-11-01 15:44:08 UTC
Well, for -2:

* License
  - Well protobuf.pc.in is still under ASL 2.0
    You should ask the author to change the license
    of this file
    (well, actually I have no idea why this pkgconfig
     file has license term, first of all...)
    Fortunately currently the author is in CC list
    of this bug. Rick, would you agree to change .pc.in
    file you wrote to be under BSD or to remove license
    term completely?

* BuildRequires
  - This package won't build without 
    "BuildRequires: python-setuptools-devel" (note: here
    I don't say about Requires).

* Requires
  - "Requires: %{name}-java-%{version}-%{release}" should be
    "Requires: %{name}-java = %{version}-%{release}"

* rpmlint issue
** non-standard-group
  - Group "Development/Documentation" should simply be
    "Documentation".

** non-executable-script
------------------------------------------------------------
E: non-executable-script /usr/lib/python2.5/site-packages/google/protobuf/descriptor_pb2.py 0644
------------------------------------------------------------
   - If this script are not meant to be executed by user directly,
     then this script must not have shebang (anyway the shebang
     #!/usr/bin/python2.4 is wrong because we use python 2.5)

Comment 27 Rick L Vinyard Jr 2008-11-02 19:14:59 UTC
(In reply to comment #26)
> Well, for -2:
> 
> * License
>   - Well protobuf.pc.in is still under ASL 2.0
>     You should ask the author to change the license
>     of this file
>     (well, actually I have no idea why this pkgconfig
>      file has license term, first of all...)
>     Fortunately currently the author is in CC list
>     of this bug. Rick, would you agree to change .pc.in
>     file you wrote to be under BSD or to remove license
>     term completely?

I didn't realize I had left the license in the header... was just copying and pasting. The license should be removed completely... there's nothing there to license... it's just a pkgconfig file.

Comment 28 Lev Shamardin 2008-11-11 13:54:30 UTC
(In reply to comment #25)
> Unless you provide the concrete case for this package I strongly
> disagree (packaging guidelines say that the "compelling reason"
> must be provided)
> (If you still want I probably have to ask for FESCo:
> https://fedoraproject.org/wiki/Packaging/Guidelines#Staticly_Linking_Executables
> )

The link above prohibits static linking of executables, and I
completely agree with this policy. But I don't understand how does
this prohibit providing -static library packages without any
statically linked executables.

> > and for these cases having
> > static libraries packaged saves you from rebuilding the required
> > libraries yourself. 
> 
> This is exactly why we think we must _not_ provide static archives unless
> avoided.
> Using static archives will cause problem when some security
> issues or so are found in protobuf and people forget that they are
> using old static protobuf archive, for example.

Are we supposed to fix all potential security issues in the universe?
Potential security issues in fedora packages are effectively handled
in this case with the policy of prohibiting static linking of
*executables*, because in this case the number of statically linked
packages in fedora is minimal. But if the local administrator decides
to build something locally against a -static library, he surely has a
Reason to do this, and he has to perform some additional non-standard
steps to do this (install -static subpackage), and of course he
understands the drawbacks. If I want to make a statically linked
executable for my local system not providing a -static subpackage just
adds some additional inconvenience for me, but will never stop from
static linkage, because I will simply build the static library myself.

To summarize, there are no statically linked *executables* in
protobuf-* packages, so I think that the policy is fulfilled.

> 
> > > !!For -vim subpackage
> > >   ! Neither of %_datadir/vim/vimfiles/{ftdetect,syntax} are owned
> > >     by any packages, however I will ask vim maintainer about this.
> > > 
> > 
> > Any news on this item?
> Oops, completely forgotton, I will surely ask later...
> 
> > > ------------------------------------------------------
> > > Additional remark about python subpackage:
> > > The -python subpackage should not depend on the base package or any other
> > > packages because it is a pure python implementation.
> > > ------------------------------------------------------
> > >     - Well, for technical discussion, does this mean that there will
> > >       be no problem even if the installed version of protobuf and
> > >       protobuf-python differ? (if you don't write Requires this
> > >       can happen).
> > >       This discussion can be applied for -java subpackage.
> > 
> > From my point of view, the only possible problem is that someone can
> > finish using newer protobuf-compiler with older python/java
> > bindings. Both java and python implementations are usable as a runtime
> > without any C++ code, you only need corresponding version of
> > protobuf-compiler for development.
> 
> Then you should ensure that the trouble you mentioned here won't happen.
> * One method is to make -compiler subpackage have:
> -----------------------------------------------------
> Conflicts: %{name}-java < %{version}
> Conflicts: %{name}-java > %{version}
> -----------------------------------------------------
> or so.

I've added 
Conflicts: %{name}-compiler > %{version}
Conflicts: %{name}-compiler < %{version}
to -java and -python subpackages.

(In reply to comment #26)
> Well, for -2:
> 
> * License
>   - Well protobuf.pc.in is still under ASL 2.0
>     You should ask the author to change the license
>     of this file

I removed the license header from the file, leaving only Copyright
notice, as the author suggested.

> * BuildRequires
>   - This package won't build without 
>     "BuildRequires: python-setuptools-devel" (note: here
>     I don't say about Requires).

Fixed. However this is strange, since it did build successfully in
mock on my Fedora 8 system.

> 
> * Requires
>   - "Requires: %{name}-java-%{version}-%{release}" should be
>     "Requires: %{name}-java = %{version}-%{release}"

Fixed.

> 
> * rpmlint issue
> ** non-standard-group
>   - Group "Development/Documentation" should simply be
>     "Documentation".

Fixed.

> 
> ** non-executable-script
> ------------------------------------------------------------
> E: non-executable-script
> /usr/lib/python2.5/site-packages/google/protobuf/descriptor_pb2.py 0644
> ------------------------------------------------------------
>    - If this script are not meant to be executed by user directly,
>      then this script must not have shebang (anyway the shebang
>      #!/usr/bin/python2.4 is wrong because we use python 2.5)

This was in the protoc-generated code. It should be properly fixed in
the upstream code, I've submitted a bug report:
http://code.google.com/p/protobuf/issues/detail?id=56

Meanwhile, I've modified the %build step to fix this.

I've finally converted this package to use gtest library.

Updated SPEC: http://shamardin.googlepages.com/protobuf.spec
New SRPM: http://shamardin.googlepages.com/protobuf-2.0.2-3.fc8.src.rpm

* Tue Nov 11 2008 Lev Shamardin <shamardin> - 2.0.2-3
- Added conflicts to java and python subpackages to prevent using with
  wrong compiler versions.
- Fixed license.
- Fixed BuildRequires for -python subpackage.
- Fixed Requires and Group for -javadoc subpackage.
- Fixed Requires for -devel subpackage.
- Fixed issue with wrong shebang in descriptor_pb2.py.
- Specify build options via --with/--without.
- Use Fedora-packaged gtest library instead of a bundled one by
  default (optional).

Comment 29 Mamoru TASAKA 2008-11-13 17:47:45 UTC
Well,
+ As you seem to have reason I will admit -static subpackage for this package
+ This package itself is now in good shape.

Then:
-------------------------------------------------------------
NOTE: Before being sponsored:

This package will be accepted with another few work. 
But before I accept this package, someone (I am a candidate) 
must sponsor you.

Once you are sponsored, you have the right to review other 
submitters' review requests and approve the packages formally. 
For this reason, the person who want to be sponsored (like you) 
are required to "show that you have an understanding 
of the process and of the packaging guidelines" as is described
on :
http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored

Usually there are two ways to show this.
A. submit other review requests with enough quality.
B. Do a "pre-review" of other person's review request
   (at the time you are not sponsored, you cannot do
   a formal review)

When you have submitted a new review request or have pre-reviewed other 
person's review request, please write the bug number on this bug report 
so that I can check your comments or review request.

Fedora package collection review requests which are waiting for someone to
review can be checked on:
http://fedoraproject.org/PackageReviewStatus/NEW.html
(NOTE: please don't choose "Merge Review")


Review guidelines are described mainly on:
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
http://fedoraproject.org/wiki/Packaging/Guidelines
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
------------------------------------------------------------

Comment 30 John A. Khvatov 2008-11-17 17:19:05 UTC
protobuf-2.0.2-3:
Build filed on my Fedora 9, x86_64 such as:
http://code.google.com/p/protobuf/issues/detail?id=45.

Source update needed.

Comment 31 Mamoru TASAKA 2008-11-20 18:35:06 UTC
ping?

Comment 32 Lev Shamardin 2008-11-22 10:02:59 UTC
(In reply to comment #30)
> protobuf-2.0.2-3:
> Build filed on my Fedora 9, x86_64 such as:
> http://code.google.com/p/protobuf/issues/detail?id=45.

I have included the patch from svn to the package. Can you please check if it builds succesfully now?

New SPEC: http://shamardin.googlepages.com/protobuf.spec
New SRPM: http://shamardin.googlepages.com/protobuf-2.0.2-4.fc8.src.rpm 

Changelog:
* Sat Nov 22 2008 Lev Shamardin <shamardin> - 2.0.2-4
- Added patch from subversion r70 to workaround gcc 4.3.0 bug (see
  http://code.google.com/p/protobuf/issues/detail?id=45 for more
  details).

Comment 33 John A. Khvatov 2008-11-22 11:07:23 UTC
(In reply to comment #32)

> New SPEC: http://shamardin.googlepages.com/protobuf.spec
> New SRPM: http://shamardin.googlepages.com/protobuf-2.0.2-4.fc8.src.rpm 
> 
> Changelog:
> * Sat Nov 22 2008 Lev Shamardin <shamardin> - 2.0.2-4
> - Added patch from subversion r70 to workaround gcc 4.3.0 bug (see
>   http://code.google.com/p/protobuf/issues/detail?id=45 for more
>   details).

It builds and works fine, thanks.

Comment 34 Lev Shamardin 2008-11-22 12:33:19 UTC
I've made a pre-review here: https://bugzilla.redhat.com/show_bug.cgi?id=438811

Comment 35 Mamoru TASAKA 2008-11-23 17:01:59 UTC
Okay

+ Now this package itself is good
+ Your pre-review seems good for initial comments

-------------------------------------------------------------
    This package (protobuf) is APPROVED by mtasaka
-------------------------------------------------------------

Please follow the procedure written on:
http://fedoraproject.org/wiki/PackageMaintainers/Join
from "Get a Fedora Account".
After you request for sponsorship a mail will be sent to sponsor 
members automatically (which is invisible for you) which notifies 
that you need a sponsor. After that, please also write on
this bug for confirmation that you requested for sponsorship and
your FAS (Fedora Account System) name. Then I will sponsor you.

If you want to import this package into Fedora 9/10, you also have
to look at
http://fedoraproject.org/wiki/Infrastructure/UpdatesSystem/Bodhi-info-DRAFT
(after once you rebuilt this package on koji Fedora rebuilding system).

If you have questions, please ask me.

Comment 36 Lev Shamardin 2008-11-23 17:09:26 UTC
I have filed an application to the packagers group, my FAS login name is 'abbot'.

Comment 37 Mamoru TASAKA 2008-11-23 17:13:47 UTC
Confirmed. Now I am sponsoring you. Please follow "Join" wiki again.

Comment 38 Lev Shamardin 2008-11-24 10:03:58 UTC
New Package CVS Request
=======================
Package Name: protobuf
Short Description: Protocol Buffers - Google's data interchange format
Owners: abbot
Branches: F-8 F-9
InitialCC: abbot

Comment 39 Dennis Gilmore 2008-11-25 16:39:03 UTC
CVS Done,  no need to put an owner in the initialCC list

Comment 40 Fedora Update System 2008-11-27 12:10:01 UTC
protobuf-2.0.2-5.fc8 has been submitted as an update for Fedora 8.
http://admin.fedoraproject.org/updates/protobuf-2.0.2-5.fc8

Comment 41 Fedora Update System 2008-11-27 12:17:19 UTC
protobuf-2.0.2-5.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/protobuf-2.0.2-5.fc9

Comment 42 Lev Shamardin 2008-11-27 12:22:07 UTC
Package Change Request
======================
Package Name: protobuf
New Branches: F-10

Comment 43 Kevin Fenzi 2008-12-01 20:42:07 UTC
cvs done.

Comment 44 Fedora Update System 2008-12-02 07:23:48 UTC
protobuf-2.0.2-5.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/protobuf-2.0.2-5.fc10

Comment 45 Mamoru TASAKA 2008-12-02 13:41:35 UTC
Okay, thanks.

Comment 46 Lev Shamardin 2008-12-04 07:41:44 UTC
Dear all, If you were interested in this package, can you please test it from updates-testing in Fedora 8 and/or Fedora 9 and provide feedback in bodhi here at these urls?

https://admin.fedoraproject.org/updates/F8/FEDORA-2008-10531
https://admin.fedoraproject.org/updates/F9/FEDORA-2008-10527

I would really like to have someone except me to check that the package works fine.

Comment 47 Fedora Update System 2008-12-08 13:01:09 UTC
protobuf-2.0.2-5.fc8 has been pushed to the Fedora 8 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 48 Fedora Update System 2008-12-08 13:04:03 UTC
protobuf-2.0.2-5.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 49 Fedora Update System 2008-12-08 13:04:17 UTC
protobuf-2.0.2-5.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.


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