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

Tue Mar 17 09:48:27 UTC 2009

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

--- Comment #3 from Ben Martin <monkeyiq at users.sf.net>  2009-03-17 05:48:25 EDT ---
Firstly, thank you for your indepth and very informative comments! A
few issues like not having split out a -devel package I forgot to do
before submitting and apologize for that.

I should mention up front that I've been using this spec on the
openSUSE build system (OBS) to make rpms for Fedora and openSUSE. If
possible I'd like to continue using the same spec in Fedora and in the
OBS to make openSUSE rpms too. I'm not sure if this is possible or
not, but I can always hope.

I've updated the spec file and src.rpm file with changes from your
comments. This should also make the next spec less of a hassle because
I'll have already addressed these comments.

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

There are still a few issues rpmlint picks up. The main one is how to
use multiple licenses properly.

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

OK, I've taken a look at these to see if I could consolidate things (if I owned
copyright on the file & code) but it looks like we'll need these three
liceneses
unless I plan to rewrite some already working code. So now we have:

License: LGPLv2+, Boost, Copyright only

I'm not sure how to chain them though. rpmlint doesn't seem to like comma
separated 
and rpmbuild doesn't like multiple tags.

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

Excellent. The above URL was one I came up with ages ago, though it is
surprisingly close to the Fedora recommendation. I've updated to fix
this inline with the URL above.

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

Removed. Though I also plan to try to use the same .spec with OBS to
make openSUSE rpms, so I might have to add it back conditionally.

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

Ah, if this redundancy is OK can we leave it? To build with OBS you
have to include the compiler too so I need that redundant line for the
spec to with on OBS too.

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

Done. If openSUSE doesn't actually set _smp_mflags I might add some
glue in the header of the spec to set it properly on OBS.

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

This is one area where I thought using the macro was the golden thing
to do.  Changed to the second version with the modification you
included.

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

Fixed.

> 
> > %{_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?

OK I split the package to include a -devel package and distribute the
libdir stuff as mentioned. The strange .cpp files are no longer
packaged.

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

Updated to remove the sigc++ stuff from the cflags and libs in the .pc
and use Requires for both sigc++ and boost instead.

The problem that lead to this is that I have added some extensions to
the libferrisloki library beyond what the mainline Loki library
has. These extensions are in

%{_includedir}/FerrisLoki/*.hh

and over time have grown to include sigc++ and boost support to make
all the higher level libraries that use libferrisloki easier to
write. Of course, if you don't #include any of these extensions then
sigc++ and boost are not strictly needed by libferrisloki. On the
other hand, since I'm the main (sole?) user of libferrisloki just
adding both sigc++ and boost to the requires should make things
cleaner.

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