[Bug 459088] Review Request: protobuf - Protocol Buffers - Google's data interchange format
bugzilla at redhat.com
bugzilla at redhat.com
Fri Oct 31 14:07:24 UTC 2008
Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=459088
Lev Shamardin <shamardin at gmail.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Flag|needinfo?(shamardin at gmail.c |
|om) |
--- Comment #23 from Lev Shamardin <shamardin at gmail.com> 2008-10-31 10:07:22 EDT ---
(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 at localhost ~]$ rpm -qf /usr/include/google/protobuf/
> protobuf-devel-2.0.2-1.fc10.i386
> [tasaka1 at 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 at gmail.com> - 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?
--
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.
More information about the Fedora-package-review
mailing list