Bug 713122

Summary: Review Request: flyback - Git based backup software
Product: [Fedora] Fedora Reporter: Heiko Adams <bugzilla>
Component: Package ReviewAssignee: Nobody's working on this, feel free to take it <nobody>
Status: CLOSED WONTFIX QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: unspecified    
Version: rawhideCC: fedora-package-review, herrold, mail, martin.gieseking, metherid, notting, opensource, pahan, richardfearn, volker27
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2011-11-19 18:23:28 EST Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
Bug Depends On:    
Bug Blocks: 201449    

Description Heiko Adams 2011-06-14 07:14:53 EDT
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 08:09:27 EDT
Comment 2 Kevin Kofler 2011-06-14 11:08:36 EDT
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 12:43:32 EDT
okay, will be changed with next release. Thanks for advice.
Comment 6 Fabian Affolter 2011-06-19 06:41:52 EDT
Just some quick comments:

- You are mixing macro style and variable style (${RPM_BUILD_ROOT} vs.  %{buildroot})
- Comment the patches
Comment 7 Martin Gieseking 2011-07-17 14:47:30 EDT
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:

- 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 
Comment 8 Heiko Adams 2011-07-27 08:12:17 EDT
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 08:22:13 EDT
(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 02:42:48 EDT
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 13:21:39 EDT
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 15:10:13 EST
Heiko, please remove 'StalledSubmitter' from the whiteboard when you performed the changes suggested in comment:13.
Comment 16 Till Maas 2011-11-19 16:12:28 EST
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
- 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 16:15:43 EST
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 17:17:51 EST
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