[Bug 168310] Review Request: swish-e <bkyoung>

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

Summary: Review Request: swish-e <bkyoung>


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





------- Additional Comments From bugs.michael at gmx.net  2005-09-28 08:07 EST -------
A little feedback on: swish-e-2.4.2-4_FC4.src.rpm

> %define	name	swish-e
> %define	version	2.4.2
> %define release 4_FC4

This is commonly considered bad style. All three macros are defined
via the "Name: ...", "Version: ..." and "Release: " tags.

> %define release 4_FC4

Fedora Extras packagers have the option to append %{?dist} like in
"Release: 4%{?dist}" instead of making up their own dist tags. "_FC4"
must go, also because "FC4" is older than "fc3" in RPM version comparison.
%{?dist} expands to ".fc4", ".fc3" and so on, in the Fedora Extras build
system and is also understood by "make tag" in Fedora Extras CVS.

> %define httpd_conf_d_dir /etc/httpd/conf.d

/etc is %_sysconfdir, which you use elsewhere, too.

> # Conditionals
> # --with debug: Replaces -O2 with -O0 in CFLAGS
> #		Builds debuginfo package.
> #		Defines compile time macro DEBUG.
> # --without debug OR missing: Disables debuginfo package AND Removes -g.
> 
> %{!?_with_debug:%define debug_package %{nil}}

Why? Default build ought to use $RPM_OPT_FLAGS and create a good
"debuginfo" package automatically. If compiler flags are modified to
not include -g, patch the code. Disabling the debuginfo package is a
last resort only.

> Summary:        SWISH-E - Simple Web Indexing System for Humans - Enhanced

Repeating the package name in the summary is considered bad style.
The summary is the only important thing.

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

Fill in the values here, not via defining %name, %version and %release.
Avoid multiple places where to define these values.

> Requires:       libxml2 >= 2.6.19, pcre >= 5.0, zlib >= 1.2.2.2

All these three should be automatic already via SONAME requirements,
look at "rpm -qR swish-e" of your binary package. It didn't build for
me (attaching log later), so I couldn't check this. Explicit dependencies
on package names make it much more difficult to move/upgrade libraries.

> BuildArch:      i386 x86_64

Why that? Did you mean to do "ExcludeArch: ppc ppc64" possibly? If so,
the spec file should give a rationale (and later link to a bugzilla
ticket where the failure is tracked).

> %description
> Swish-e is Simple Web Indexing System for Humans - Enhanced

What is it called? Swish-e or SWISH-E?

> %description    perl
> PERL SWISH-E language bindings and scripts.

It's called "Perl", not PERL.

> %package        perl
> Summary:        SWISH-E - PERL Scripts and Modules
> Group:          Applications/Internet
> Requires:       %{name} = %{version}, perl >= 5.8.0

Couldn't build the package, but I believe that since you install into
versioned vendor locations, you want a different dependency on "perl":

Requires: perl(:MODULE_COMPAT_%(eval "`%{__perl} -V:version`"; echo $version))

> %package	devel

The -devel subpackage ought to require the full %version-%release of the
main package to always stay in sync with any patches, changelog entries, ...

> %build

Can you please explain what all that stuff in there is supposed to
achieve. Why don't you just use the %configure macro, which takes care
of setting CFLAGS, CXXFLAGS, FFLAGS, as well as setting --prefix and friends?

> for i in $(find . -name config.guess -o -name config.sub) ; do
> 	if [ -f /usr/lib/rpm/redhat/$(basename $i) ]; then
> 	%{__rm} -f $i
> 	%{__cp} -fv /usr/lib/rpm/redhat/$(basename $i) $i
> 	fi
> done

Without any comment at all? :-O

> [ "${RPM_BUILD_ROOT}" != "/" ] && [ -d ${RPM_BUILD_ROOT} ] && rm -rf
> ${RPM_BUILD_ROOT};

This check only decreases readability. Delete it. You define a good
buildroot at the top. This has been beaten to death in older reviews
and on extras-list.

> for i in $RPM_BUILD_ROOT%{_mandir}/man1/*.gz; do
> 	%{__rm} -f $i
> 	done

Why? Things like that ought to be commented.

> %post devel -p /sbin/ldconfig
> 
> %postun devel -p /sbin/ldconfig

Why?

> %files
>
> %{_libdir}/swish-e/swishspider

Directory %_libdir/swish-e is not included, can be created with bad
permissions and would not be removed upon erasing the package.

> %files

GNU GPL must be included in the main package at least. The lawyers want that.

> %files perl
> %defattr(-, root, root)
> %{_libdir}/swish-e/perl/SWISH/Filters/Doc2txt.pm

Lots of directories not included in that path. Also, are you aware of
using wildcards in the %files section? Is it really necessary to list
each and every file manually?

> %{_datadir}/swish-e/search.tt

%_datadir/swish-e not included in the package either.

> %{_libdir}/libswish-e.la
> %{_libdir}/libswish-e.a

Avoid packaging libtool archives and static libraries unless there is
good reason to package them. Libtool archives create nasty inter-library
dependencies which also bloat up build requirements.


-- 
Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.