[Bug 768894] Review Request: haven - Next Generation Backup System

[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=768894

--- Comment #8 from Michael Schwendt <mschwendt at gmail.com> 2012-03-21 17:02:39 EDT ---
Non-trivial to review because a few more iterations will be needed, it seems.


> The upstream version is 0.0.2, whereas the specfile's version is 0_0_2,
> indirectly set using a macro.

Not true. Let's take a look:

| %global src_verstr 0.0.2
| %global rpm_verstr %(echo %src_verstr | sed -e 's/-/_/g')
| 
| Version: %rpm_verstr

It would substitute '-' with '_', but currently there is no '-' in "0.0.2".

This has been taken over from the spec included in the tarball.
It's dubious for a few reasons.

If the upstream version string ever contained a '-', the entire version would
need to be mapped into Fedora's versioning schemes. (For example: 0.0.2-beta1
=> apply Fedora's pre-release versioning scheme, which is *not* 0.0.2_beta1.)

In general, it can be beneficial to do

  %global tar_ver 0.0.2
  Version: 0.0.2
  ...

if %tar_ver frequently contains stuff like "-alphaX", "-rcX", which need to be
moved into the %release value. Then one can use the different %tar_ver and
%version in other places of the spec file.

What shouldn't be done is something close to

  %global version 0.0.2
  Version: %version

because the "Version" tag already defines %version, and it makes no sense to
redefine it.

An example from the spec that can lead to problems:

> Requires: %{name} = %{src_verstr}-%{release}

Here you should use %version instead of %src_verstr, or else you could not
split wisely between RPM-compatible and Fedora-compatible version values and
upstream version values.

Btw, the following applies, too:
https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package

But why do the subpackages depend on the "haven" package? Is it for files in
that package? A comment in the spec should explain the dependency.


> Requires(post): /usr/bin/gtk-update-icon-cache
> Requires(postun): /usr/bin/gtk-update-icon-cache

That's not Fedora's way to add such dependencies:
https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Icon_Cache


> The "%global _configure ../../configure" macro is not used and can
> be removed.

It _is_ used to alter the %configure macro.



> You have a configure-without-libdir-spec warning. Add --libdir=%{_libdir}

rpmlint false positive, it seems.


> %description
> Haven backup system. 
> 
> Next Generation Backup System, from desktop personal backup to
> Enterprise Disaster Recovery.

This description is somewhat strange. No full sentences and weird usage of
uppercase characters. Why not

"Haven aims at becoming a next generation backup system, from desktop personal
backup to enterprise-class disaster recovery."


> %install
...
> mv "icons" "%{buildroot}/%{_datadir}/"

Caution! Please don't break --short-circuit installs. Copy or install files
from within the build dir, don't move them.

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