[Bug 486804] Review Request: libferrisloki - customized build of Loki library from Modern C++ Design for libferris

[bugzilla at redhat.com]

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=486804


Michael Schwendt <bugs.michael at gmx.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bugs.michael at gmx.net




--- Comment #1 from Michael Schwendt <bugs.michael at gmx.net>  2009-03-16 12:39:17 EDT ---
Might be that I've caught all issues, but a lot of work is needed to bring this
into shape:


* Run "rpmlint -i" on your src.rpm and also on all built packages. Try to fix
as many Warnings and Errors as plausible.


> License: GPL

This is not just an invalid value for the "License" tag, it is inaccurate. Some
source files mention "GPLv2+", some the "Boost Software License 1.0". Others
contain a "Copyright Only" header:
https://fedoraproject.org/wiki/Licensing/CopyrightOnly
The "macros/ferrismacros.m4" file contains pieces licenced under the "LGPLv2+".

https://fedoraproject.org/wiki/Licensing


> Source: http://prdownloads.sourceforge.net/witme/%{name}-%{version}.tar.bz2

There are special guidelines for Sourceforge.net download locations:
https://fedoraproject.org/wiki/Packaging/SourceURL#Sourceforge.net


> Packager: Ben Martin <monkeyiq at users.sourceforge.net>

Don't set this tag. The build-system will do it. In general, be careful with
hardcoding "Packager"/"Vendor" tags in spec files you release. There are people
who build broken binary rpms, which would appear as if they have been built by
you, because they contain your name in the "Packager" tag. The spec %changelog
is less of a problem in case you wonder.


> BuildRequires: gcc-c++

Redundant, as the C++ compiler is available in the minimal build environment:
https://fedoraproject.org/wiki/Packaging:Guidelines#Exceptions_2


> if [ "$SMP" != "" ]; then
>   (make "MAKE=make -k -j $SMP"; exit 0)
>   make
> else
>   make
> fi

At least with Fedora, you can replace this with just:

make %{?_smp_mflags}


> %install
> %makeinstall

First command in %install section must be: rm -rf $RPM_BUILD_ROOT
(or "rm -rf %buildroot" if you prefer the lower case macro everywhere)

make DESTDIR="$RPM_BUILD_ROOT" install
or:
make DESTDIR="$RPM_BUILD_ROOT" INSTALL="install -p"

shall be preferred over %makeinstall.


> %files
> %defattr(-,root,root,0755)

Doesn't %defattr(-,root,root,-) work?

> %doc AUTHORS README COPYING ChangeLog INSTALL

Typically, the standard file "INSTALL" is irrelevant to RPM package users. Here
it is empty even.

> %{_libdir}/*
> %{_includedir}/*

Package must be split into a main library pkg and a ferrisloki-devel
sub-package, which contains the files needed only for software development
(i.e. the *.so symlink and the headers).

%{_libdir}/*  includes too many files it must not include (e.g. the debuginfo
files). Use at most  %{_libdir}/*.so.*   for the main pkg and %{_libdir}/*.so 
for the -devel subpkg.


> -rw-r--r--  /usr/lib/libferrisloki.a
> -rwxr-xr-x  /usr/lib/libferrisloki.la

Don't build/include these.
https://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries


> -rw-r--r--  /usr/include/FerrisLoki/Extensions.cpp
> -rw-r--r--  /usr/include/FerrisLoki/OrderedStatic.cpp
> -rw-r--r--  /usr/include/FerrisLoki/SafeFormat.cpp
> -rw-r--r--  /usr/include/FerrisLoki/Singleton.cpp
> -rw-r--r--  /usr/include/FerrisLoki/SmallObj.cpp
> -rw-r--r--  /usr/include/FerrisLoki/SmartPtr.cpp
> -rw-r--r--  /usr/include/FerrisLoki/StrongPtr.cpp

Hmm?


* The pkgconfig file ferrisloki.pc is questionable, because it is tuned for
static linking and does a few bad things:

> Libs: -L${libdir} -lferrisloki  -lsigc-2.0  
The shared library is linked with libsigc-2.0 already. No need to link again.

> Requires: 

The pkgconfig dependency on "sigc++-2.0" is missing in this field if you really
want it to be a strict dependency - I doubt you want it. It would also add the
proper libsigc++20 CFLAGS and LDFLAGS automatically when running pkg-config,
and you would not need to add them to your .pc file manually.

For platforms older than Fedora 11, the ferrisloki-devel subpackage must
"Requires: libsigc++20-devel", however see below.

> Cflags: -I${includedir} -I${includedir}/FerrisLoki
> -I${includedir}/FerrisLoki/loki   -I/usr/include/sigc++-2.0
> -I/usr/lib/sigc++-2.0/include

Why do add two search paths for FerrisLoki and FerrisLoki/loki? There are
several files which include <loki/...>, so the extra search path is not needed.

The sigc++-2.0 related Cflags are redundant, if you would fill in the Requires
field correctly. However, only the boost extension uses sigc++20 headers. And
that extension would need "Requires: boost-devel" in the spec file.

In other words, I don't see why the sigc++ stuff is in the .pc file at all.

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