Bug 468462 - Review Request: sbackup - Simple Backup Suite for desktop use
Review Request: sbackup - Simple Backup Suite for desktop use
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Christoph Wickert
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-10-24 16:23 EDT by Simon
Modified: 2009-01-21 16:38 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-01-21 16:33:08 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
cwickert: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Simon 2008-10-24 16:23:08 EDT
Spec URL: 
http://cassmodiah.fedorapeople.org/sbackup-0.10.5/sbackup.spec

SRPM URL: 
http://cassmodiah.fedorapeople.org/sbackup-0.10.5/sbackup-0.10.5-1.fc10.src.rpm

Description: 
Simple Backup Suite is a set of backend backup daemon and GNOME
GUI frontends that provide a simple yet powerful backup
solution for common desktop users.

Backups can be written to local directory or remote servers using
GNOME VFS technology. A fine control is possible regarding what
folders and files to backup. Files can be excluded even with a set
of regular expressions. Regular backups can be scheduled.
Comment 1 Simon 2008-11-01 09:39:14 EDT
Spec URL: 
http://cassmodiah.fedorapeople.org/sbackup-0.10.5/sbackup.spec

SRPM URL: 
http://cassmodiah.fedorapeople.org/sbackup-0.10.5/sbackup-0.10.5-2.fc10.src.rpm

I think this would be a good package for EPEL.
Comment 2 Simon 2008-11-26 18:15:34 EST
i have a little problem with consolehelper and need a little help.

Spec URL: 
http://cassmodiah.fedorapeople.org/sbackup-0.10.5/sbackup.spec

SRPM URL: 
http://cassmodiah.fedorapeople.org/sbackup-0.10.5/sbackup-0.10.5-3.fc10.src.rpm

i can't integrate consolehelper on this way, i can't get authentication as root.
Comment 3 Simon 2008-11-27 08:13:50 EST
> Spec URL: 
> http://cassmodiah.fedorapeople.org/sbackup-0.10.5/sbackup.spec
> 
> SRPM URL: 
> http://cassmodiah.fedorapeople.org/sbackup-0.10.5/sbackup-0.10.5-3.fc10.src.rpm

ah, i found the problem and updated the spec above.
no new changelog, just a non-mentionable typo.

I hope this would do the job.
Comment 4 Simon 2008-11-27 11:47:45 EST
Koji: 
http://koji.fedoraproject.org/koji/taskinfo?taskID=954509

rpmlint output:
[cassmodiah@schafwiese SPECS]$ rpmlintsetuptree 
sbackup.src: W: mixed-use-of-spaces-and-tabs (spaces: line 4, tab: line 75)
1 packages and 0 specfiles checked; 0 errors, 1 warnings.
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

the columns in the pam-files are seperated with tabs.
the spec itself is tabfree...
Comment 5 Christoph Wickert 2009-01-03 20:15:28 EST
REVIEW FOR 9050675dce622f3983571eb094ca60ec  sbackup-0.10.5-3.fc10.src.rpm


OK - MUST: ]$ rpmlint /var/lib/mock/fedora-rawhide-i386/result/sbackup-0.10.5-3.fc11.*
sbackup.src: W: mixed-use-of-spaces-and-tabs (spaces: line 4, tab: line 76)
2 packages and 0 specfiles checked; 0 errors, 1 warnings.
can be ignored, see comment # 4.
OK - MUST: The package is named according to the Package Naming Guidelines.
OK - MUST: The spec file name matches the base package %{name}, in the format %{name}.spec.
FIX - MUST: The package does not meet the Packaging Guidelines.
- Timestamp of Source0 does not match
OK - MUST: The package is licensed with a Fedora approved license (GPLv2+) and meets the Licensing Guidelines.
OK - MUST: The License field in the package spec file matches the actual license.
OK - MUST: The license file from the source package is included in %doc.
OK - MUST: The spec file is in American English.
OK - MUST: The spec file for the package is legible.
OK - MUST: The sources used to build the package matches the upstream source by MD5 0d754b72da3b5cadf6de203cdf7afe13
OK - MUST: The package successfully compiles and builds into binary rpms on i386
N/A - MUST: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch.
OK - MUST: All build dependencies are listed in BuildRequires.
OK - MUST: The spec file handles locales properly with the %find_lang macro.
N/A - MUST: Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun.
OK - MUST: The package is not designed to be relocatable.
OK - MUST: The package owns all directories that it creates.
OK - MUST: The package does not contain any duplicate files in the %files listing.
OK - MUST: Permissions on files are set properly. Every %files section includes a %defattr(...) line.
OK - MUST: The package has a %clean section, which contains rm -rf %{buildroot}.
OK - MUST: The package consistently uses macros, as described in the macros section of Packaging Guidelines .
OK - MUST: The package contains code.
N/A - MUST: Large documentation files should go in a -doc subpackage.
OK - MUST: Files included as %doc do not affect the runtime of the application.
N/A - MUST: Header files must be in a -devel package.
N/A - MUST: Static libraries must be in a -static package.
N/A - MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' (for directory ownership and usability).
N/A - MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package.
N/A - MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release}
OK - MUST: The package does not contain any .la libtool archives.
FIX - MUST: The Package contains a GUI application and includes a %{name}.desktop file that is properly installed with desktop-file-install in the %install section, but there are some issues with the desktop files, see below.
OK - MUST: The packages does not own files or directories already owned by other packages.
OK - MUST: At the beginning of %install, the package runs rm -rf %{buildroot}.
OK - MUST: All filenames in rpm packages are valid UTF-8.

SHOULD Items:
N/A - SHOULD: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it.
N/A - SHOULD: The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available.
OK - SHOULD: The package builds in mock.
OK - SHOULD: The package should compile and build into binary rpms on all supported architectures.
OK - SHOULD: The package functions as described.
OK - SHOULD: The scriptlets used are must be sane.
N/A - SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency.
N/A - SHOULD: The placement of pkgconfig(.pc) files depends on their usecase, and this is usually for development purposes, so should be placed in a -devel pkg.
N/A - SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself.


Issues:
- Timestamp of Source0 does not match, see
https://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps

- Desktop files:
  - paths are hardcoded
  - key "Categories" is a list and does not have a semicolon as trailing character
  - Categories are IMO not correct. Suggestion:
     sbackup-restore = System;Utility;Filesystem;Archiving;GNOME;GTK;
     sbackup-conf =  System;Settings;SystemSettings;GNOME;GTK;
     In case of doubt see
     http://standards.freedesktop.org/menu-spec/latest/apa.html
  - add GenericNames for KDE compatibility. Use the window title
    Restore files/directories, Backup Properties
  - please add (at least German) Translations for the keys, e. g.
    GenericName[de]=Dateien/Verzeichnisse wiederherstellen


- Requires:       usermode-gtk for the password dialog

- Use global pam config?
    #%PAM-1.0
    auth            include         config-util
    account         include         config-util
    session         include         config-util

- I suggest you include pam config as separate sources instead of creating them on the fly.

- include an initscript for sbackupd?
- What are the Exclude statements for?
Comment 6 Simon 2009-01-05 11:46:18 EST
> Issues:
> - Timestamp of Source0 does not match, see
> https://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps

I downloaded it again. Now it should be right

> - Desktop files:
>   - paths are hardcoded
>   - key "Categories" is a list and does not have a semicolon as trailing
> character
>   - Categories are IMO not correct. Suggestion:
>      sbackup-restore = System;Utility;Filesystem;Archiving;GNOME;GTK;
>      sbackup-conf =  System;Settings;SystemSettings;GNOME;GTK;
>      In case of doubt see
>      http://standards.freedesktop.org/menu-spec/latest/apa.html
>   - add GenericNames for KDE compatibility. Use the window title
>     Restore files/directories, Backup Properties
>   - please add (at least German) Translations for the keys, e. g.
>     GenericName[de]=Dateien/Verzeichnisse wiederherstellen

Should now meet your requirements


> - Requires:       usermode-gtk for the password dialog
rpmlint says "sbackup.noarch: W: no-dependency-on usermode" should be okay because usermode-gtk requires usermode

> - Use global pam config?
>     #%PAM-1.0
>     auth            include         config-util
>     account         include         config-util
>     session         include         config-util

Okay, i changed it.

> - I suggest you include pam config as separate sources instead of creating them
> on the fly.
I think creating them on the fly is very beautiful, because there are no hardcoded paths
 
> - include an initscript for sbackupd?
not relevant, because sbackupd will be controled by a crontab, created by simple backup config

> - What are the Exclude statements for?
https://fedoraproject.org/wiki/Packaging/Python#Unnecessary_Byte_compilation


SPEC: http://cassmodiah.fedorapeople.org/sbackup-0.10.5/sbackup.spec
SRPM: http://cassmodiah.fedorapeople.org/sbackup-0.10.5/sbackup-0.10.5-4.fc10.src.rpm
Comment 7 Christoph Wickert 2009-01-11 18:56:22 EST
- Timestamp of Source0 is fixed.
- Please remove SystemSettings from the desktop files again, it's better for comptatibility. Sorry for the noise, apart from that the desktop files are ok now.
- rpmlint warning regarding usermode can be ignored, but the changelog could be a little better, e. g.: "Require usermode-gtk instead of usermode for the password dialog." But this is really trivial.
- on the fly creation of files: Your decision, you are the one to maintain the package. ;)
- (How) Do we own the cron files?
- Can you explain the makefile.patch a little? Why are you preventing installation of the desktop files and the locales?
- Provides: gvfs >= 1.0 looks bogus to me
Comment 8 Simon 2009-01-16 18:41:12 EST
(In reply to comment #7)
> - Please remove SystemSettings from the desktop files again, it's better for
> comptatibility. Sorry for the noise, apart from that the desktop files are ok
> now.

Okay! Done in -5


> - rpmlint warning regarding usermode can be ignored, but the changelog could be
> a little better, e. g.: "Require usermode-gtk instead of usermode for the
> password dialog." But this is really trivial.

Okay, you are right.


> - on the fly creation of files: Your decision, you are the one to maintain the
> package. ;)

Okay thank you

> - (How) Do we own the cron files?
mkdir -p %{buildroot}%{_sysconfdir}/{cron.d,cron.daily,cron-hourly,cron.monthly,cron.weekly}/
touch %{buildroot}%{_sysconfdir}/{cron.d,cron.daily,cron-hourly,cron.monthly,cron.weekly}/%{name}
%ghost %{_sysconfdir}/cron*/%{name}

this should do the trick...


> - Can you explain the makefile.patch a little? Why are you preventing
> installation of the desktop files and the locales?


i changed this part, completely! I hope you'll like it.
The patch for the makefile is now only for the settings of the directories and the permission of the configfile. The makefile is damn ugly and almost unusuable. I made a mix of patching makefile and installation via spec. this was very ugly, too...


locales:
there are unofficial translations which are not listed in the makefile, but shipped with sbackup.
why are they unofficial? 
These translation were made by the ubuntu-community (this project is an Ubuntu project) so they are not made by the "upstream team" and are unofficial.
I added the others to use all available locales. I realized the installation of the locales with a sed command. 

desktop files:
Now the original desktop files will be deleted and the new ones will installed. Commenting out the desktopfiles in the makefile was a bad idea.



> - Provides: gvfs >= 1.0 looks bogus to me

yes, you are right. This is a bogus. I removed it in -5 

SPEC: http://cassmodiah.fedorapeople.org/sbackup-0.10.5/sbackup.spec
SRPM: http://cassmodiah.fedorapeople.org/sbackup-0.10.5/sbackup-0.10.5-5.fc10.src.rpm
Comment 9 Christoph Wickert 2009-01-17 14:26:47 EST
Let me see: sbackup-0.10.5-5
- owns the cron files
- requires gvfs
- has nice desktop files
- includes updated patches that now work as expected and as we figured out on irc.

I see no other blockers and issues and therefore the package is APPROVED.
Comment 10 Simon 2009-01-17 14:29:48 EST
Thank you Christoph

New Package CVS Request
=======================
Package Name: sbackup
Short Description: Simple Backup Suite for desktop use
Owners: cassmodiah
Branches: F-9 F-10 EL-5
InitialCC:
Comment 11 Kevin Fenzi 2009-01-18 17:28:20 EST
cvs done.
Comment 12 Fedora Update System 2009-01-19 10:02:08 EST
sbackup-0.10.5-5.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/sbackup-0.10.5-5.fc10
Comment 13 Fedora Update System 2009-01-19 10:03:20 EST
sbackup-0.10.5-5.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/sbackup-0.10.5-5.fc9
Comment 14 Fedora Update System 2009-01-21 16:33:05 EST
sbackup-0.10.5-5.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 15 Fedora Update System 2009-01-21 16:38:02 EST
sbackup-0.10.5-5.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

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