Bug 713122 - Review Request: flyback - Git based backup software
Summary: Review Request: flyback - Git based backup software
Keywords:
Status: CLOSED WONTFIX
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: FE-DEADREVIEW
TreeView+ depends on / blocked
 
Reported: 2011-06-14 11:14 UTC by Heiko Adams
Modified: 2011-11-19 23:23 UTC (History)
10 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-11-19 23:23:28 UTC


Attachments (Terms of Use)

Description Heiko Adams 2011-06-14 11:14:53 UTC
Spec URL: https://dl.dropbox.com/s/zjkybrv5vywsgod/flyback.spec?dl=1
SRPM URL: https://dl.dropbox.com/s/5reu45rmwkl2m3o/flyback-0.6.5-1.fc15.src.rpm?dl=1
Description: flyback is a python application that aims to be a clone of Apple's Timemachine just for Linux systems.

This is my first package so regarding to fedora's rules I need a sponsor

Comment 1 Rahul Sundaram 2011-06-14 12:09:27 UTC
Blocking FE-NEEDSPONSOR.

Comment 2 Kevin Kofler 2011-06-14 15:08:36 UTC
Please don't put trademarks into the Summary like that (even if you misspell them ;-) ). Try to describe what the package objectively DOES instead.

Comment 3 Heiko Adams 2011-06-14 16:43:32 UTC
okay, will be changed with next release. Thanks for advice.

Comment 6 Fabian Affolter 2011-06-19 10:41:52 UTC
Just some quick comments:

- You are mixing macro style and variable style (${RPM_BUILD_ROOT} vs.  %{buildroot})
- Comment the patches
  https://fedoraproject.org/wiki/Packaging:Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment
-

Comment 7 Martin Gieseking 2011-07-17 18:47:30 UTC
Here are some further comments:

- If you use sources from an scm, always checkout a specific revision or 
  changeset, e.g.
  hg clone -u 80e92ef119a0 https://flyback.googlecode.com/hg/ flyback

  Otherwise, it's hard to verify the integrity of the sources. It would be 
  even better if upstream released source tarballs and not just deb files. 
  Maybe you can ask him to do this.

- As Fabian already wrote, add comments above each PatchX line telling what 
  the patch does.
  Also, to reduce the number of patches, merge all patch files that have 
  similar goals, e.g. fixing paths.

- There's no copyright information in the source files, so the license is
  GPL+. You should ask upstream to add a copyright header to the source files
  as mentioned in the GPL license text. The version information in the
  enclosed GPL.txt does not automatically apply to the sources.

- Please use plain shell commands instead of the corresponding macros:
  http://fedoraproject.org/wiki/PackagingGuidelines#Macros

- As there's no BuildRoot field in your spec file, you probably don't plan to 
  support EPEL < 6. Thus, you can also drop the %clean section.

- replace %defattr(-,root,root) with %defattr(-,root,root,-)

- Please be more specific in %files, i.e. prefer complete or partial 
  filenames over a generic "*" (escpecially, if only a single file is added).

- Drop the asterisk from %{_datadir}/%{name}/* for proper directory ownership.

- Why do you need the shell script "flyback"? Wouldn't be a symlink 
  sufficient?

Comment 8 Heiko Adams 2011-07-27 12:12:17 UTC
I'm very busy atm but as soon as I've got more free time I'll try to apply your suggestions. But in the Meantime you could tell me what's the best way to create a symlink in a specfile ;-)

Comment 9 Martin Gieseking 2011-07-27 12:22:13 UTC
(In reply to comment #8)
> But in the Meantime you could tell me what's the best way to
> create a symlink in a specfile ;-)

Sure. Something like this in %install should do the job:
ln -s %{_datadir}/%{name}/flyback.py %{buildroot}%{_bindir}/flyback

Comment 11 Volker Fröhlich 2011-08-27 06:42:48 UTC
You're still using macros for mkdir and install. Replace that with commands. As Martin also mentioned, the clean section is still there.

The defattr line was found to be no longer necessary some time ago.

"Apple's Time Machine for Linux" -- That comment in the desktop file is inacceptable. See http://fedoraproject.org/wiki/PackagingGuidelines#Trademarks_in_Summary_or_Description

The chmod doesn't seem to do anything.

Comment 13 Volker Fröhlich 2011-09-11 17:21:39 UTC
You're requiring Python twice.

You still have the clean section, you're still having the install macros. Defattr is still not necessary.

Changelog entries like "minor changes" and comments like "fix some paths" are pointless, in my opinion. Please be a bit more concrete!

Comment 14 Till Maas 2011-11-19 20:10:13 UTC
Heiko, please remove 'StalledSubmitter' from the whiteboard when you performed the changes suggested in comment:13.

Comment 16 Till Maas 2011-11-19 21:12:28 UTC
Some preliminary comments:

- There is a spelling error: it's 'deprecated' and not 'deprecheated'
- Leave the build section empty and add a comment that it is not needed instead of commenting it out. This will remove the following rpmlint warning:flyback.spec: W: no-%build-section
- This seems to be wrong/not needed. Why do you do this?
 #add python headlines to python-scripts
for file in src/*.py; do
   sed -i -e '1i#!/usr/bin/env python2' $file
done
- Why do you still use %{__install}?
 %{__install} -p -m 755 src/*.py %{buildroot}%{_datadir}/%{name}/

- The mode in the install command is wrong for all files except flyback.py. The other files do not need to be executable, therfore mode 644 is correct.

- Actually you do not need to require python at all, because of the shebang in flyback.py /usr/bin/python will already be required automatically

- The license is still problematic. According to the Google Project Page it is GPLv2 and not GPL+. Since the actual source does not contain any license header, it is afaik not even properly licensed as GPL+. Would you please ask upstream to add license headers as documented in http://www.gnu.org/licenses/gpl-howto.html ?

- It seems that flyback upstream is not active anymore. Is this package really worht being added to Fedora?

Comment 17 Till Maas 2011-11-19 21:15:43 UTC
And please perform informal reviews and link to them here as proposed in http://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group#Reviewing_packages

Comment 18 Heiko Adams 2011-11-19 22:17:51 UTC
You're right. Upstream seems to be allmost dead. So for me there isn't any problem to move this review request to /dev/null


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