[Bug 227669] Review Request: <ppl-0.9> - <A modern C++ library providing numerical abstractions>

Sat Feb 10 00:27:52 UTC 2007

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Review Request: <ppl-0.9> - <A modern C++ library providing numerical abstractions>

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=227669

bugs.michael at gmx.net changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|Review Request: <ppl-0.9> - |Review Request: <ppl-0.9> -
                   |<A modern C++ library       |<A modern C++ library
                   |providing numerical         |providing numerical
                   |abstractions>               |abstractions>
OtherBugsDependingO|                            |177841
              nThis|                            |

------- Additional Comments From bugs.michael at gmx.net  2007-02-09 19:27 EST -------
The spec file needs *a lot* of work.

Let me try to cover many issues in this reply, although I'm afraid
I will fail to catch all due to the sheer number of packaging problems.
In particular, sub-packages are missing.

Run "rpmlint" with option -i on your src.rpm (and also the binary
rpms!):

$ rpmlint ppl-0.9-1.src.rpm 
W: ppl summary-ended-with-dot The Parma Polyhedra Library: a C++ library for
numerical abstractions.
E: ppl no-changelogname-tag
W: ppl invalid-license GPL v2
W: ppl hardcoded-packager-tag %{packager}
W: ppl hardcoded-prefix-tag /usr
W: ppl setup-not-quiet
W: ppl mixed-use-of-spaces-and-tabs (spaces: line 12, tab: line 1)

All findings are worth fixing.  "setup-not-quiet" can be ignored,
but when adding the -q option to the %setup line of the spec file,
the build logs get more readable as the contents of the extracted
source archive are not included.

"no-changelogname-tag" means that your spec file is missing a
standard %changelog section where you sum up important changes
applied to the spec file.

> %define builddir $RPM_BUILD_DIR/%{name}-%{version}

This is unused in the spec file, so don't define it.

> %define name	ppl
> %define version 0.9
> %define release 1

Don't. All macros are defined in the lines below. Here:

> Name:		%{name}
> Version:	%{version}
> Release:	%{release}

Fill in the values in those lines, instead of redefining macros
via macros. Do this:

Name: ppl
Version: 0.9
Release: 1

It defines %name, %version, and %release. You can move these lines to
the top of the spec file and improve readability.

> Vendor:		ppl-devel at cs.unipr.it
> Packager:	%{packager}

Don't. Build systems need to be able to override these, so only
define them in your local configuration. You don't want anybody,
who builds broken binary rpms from your src.rpm, to mark them
as coming from you. The included spec %changelog is an entirely
different thing.

> License:	GPL v2

It's just "GPL".

> Requires:	gmp >= 4.1.3, gcc-c++ >= 4.0.2

Both are wrong. Instead, you want "BuildRequires: gmp-devel",
provided that you want to compile with GNU MP. If so, the dependency
on the GNU MP library soname is automatically inserted by rpmbuild. 

> Prefix:		/usr

This means that you want to mark the packages as relocatable.
Whether it really is relocatable remains to be seen after bringing
it into shape first. You can safely delete this line unless you
insist on making it relocatable.

> %setup -n %{name}-%{version}

Just  %setup -q  is fine, since -n %{name}-%{version} is the
default.

> %build
> CXXFLAGS="$RPM_OPT_FLAGS" ./configure --enable-shared \
[snip]

Instead of this long command-line, simply run the %configure macro
and add any options to it, which it doesn't set automatically.

  %configure --enable-shared

Look at  rpm --eval %configure  to see what it does. For example,
it sets CXXFLAGS for you, too.

> %install
> if [ -d $RPM_BUILD_ROOT ]
> then
> 	rm -rf $RPM_BUILD_ROOT
> fi
> mkdir -p $RPM_BUILD_ROOT

Not needed and superfluous. Use just:

  %install
  rm -rf $RPM_BUILD_ROOT

> make prefix=$RPM_BUILD_ROOT%{_prefix} bindir=$RPM_BUILD_ROOT%{_bindir} \
[snip]

With standard GNU autotools packages, prefer this line

  make DESTDIR=$RPM_BUILD_ROOT install

over the rather long command-line in your spec file. When it
doesn't work (unlikely with well-maintained autotools code), there
is the %makeinstall macro, which can be used instead. Look at
rpm --eval %makeinstall   to see what it would do. But prefer the
DESTDIR install.

> %files

The main "ppl" package contains a mixture of files. Split off a
"ppl-devel" package, which contains the files needed only for
software development (include files, the libppl.so symlink,
documentation for developers, ppl-config, and so on).
The "ppl" package should only contain the main library files,
the licence, and any files relevant to the library run-time.

Don't include static libraries. If possible, disable them at configure
time with --disable-static  or just delete them in %install. Also
don't include libtool archive files *.la.

> %files c

Same here. A "ppl-c-devel" package is missing. But doesn't it make
sense to put C and C++ library and API into the "ppl" and
"ppl-devel" packages?

> %files gprolog
> %defattr(-,root,root)
> %{_bindir}/ppl_gprolog
> %{_libdir}/ppl/ppl_gprolog.pl

The directory %{_libdir}/ppl/ is not included. You can add
%dir %{_libdir}/ppl   in the right package which should own this
directory entry.

>%files sicstus

Same here and the other sub-packages.
They also contain an orphaned %_libdir/ppl directory.

> %post
> /sbin/ldconfig

> %postun
> /sbin/ldconfig

> %post c
> /sbin/ldconfig

> %postun c
> /sbin/ldconfig

Better:

%post -p /sbin/ldconfig
%postun -p /sbin/ldconfig
%post c -p /sbin/ldconfig
%postun c -p /sbin/ldconfig

That executes /sbin/ldconfig directly instead of via /bin/sh,
and it creates a dependency on /sbin/ldconfig automatically.

-- 
Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug, or are watching the QA contact.