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@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|needinfo?(shamardin@gmail.c | |om) |
--- Comment #23 from Lev Shamardin shamardin@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@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@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?