Bug 870853 - Review Request: haven - Next Generation Backup System [NEEDINFO]
Review Request: haven - Next Generation Backup System
Status: ASSIGNED
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Björn "besser82" Esser
Fedora Extras Quality Assurance
:
: 768894 (view as bug list)
Depends On:
Blocks: FE-NEEDSPONSOR
  Show dependency treegraph
 
Reported: 2012-10-28 20:37 EDT by Davide Benini
Modified: 2014-10-11 16:40 EDT (History)
11 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: 768894
Environment:
Last Closed:
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
fedora: fedora‑review?
i: needinfo? (fedora)
i: needinfo? (dbenini)


Attachments (Terms of Use)

  None (edit)
Description Davide Benini 2012-10-28 20:37:06 EDT
+++ This bug was initially created as a clone of Bug #768894 +++

Spec URL: http://www.dbenini.com/haven/fedora/haven.spec
SRPM URL: http://www.dbenini.com/haven/fedora/haven-0.0.2-2.fc16.src.rpm

Description:
SafeHaven is a next generation backup system, from desktop personal backup to enterprise disaster recovery.

Features:
* Personal backup
* Enterprise backup
* Disaster recovery
* Heavy deduplication & compression

Notes:
- This is my first Fedora package and I'm looking for sponsorship.
- The package builds in mock.

--- Additional comment from dbenini@redhat.com on 2011-12-19 05:58:59 EST ---

rpmlint output:

haven.spec:57: W: configure-without-libdir-spec
haven.spec: W: invalid-url Source1: haven-icons-0.0.2.tar.gz
haven.spec: W: invalid-url Source0: haven-0.0.2.tar.gz

--- Additional comment from tchollingsworth@gmail.com on 2011-12-20 09:38:26 EST ---

(In reply to comment #1)
> haven.spec: W: invalid-url Source1: haven-icons-0.0.2.tar.gz
> haven.spec: W: invalid-url Source0: haven-0.0.2.tar.gz

If you can't provide the full URL to these sources in SourceN for some reason, you need to add a comment indicating how to obtain them.  For more information, see:
http://fedoraproject.org/wiki/Packaging:SourceURL

You used "haven-gtkgui" as the subpackage for the GUI application, but most Fedora packages use subpackages like "haven-gtk".  Consider changing it to be consistent with existing packages.

You call the "-full" subpackage the "Basic version" in the summary.  I think you got that backwards.  ;-)

Also, consider briefly articulating the difference between the full and basic versions in the %description, to assist users in deciding which one to install.  (See "yum info vim-minimal vim-enhanced" for an example.)

--- Additional comment from tchollingsworth@gmail.com on 2011-12-20 09:41:22 EST ---

One More Thing...unless the GUI is guaranteed to work with any version of the software always and forever, you probably want to version that dependency, e.g.:

Requires:  haven = %{version}-%{release}

--- Additional comment from dbenini@redhat.com on 2011-12-24 14:48:47 EST ---

Hi T.C.,

thank you very much for the review.

I've made the suggested changes to the spec file and re-packaged the application:

Spec URL: http://www.dbenini.com/haven/fedora/haven.spec
SRPM URL: http://www.dbenini.com/haven/fedora/haven-0.0.2-3.fc16.src.rpm


* Koji scratch build info:

https://koji.fedoraproject.org/koji/taskinfo?taskID=3603971


* rpmlint output:

$ rpmlint -v haven.spec
haven.spec:60: W: configure-without-libdir-spec
haven.spec: W: invalid-url Source1: haven-icons-0.0.2.tar.gz
haven.spec: I: checking-url http://downloads.sourceforge.net/safe-haven/haven-0.0.2.tar.gz (timeout 10 seconds)
0 packages and 1 specfiles checked; 0 errors, 2 warnings.

$ rpmlint -v haven
haven.x86_64: I: checking
haven.x86_64: I: checking-url http://sourceforge.net/projects/safe-haven/ (timeout 10 seconds)
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

$ rpmlint -v haven-enhanced
haven-enhanced.x86_64: I: checking
haven-enhanced.x86_64: I: checking-url http://sourceforge.net/projects/safe-haven/ (timeout 10 seconds)
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

$ rpmlint -v haven-gtk
haven-gtk.x86_64: I: checking
haven-gtk.x86_64: I: checking-url http://sourceforge.net/projects/safe-haven/ (timeout 10 seconds)
haven-gtk.x86_64: W: no-documentation
haven-gtk.x86_64: W: no-manual-page-for-binary ghaven
1 packages and 0 specfiles checked; 0 errors, 2 warnings.

--- Additional comment from jamielinux@fedoraproject.org on 2012-02-08 06:39:24 EST ---

Some comments:

1) BuildRoot tag is no longer required
2) Removing %{buildroot} in the %install section is no longer required
3) %clean section no longer required
4) defattr in %files section is no longer required

--- Additional comment from dbenini@redhat.com on 2012-02-12 18:51:58 EST ---

Thank you Jamie,

I removed useless sections and tags as suggested.

* Updated spec file and source RPM:

Spec URL: http://www.dbenini.com/haven/fedora/haven.spec
SRPM URL: http://www.dbenini.com/haven/fedora/haven-0.0.2-4.fc16.src.rpm

* Koji scratch build info:

http://koji.fedoraproject.org/koji/taskinfo?taskID=3784798

--- Additional comment from leamas.alec@gmail.com on 2012-02-23 06:24:17 EST ---

No, I'm no reviewer. Still, I have some remarks and questions:

The upstream version is 0.0.2, whereas the specfile's version is 0_0_2,
indirectly set using a macro. Basically, the Version: tag should reflect
the upstream version i. e. "Version: 0.0.2". Note that from that point
an implicit %{version} macro is defined, which you can use to derive
0_0_2 which is used in many places.

The "%global _configure ../../configure" macro is not used and can be removed.
The %{__mkdir_p} type of macros are not used in Fedora, plain 'mkdir -p' is fine
See http://fedoraproject.org/wiki/Packaging:Guidelines#macros

The source seems to be GPLv3 or later i. e., "License: GPLv3+" ?

Backup software is sensitive... Maybe you should  enable PIE? See
http://fedoraproject.org/wiki/Packaging:Guidelines#PIE

Why have you called the desktop file ghaven.desktop? The normal convention
is to use the package name i. e., haven.desktop or haven-gtk.desktop? The
same question applies to the icons.

In the %files section, you have entries like %{_mandir}/man1/haven.1.gz.
These are better written %{_mandir}/man1/*, partly for simplicity, partly
because the .gz suffix might change in the future.

Since the %check target is empty, you might as well remove it.

You have a configure-without-libdir-spec warning. Add --libdir=%{_libdir} to
the configure calls so that configure knows where to install (lib/lib64).

You have an empty-debuginfo-package rpmlint warning. This might be because of
the install-strip targets used, if they indeed strip the code. Basically,
you should not strip the code but instead build it with debugging enabled.
rpmbuild strips it while postprocessing, using the debuginfo in the debug
package.


Hope this helps,

--a

--- Additional comment from mschwendt@gmail.com on 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.

--- Additional comment from dbenini@redhat.com on 2012-03-23 09:19:43 EDT ---

Alec, Michael,

thank you for your comments.

I will analyse your suggestions (and discuss some of them with upstream) and then I'll come back with a new version of the package.

--- Additional comment from tibbs@math.uh.edu on 2012-07-03 22:35:15 EDT ---

So, over three months later, was there ever any progress here?

--- Additional comment from dbenini@redhat.com on 2012-07-08 17:39:07 EDT ---

Sorry for the long delay,

I've made the following changes to the spec file and re-packaged the application:
- removed macro forms of system executables, i.e. %{__mkdir_p} macros
- changed license from GPLv3 to GPLv3+
- enabled PIE
- replaced .gz man files suffix with .* in %files section
- removed %check target since it was empty
- replaced "install-strip" make rule with plain "install"
- removed %src_verstr and %rpm_verstr definition to avoid confusion. %src_verstr will be reintroduced if necessary, but at the moment only %version is used
- removed subpackages dependency from the "haven" (base) package including licence texts in each subpackage
- removed /usr/bin/gtk-update-icon-cache requirement
- changed description
- files are now copied, not moved, from within the build dir

* Updated spec file and source RPM:

Spec URL: http://www.dbenini.com/haven/fedora/haven.spec
SRPM URL: http://www.dbenini.com/haven/fedora/haven-0.0.2-5.fc16.src.rpm

* Koji scratch build info:

http://koji.fedoraproject.org/koji/taskinfo?taskID=4225990

--- Additional comment from mschwendt@gmail.com on 2012-09-08 18:44:28 EDT ---

When I wanted to take another look at it, it failed to build for Fedora 17 due to the header changes in glib2 ("Only <glib.h> can be included directly.").

--- Additional comment from tibbs@math.uh.edu on 2012-10-01 11:38:13 EDT ---

Indeed, fails to build for me in rawhide as well.

--- Additional comment from dbenini@gmail.com on 2012-10-10 18:53:18 EDT ---

Hi all,

Unfortunately my previous Bugzilla account has been disabled and I'm still trying to figure out how to move this package review to my new account.

Anyway, I fixed the Fedora 17+ build issue:

Spec URL: http://www.dbenini.com/haven/fedora/haven.spec
SRPM URL: http://www.dbenini.com/haven/fedora/haven-0.0.2-6.fc17.src.rpm

* Koji scratch build info:

http://koji.fedoraproject.org/koji/taskinfo?taskID=4575069
http://koji.fedoraproject.org/koji/taskinfo?taskID=4575082
http://koji.fedoraproject.org/koji/taskinfo?taskID=4575129

--- Additional comment from mschwendt@gmail.com on 2012-10-11 11:36:08 EDT ---

Since mere mortals cannot change the "Reporter" field in bugzilla, one way to transfer the ticket to you would be to clone it, 
  https://bugzilla.redhat.com/enter_bug.cgi?cloned_bug_id=768894
and then continue there.
Comment 1 T.C. Hollingsworth 2012-10-29 18:41:29 EDT
*** Bug 768894 has been marked as a duplicate of this bug. ***
Comment 2 Volker Fröhlich 2013-05-07 13:53:11 EDT
Have you found a sponsor yet Davide?
Comment 3 Davide Benini 2013-05-09 17:39:24 EDT
I've not found a sponsor yet Volker.

If you could sponsor me it would be great.
Comment 4 Davide Benini 2013-07-28 18:01:24 EDT
Koji scratch build info for the latest Fedora releases:

* Fedora 18
  http://koji.fedoraproject.org/koji/taskinfo?taskID=5642342

* Fedora 19
  http://koji.fedoraproject.org/koji/taskinfo?taskID=5656767
Comment 5 Christopher Meng 2013-07-28 20:40:41 EDT
I'll take an informal review.

1. Why not nameing it safehaven but haven?

2. %define _hardened_build 1

should be

%global _hardened_build 1

3. Can you ask upstream to avoid using 

%global _configure ../../configure
mkdir -p build/standard build/gtkgui build/full
cd build/standard 
%configure --disable-ext2fs --disable-gmime --without-postgresql?

I think this is not cmake, why should we burden ourselves?

4. I think you should add requires of main package for all subpackages.

5. Once you've done the 4, you should remove subpackages'

%doc README COPYING

As these doc should only present one time in main package.

6. You shouldn't create a new tarball when the download URL is available.
Comment 6 Veaceslav Mindru 2013-07-29 19:01:33 EDT
rpmlint -i SPECS/haven.spec

>SPECS/haven.spec:57: W: configure-without-libdir-spec
A configure script is run without specifying the libdir. configure options
must be augmented with something like --libdir=%{_libdir} whenever the script
supports it.

>SPECS/haven.spec: W: invalid-url Source1: haven-icons-0.0.2.tar.gz
The value should be a valid, public HTTP, HTTPS, or FTP URL.

>SPECS/haven.spec: W: invalid-url Source0: haven-0.0.2.tar.gz
The value should be a valid, public HTTP, HTTPS, or FTP URL.
Comment 7 Davide Benini 2013-08-04 18:56:27 EDT
(In reply to Christopher Meng from comment #5)

Hi Christopher,

> 1. Why not nameing it safehaven but haven?

To be aligned with upstream name.

> 2. %define _hardened_build 1

I've made the suggested change to the spec file:

Spec URL: http://www.dbenini.com/haven/fedora/haven.spec
SRPM URL: http://www.dbenini.com/haven/fedora/haven-0.0.2-7.fc19.src.rpm

* Koji scratch build info:

https://koji.fedoraproject.org/koji/taskinfo?taskID=5776199

> 3. Can you ask upstream to avoid using 
> 
> %global _configure ../../configure
> mkdir -p build/standard build/gtkgui build/full
> cd build/standard 
> %configure --disable-ext2fs --disable-gmime --without-postgresql?
> 
> I think this is not cmake, why should we burden ourselves?

I will get in touch with upstream about this.

> 4. I think you should add requires of main package for all subpackages.
> 
> 5. Once you've done the 4, you should remove subpackages'

There is no main package, see previous comments.

> 6. You shouldn't create a new tarball when the download URL is available.

Could you please elaborate this one?

Thanks
Comment 8 Davide Benini 2013-08-04 19:05:09 EDT
(In reply to Veaceslav Mindru from comment #6)

Hi Veaceslav,

> rpmlint -i SPECS/haven.spec

These seem to be rpmlint false positives.

Please check previous comments.
Comment 9 Christopher Meng 2013-08-04 21:01:30 EDT
Hi,

I can see "%doc README COPYING" 3 times.

Please only ship one time. I suggest shipping them in main package.

And, what does it mean?> --disable-ext2fs --disable-gmime

Thanks.
Comment 10 Davide Benini 2013-08-07 09:03:02 EDT
Christopher, since each package can be installed separately, as per Fedora Package Licensing Guidelines (https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Subpackage_Licensing):

"if a subpackage is independent of any base package (it does not require it, either implicitly or explicitly), it must include copies of any license texts (as present in the source) which are applicable to the files contained within the subpackage."


Regarding the flags, they come from upstream.
Comment 11 Björn "besser82" Esser 2013-10-19 05:38:38 EDT
taken  ;)
Comment 12 Christopher Meng 2013-12-13 01:16:35 EST
ping.
Comment 13 Christopher Meng 2014-04-24 04:48:45 EDT
Question again, have you found a sponsor yet?

Question again, status of this bug?
Comment 14 Volker Fröhlich 2014-10-11 16:40:33 EDT
I think we can safely close this request.

Note You need to log in before you can comment on or make changes to this bug.