Bug 754975 - Review Request: pion-net - Pion Network Library
Summary: Review Request: pion-net - Pion Network Library
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Michael Schwendt
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 752020
TreeView+ depends on / blocked
 
Reported: 2011-11-18 12:34 UTC by Jan Vcelak
Modified: 2013-03-04 01:29 UTC (History)
4 users (show)

Fixed In Version: pion-net-4.0.7-4.fc17
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-11-28 10:48:56 UTC
Type: ---
bugs.michael: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Jan Vcelak 2011-11-18 12:34:06 UTC
Spec URL:
http://jvcelak.fedorapeople.org/review/pion-net/pion-net.spec

SRPM URL:
http://jvcelak.fedorapeople.org/review/pion-net/pion-net-4.0.7-1.fc17.src.rpm

Builds for F17:
http://jvcelak.fedorapeople.org/review/pion-net/x86_64/
http://jvcelak.fedorapeople.org/review/pion-net/i686/

Description:
Pion Network Library is a C++ framework for building lightweight HTTP interfaces.

$ rpmlint pion-net.spec pion-net-4.0.7-1.fc17.src.rpm
1 packages and 1 specfiles checked; 0 errors, 0 warnings.

$ rpmlint pion-net-4.0.7-1.fc17.x86_64.rpm pion-net-doc-4.0.7-1.fc17.noarch.rpm pion-net-devel-4.0.7-1.fc17.x86_64.rpm pion-net-debuginfo-4.0.7-1.fc17.x86_64.rpm
pion-net-doc.noarch: E: devel-dependency pion-net-devel
pion-net-devel.x86_64: W: no-documentation
4 packages and 0 specfiles checked; 1 errors, 1 warnings.

> pion-net-doc.noarch: E: devel-dependency pion-net-devel
> pion-net-devel.x86_64: W: no-documentation

Package pion-net-doc contains only development documentation, therefore depends on devel package.

Comment 1 Michael Schwendt 2011-11-19 11:40:18 UTC
Just a brief look at the spec file:


> Summary:	Pion Network Library

Please sum up what it does not what it is called. For example:

Sumamry: C++ library for building lightweight HTTP interfaces


> %package	devel
> Summary:	Development files for pion-net
> Requires:	pion-net = %{version}-%{release}

https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package


> %package	doc
> Summary:	Documentation for pion-net
> Requires:	pion-net-devel = %{version}-%{release}

It's a bad habit to let documentation depend on -devel packages. It makes it more difficult (if not impossible) to create a collection of documentation packages, because they require growing -devel trees to be installable. Surely one can read the docs without installing the -devel files.


> %install
> rm -rf %{buildroot}

https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag


> %files
> %doc AUTHORS COPYING NEWS TODO
> %{_libdir}/libpion-*.so
> %{_libdir}/pion/plugins/*.so

https://fedoraproject.org/wiki/Packaging:UnownedDirectories

Comment 2 Jan Vcelak 2011-11-22 09:35:53 UTC
(In reply to comment #1)
> Just a brief look at the spec file:

Thank you for catching all these -- everything is fixed.

Updated spec URL:
http://jvcelak.fedorapeople.org/review/pion-net/4.0.7-2/pion-net.spec

Updated SRPM URL:
http://jvcelak.fedorapeople.org/review/pion-net/pion-net-4.0.7-2.fc17.src.rpm

Comment 3 Michael Schwendt 2011-11-22 22:33:58 UTC
* F-16 and F-15 builds complete for both x86_64 and i686.


* HTML documentation is also included in the base package. This is because the %doc macro for the base package installs the files in %_docdir/%name-%version, which is exactly the path you use in the -doc subpackage. In the same way, the -doc subpkg duplicates the %doc files from the base pkg %files list.


* -devel package is missing dependencies. In the C++ headers I see references to Boost headers, OpenSSL, log4cpp. It's likely one cannot include pion-net headers before installing missing -devel packages.


* The pkg-config file is broken in several ways:
 - on x86_64, it includes a hardcoded -L/usr/lib
 - its Libs definition duplicates the linker args that have been used to link pion-net itself, not what is necessary to link with pion-net
 - the Cflags line needs a second look, in particular where the hardcoded BOOST_FILESYSTEM_VERSION definition comes from (version 2 is deprecated)
 - $datarootdir and $datadir are undefined/unexpanded


* Source doesn't use Fedora's global compiler %optflags.

Comment 4 Jan Vcelak 2011-11-23 14:36:35 UTC
Update spec URL:
http://jvcelak.fedorapeople.org/review/pion-net/4.0.7-3/pion-net.spec

Updated SRPM url:
http://jvcelak.fedorapeople.org/review/pion-net/pion-net-4.0.7-3.fc17.src.rpm

(In reply to comment #3)
> * HTML documentation is also included in the base package. This is because the
> %doc macro for the base package installs the files in %_docdir/%name-%version,
> which is exactly the path you use in the -doc subpackage. In the same way, the
> -doc subpkg duplicates the %doc files from the base pkg %files list.

Resolved. Documentation is now in %{_docdir}/pion-net-doc-%{version}

> * -devel package is missing dependencies. In the C++ headers I see references
> to Boost headers, OpenSSL, log4cpp. It's likely one cannot include pion-net
> headers before installing missing -devel packages.

Resolved. Added the same -devel libs.

> * The pkg-config file is broken in several ways:
>  - on x86_64, it includes a hardcoded -L/usr/lib
>  - its Libs definition duplicates the linker args that have been used to link
> pion-net itself, not what is necessary to link with pion-net
>  - the Cflags line needs a second look, in particular where the hardcoded

I'm not a pkg-config guru. I have updated the .pc file to include only pion-net libraries in Libs. And added Libs.private to include the external libraries. I hope it is correct.

> BOOST_FILESYSTEM_VERSION definition comes from (version 2 is deprecated)
>  - $datarootdir and $datadir are undefined/unexpanded

That's right. The library is not compatible with new boost::filesystem. But I updated the affected parts, there were only a few. The updated package links against boost::filesystems v3 successfully.

> * Source doesn't use Fedora's global compiler %optflags.

Resolved, I patched the autoconf files not to clear the flags.

I found another issues, which were fixed as well:
- A typo in configure option (--with-pic).
- libicu was hardcoded in autoconf. I removed it and libicu-devel in BuildRequires is no longer needed.
- Unversioned .so files were moved into the -devel package.

Comment 5 Jan Vcelak 2011-11-23 14:39:14 UTC
(In reply to comment #3)
>

And thank you, Michael!

Comment 6 Jan Vcelak 2011-11-25 22:21:06 UTC
Michael, are you going to review this package? Or should I offer someone a review swap?

Comment 7 Michael Schwendt 2011-11-27 15:18:49 UTC
[pkg-config file changes]

> I hope it is correct.

Yes, it makes sense to move the external libs into Libs.private. Just that at Fedora we try to avoid static linking, so it might be that some of the libs in Libs.private are not available as -static packages.


> - A typo in configure option (--with-pic).

:)  Didn't make a difference, though. The default pic mode also compiled with -fPIC -DPIC.


> - libicu was hardcoded in autoconf. I removed it and libicu-devel in
> BuildRequires is no longer needed.

Confirmed. BuildRequires now match the list at
http://www.pion.org/projects/pion-network-library/libraries
however zlib and bzip2 dependency is a mystery, too. They seem to be unused but are linked with.

$ grep zlib * -R|grep '#include'
common/build/pion-config.inc:	AC_TRY_LINK([#include <zlib.h>],[ zlibVersion(); return(0); ],
common/build/pion-config.inc:	AC_TRY_LINK([#include <bzlib.h>],[ BZ2_bzlibVersion(); return(0); ],
configure:#include <zlib.h>
configure:#include <bzlib.h>


> - Unversioned .so files were moved into the -devel package.

Confirmed.


* Exlicit -devel Requires could be arch-specific using %{?_isa}.


* Upstream download section offers a shorter .tar.bz2 source archive.


* APPROVED

Comment 8 Jan Vcelak 2011-11-27 20:33:35 UTC
(In reply to comment #7)
> * APPROVED

Thank you.

Comment 9 Jan Vcelak 2011-11-27 20:37:43 UTC
New Package SCM Request
=======================
Package Name: pion-net
Short Description: C++ library for building lightweight HTTP interfaces
Owners: jvcelak
Branches: f15 f16
InitialCC:

Comment 10 Gwyn Ciesla 2011-11-27 22:04:33 UTC
Git done (by process-git-requests).

Comment 11 Fedora Update System 2011-11-28 10:46:49 UTC
pion-net-4.0.7-4.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/pion-net-4.0.7-4.fc15

Comment 12 Fedora Update System 2011-11-28 10:48:11 UTC
pion-net-4.0.7-4.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/pion-net-4.0.7-4.fc16

Comment 13 Fedora Update System 2011-12-10 19:32:42 UTC
pion-net-4.0.7-4.fc15 has been pushed to the Fedora 15 stable repository.

Comment 14 Fedora Update System 2011-12-10 20:00:01 UTC
pion-net-4.0.7-4.fc16 has been pushed to the Fedora 16 stable repository.


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