Bug 205075 - Review Request: fwbackups - a user backup program, with support for automated backups and on-demand backups
Review Request: fwbackups - a user backup program, with support for automated...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-09-03 16:21 EDT by Stewart Adam
Modified: 2007-11-30 17:11 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-10-02 16:42:33 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Stewart Adam 2006-09-03 16:21:16 EDT
Spec URL: http://www.diffingo.com/downloads/FWBackups/fwbackups.spec
SRPM URL: http://www.diffingo.com/downloads/FWBackups/fwbackups-1.42-2.src.rpm
Description:
fwbackups is a user backups program that can run a backup on-the-spot based
on user-specified paths, or by it's automated backup feature which will backup
user-specified paths and backup them to an appropriate location automatically,
at a specified time and date.

** this is my first package submission and I need a sponsor. **
Comment 1 Parag AN(पराग) 2006-09-06 01:22:15 EDT
rpmlint on binary rpm is not silent
W: fwbackups summary-not-capitalized fwbackups is a user backup program
Summary doesn't begin with a capital letter.

W: fwbackups symlink-should-be-relative /usr/bin/fwbackups /usr/bin/consolehelper
Absolute symlinks are problematic eg. when working with chroot environments.

W: fwbackups symlink-should-be-relative /usr/bin/fwbackups-run
/usr/bin/consolehelper
Absolute symlinks are problematic eg. when working with chroot environments.

E: fwbackups use-old-pam-stack /etc/pam.d/fwbackups
Update pam file to use include instead of pam_stack.

E: fwbackups use-old-pam-stack /etc/pam.d/fwbackups-run
Update pam file to use include instead of pam_stack.

update changes as suggested above 
Comment 2 Stewart Adam 2006-09-06 17:47:47 EDT
I've made new a nwe SRPM / Spec:
Spec URL: http://www.diffingo.com/downloads/FWBackups/fwbackups.spec
SRPM URL: http://www.diffingo.com/downloads/FWBackups/fwbackups-1.42-3.src.rpm
Thanks,
Stewart
Comment 3 Parag AN(पराग) 2006-09-13 08:36:46 EDT
{Not Official Reviewer}
packaging looks ok.
+ Mockbuild is successfull for i386 FC6
+ rpmlint on binary rpm is silent
+ dist tag is present
+ Buildroot is correct
- source URL is NOT correct
 wget  http://www.diffingo.com/content/view/12/45/lang,en/fwbackups-1.42.tar.gz
--18:03:48-- 
http://www.diffingo.com/content/view/12/45/lang,en/fwbackups-1.42.tar.gz
Resolving www.diffingo.com... 69.90.92.10
Connecting to www.diffingo.com|69.90.92.10|:80... connected.
HTTP request sent, awaiting response... 404 Not Found
18:03:49 ERROR 404: Not Found.

+ BR is correct
+ License used is GPL
+ License file COPYING is included
+ MD5 sum on tarball is matching upstream tarball
9dc696ddf62f26827715fecbb15d6134  fwbackups-1.42.tar.gz
+ No duplicate files

Not tested package.

However when i check http://fedoraproject.org/wiki/Packaging/Python i found
first line in SPEC contains
%{!?python_sitelib: %define python_sitelib %(%{__python} -c "from
distutils.sysconfig import get_python_lib; print get_python_lib()")}

whereas above wikipage suggests 
%{!?python_sitelib: %define python_sitelib %(%{__python} -c "from
distutils.sysconfig import get_python_lib; print get_python_lib(1)")}

Comment 4 Stewart Adam 2006-09-13 17:47:11 EDT
True, but because it's building for noarch and it's just GTK / Python code -
wouldn't getting architecture-specific paths be useless? Alacarte, for example,
also builds for noarch and uses the line without the (1).

Anyways, I've published an updated .spec with a new SRPM:
Spec URL: http://www.diffingo.com/downloads/FWBackups/fwbackups.spec
SRPM URL: http://www.diffingo.com/downloads/FWBackups/fwbackups-1.42-4.src.rpm
Comment 5 Stewart Adam 2006-09-21 20:42:51 EDT
I've added the pygtk2-libglade %requires.
Spec URL: http://www.diffingo.com/downloads/FWBackups/fwbackups.spec
SRPM URL: http://www.diffingo.com/downloads/FWBackups/fwbackups-1.42-5.src.rpm

rpmlint on SRPM only warns of mixed space/tabs, nothing to worry about, and
rpmlint is silent on binary.
Comment 6 Jason Tibbitts 2006-09-25 23:42:46 EDT
Hmm, the tarball does not match upstream:

c987096dc11e2605f4a73c19484a4a9b  fwbackups-1.42.tar.gz
10c8fda1c5809681aa870a2c71f84ed5  ../fwbackups-1.42.tar.gz

The files are different sizes as well (31347 in the srpm, 32282 upstream).

I'm not sure why you have constructs like this in %install:
   install -p -d -m755 etc/fwbackups\
     ${RPM_BUILD_ROOT}%{_sysconfdir}/fwbackups

install -d creates each of its arguments as directories, but "etc/fwbackups"
already exists because it's in your source tarball.

You install fwbackups.conf twice.

I think you could significantly shrink your %install section with by using "cp
-rp" instead of installing each file separately, but I suppose that's up to you.

Note that there's currently some discussion about changing the recommendations
for installing desktop files, but that won't be decided for another several days
and I don't think it will effect your package.

Are you sure fwbackups.conf.default should go in /etc?  It should certainly not
be marked noreplace as you want it to change on an upgrade (since the old
version isn't the default any longer).  I would consider marking it %doc instead.
Comment 7 Stewart Adam 2006-09-26 18:54:24 EDT
I prefer to use %install because that way I can be sure that permissions, owner,
etc are setup properly and also so that if I ever want to move or exlude
something, it's easier to do that with copying everything in one command. Also,
this way macros are in effect where as in cp it's static copying. Right now it's
only python, but as fwbackups 1.43 or 1.44 comes out, it might introduce the
need for arch-specific directories and therefore builds.

As for the %install, I cleaned it up and also noticed I had a few bad GPL
copyright notices in my files, and so I used that opportunity to clean that up,
move the defaults file to /usr/share/fwbackups, and released fwbackups 1.42.1 -
Now the issues should be fixed and the tarballs matching. Here's the new locations:
Binary RPM: http://diffingo.com/downloads/fwbackups/fwbackups-1.42.1-1.noarch.rpm
SRPM: http://diffingo.com/downloads/fwbackups/fwbackups-1.42.1-1.src.rpm
Spec: http://diffingo.com/downloads/fwbackups/fwbackups.spec
rpmlint is silent on both binary and SRPM.
Comment 8 Jason Tibbitts 2006-09-30 00:20:22 EDT
Sorry for taking so long to respond; work intruded a bit into my evenings.

Firstly, the Source0: URL is not valud; it looks like you need to downcase
"FWBackups".  However, after doing this I find that again the files are not the
same.  What's in the srpm needs to match precisely what is downloaded from the
web site.

Also, for your sanity I recommend something like:

Source0: http://www.diffingo.com/downloads/fwbackups/fwbackups-%{version}.tar.gz

so that you only have the update the version in one place when you update the
package.

Aside from that, you should clean up the commented lines in %install (and the
commented Provides as well).  And I wonder what you have your tabs set for,
since the indentation is all over the place on my screen.

The package installs fine and seems to work for me on FC5.  (I did not test
actually running a backup.)

So really it's down to the tarball actually matching what's at the upstream web
site, and a few minor specfile adjustments.

Review:
X source files match upstream.
* package meets naming and packaging guidelines.
X specfile is properly named, is cleanly written and uses macros consistently
(needs minor cleanups).
* dist tag is present.
* build root is correct.
* license field matches the actual license.
* license is open source-compatible.  License text included in package.
* latest version is being packaged.
* BuildRequires are proper.
* %clean is present.
* package builds in mock (development & FC5, x86_64 & i386).
* package installs properly
* package runs OK.  (I only did some basic tests and didn't actually run a backup.)
* rpmlint is silent.
* final provides and requires are sane:
   config(fwbackups) = 1.42.1-1.fc6
   fwbackups = 1.42.1-1.fc6
  =
   /bin/bash
   /usr/bin/python
   config(fwbackups) = 1.42.1-1.fc6
   pygtk2
   pygtk2-libglade
   python(abi) = 2.4
   redhat-artwork
* %check is not present; no test suite upstream.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* no scriptlets present.
* code, not content.
* documentation is small, so no -docs subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* GUI app; .desktop files are supplied and installed properly.  No mimetype
keys, so no need to run update-mime-database. 
Comment 9 Stewart Adam 2006-09-30 12:56:55 EDT
Binary RPM: http://diffingo.com/downloads/fwbackups/fwbackups-1.42.1-2.noarch.rpm
SRPM: http://diffingo.com/downloads/fwbackups/fwbackups-1.42.1-2.src.rpm
Spec: http://diffingo.com/downloads/fwbackups/fwbackups.spec
I've fixed what you mentioned above, and rpmlint's also silent on all of the above.
Thanks!
Comment 10 Jason Tibbitts 2006-09-30 15:21:04 EDT
The source now matches upstream:
  bd0a4d63e0729fa3ed89eccb10fee2da  fwbackups-1.42.1.tar.gz
which is good.

It would still be nice if you could elide the commented out "Provides:" bit and
the commented "install" calls in the %install section and perhaps fix the
indentation, because it's better for specfiles to be neat and free of extraneous
cruft.  However, I'm not going to block on that; just clean things up when you
check in if you could.

APPROVED

Now you can go ahead and apply for cvsextras and fedorabugs access, and I'll
click on the proper buttons.  Let me know if you have any questions, or catch me
on IRC.

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