Bug 868931 - Review Request: sshuttle - Transparent Proxy VPN
Review Request: sshuttle - Transparent Proxy VPN
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Tom "spot" Callaway
Fedora Extras Quality Assurance
:
: 868930 (view as bug list)
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-22 09:40 EDT by Marcel Wysocki
Modified: 2013-01-23 17:02 EST (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-01-16 14:19:52 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tcallawa: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Marcel Wysocki 2012-10-22 09:40:34 EDT
Spec URL: http://maci.satgnu.net/rpmbuild/SPECS/sshuttle.spec
SRPM URL: http://maci.satgnu.net/rpmbuild/SRPMS/sshuttle-20121019-2.gitg9ce2fa0.fc17.src.rpm
Description: Transparent proxy server that works as a poor man's VPN. Forwards over ssh.
Doesn't require admin. Works with Linux and MacOS. Supports DNS tunneling.
Fedora Account System Username: maci

Koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=4615409


This is my first package and I need a sponsor. I have some more packages I want to get into Fedora, mainly s3ql and its dependencies ( http://code.google.com/p/s3ql/ )
Comment 1 Peter Robinson 2012-10-22 09:51:48 EDT
Marcel: what's your FAS username and I'll sponsor you.
Comment 2 Marcel Wysocki 2012-10-22 09:52:48 EDT
Fedora Account System Username: maci
Comment 3 Marcel Wysocki 2012-10-22 10:39:43 EDT
*** Bug 868930 has been marked as a duplicate of this bug. ***
Comment 4 Volker Fröhlich 2012-10-22 15:41:21 EDT
Don't use macros for rm, install, chmod and cp.

Remove defattr, if you're not going for EPEL4.

Remove the clean section if you're not going for EPEL5.
Comment 7 Marcel Wysocki 2012-10-22 17:11:18 EDT
Another update: added some more dependencies

Spec URL: http://maci.satgnu.net/rpmbuild/SPECS/sshuttle.spec
SRPM URL: http://maci.satgnu.net/rpmbuild/SRPMS/sshuttle-20121019-4.gitg9ce2fa0.fc17.src.rpm
Comment 9 Marcel Wysocki 2012-11-09 08:13:56 EST
can someone please review this ?
Comment 10 Volker Fröhlich 2012-11-22 19:25:54 EST
The files you install to /usr/lib should go to %{_datadir} instead.

Please inform upstream of the (long) outdated FSF postal address in LICENSE, if you haven't done so yet.

You might be better off, making the version 0 and add the date in front of the git hash. Otherwise, if a version 0.5, 1.0 or whatever comes out, you'd have to use epochs to make it newer than 20121019.

The Documentation directory contains a manpage. Rather install that instead of including Documentation/sshuttle.md.

Please include the complete commands to create the tarball.

You don't have to BR "make". On the one hand, I think you're not using it; on the other hand it's an exception: http://fedoraproject.org/wiki/Packaging:Guidelines#Exceptions_2
Comment 11 Marcel Wysocki 2012-11-23 05:49:37 EST
- fixed hash ( woops, where did that g come from )
- fixed date to be commit date and not clone date
- use version 0 
- use datadir instead of /usr/local
- remove sshuttle.md in favor of the manpage
- remove make from BR
- added comment on how to create the source tarball


one thing i dont know how to solve is that rpmlint complains about the comment containing a macro bcs of this line:
# checkout=`git log --pretty=format:"%adgit%h" -n1 --date=short|sed 's@-@@g'`

Update:

Spec URL: http://maci.satgnu.net/rpmbuild/SPECS/sshuttle.spec
SRPM URL: http://maci.satgnu.net/rpmbuild/SRPMS/sshuttle-0-6.20120810git9ce2fa0.fc17.src.rpm
Koji URL: http://koji.fedoraproject.org/koji/taskinfo?taskID=4720613
Comment 12 Marcel Wysocki 2012-11-23 05:50:17 EST
i also upstream about the outdated fsf address
Comment 13 Marcel Wysocki 2012-11-23 05:52:06 EST
.. woops "informed upstream", sorry for the spam
Comment 14 Tom "spot" Callaway 2013-01-04 10:13:56 EST
= Review =

Good:

- rpmlint checks return:
sshuttle.src:2: W: macro-in-comment %adgit
sshuttle.src: W: invalid-url Source0: sshuttle-0-20120810git9ce2fa0.tar.gz
sshuttle.noarch: E: incorrect-fsf-address /usr/share/doc/sshuttle-0/LICENSE
2 packages and 0 specfiles checked; 1 errors, 2 warnings.

Since you've informed upstream about the FSF address issue, and the Source0 is properly documented in comments, and the macro-in-comment warning is a false positive, all of these are safe to ignore.

- package meets naming guidelines
- package meets packaging guidelines
- license (LGPLv2+) OK, text in %doc, matches source
- spec file legible, in am. english
- source matches upstream (manual diff matches upstream checkout)
- package compiles on devel (noarch)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file 

Minor:

You have %doc %{_mandir}/man8/sshuttle.8.gz. The %doc here is redundant, everything in %{_mandir} is always automatically %doc. Just drop that before you do builds. :)

I also usually prefer that files which end up in the package are not generated within the spec, but rather, included as separate Source files. This isn't strictly against the guidelines, since there are cases where you need to define paths at buildtime (usually involving %{_libdir}), but in this case it really isn't necessary. That said, I won't make you change this behavior, it is up to you how you wish to do it.

Since you seem to already be sponsored, this package is APPROVED.
Comment 15 Marcel Wysocki 2013-01-07 05:34:26 EST
New Package SCM Request
=======================
Package Name: sshuttle
Short Description: Transparent Proxy VPN
Owners: maci
Branches: f17 f18 f19 el6
InitialCC:
Comment 16 Gwyn Ciesla 2013-01-07 08:24:23 EST
Git done (by process-git-requests).
Comment 17 Fedora Update System 2013-01-07 09:04:17 EST
sshuttle-0-7.20120810git9ce2fa0.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/sshuttle-0-7.20120810git9ce2fa0.el6
Comment 18 Fedora Update System 2013-01-07 09:05:59 EST
sshuttle-0-7.20120810git9ce2fa0.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/sshuttle-0-7.20120810git9ce2fa0.fc18
Comment 19 Fedora Update System 2013-01-07 13:54:13 EST
sshuttle-0-7.20120810git9ce2fa0.el6 has been pushed to the Fedora EPEL 6 testing repository.
Comment 20 Fedora Update System 2013-01-16 14:19:55 EST
sshuttle-0-7.20120810git9ce2fa0.fc18 has been pushed to the Fedora 18 stable repository.
Comment 21 Fedora Update System 2013-01-23 17:02:21 EST
sshuttle-0-7.20120810git9ce2fa0.el6 has been pushed to the Fedora EPEL 6 stable repository.

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