Bug 478432 - Review Request: dwscan - Displays access point information
Review Request: dwscan - Displays access point information
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Extras Quality Assurance
:
Depends On: 478300
Blocks:
  Show dependency treegraph
 
Reported: 2008-12-29 18:01 EST by Fabian Affolter
Modified: 2009-04-09 12:16 EDT (History)
4 users (show)

See Also:
Fixed In Version: 0.2-3.fc10
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-04-09 12:16:17 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tibbs: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Fabian Affolter 2008-12-29 18:01:16 EST
Spec URL: http://fab.fedorapeople.org/packages/SRPMS/dwscan.spec
SRPM URL: http://fab.fedorapeople.org/packages/SRPMS/dwscan-0.2-1.fc9.src.rpm

Project URL: http://dag.wieers.com/home-made/dwscan/

Description:
Dwscan displays access point information in a useful manner.

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

rpmlint output:
[fab@laptop024 noarch]$ rpmlint dwscan-0.2-1.fc9.noarch.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

[fab@laptop024 SRPMS]$ rpmlint dwscan-0.2-1.fc9.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
Comment 1 Ralf Corsepius 2008-12-30 04:23:40 EST
Some remarks:

- Your patch is superfluous.
  Overriding at make-time:
  make install bindir=%{_bindir}
  has the same effect without hacking the code.

- The Makefile doesn't honor INSTALL 
  => make INSTALL="install -p"
  is non-functional

Furthermore:
- I am having strong doubts if installing this tool to %{_bindir} is a good idea.
Comment 2 Fabian Affolter 2008-12-30 05:12:59 EST
Thanks Ralf for your inputs.

(In reply to comment #1)
> - Your patch is superfluous.
>   Overriding at make-time:
>   make install bindir=%{_bindir}
>   has the same effect without hacking the code.

It's much smarter.  Changed
 
> - The Makefile doesn't honor INSTALL 
>   => make INSTALL="install -p"
>   is non-functional

fixed
 
> Furthermore:
> - I am having strong doubts if installing this tool to %{_bindir} is a good
> idea.

What location do you have in mind?

Here are the updated files.

Spec URL: http://fab.fedorapeople.org/packages/SRPMS/dwscan.spec
SRPM URL: http://fab.fedorapeople.org/packages/SRPMS/dwscan-0.2-2.fc9.src.rpm
Comment 3 Jason Tibbitts 2009-03-09 21:51:12 EDT
I'm guessing that Ralf's concern was that the program needs privilege to run and so should be installed in /usr/sbin.  Of course, the current default path kind of eliminates the difference, but upstream seems to agree.  Do you have any thoughts on this?

The software seems confused about its license.  The actual program clearly indicates LGPLv2 (only) but COPYING and the included spec say GPLv2.  The software itself overrides, of course, but then I don't see the value in packaging the useless COPYING file.  Could you ping upstream and see if you can get them to clarify?

Are you sure this really needs python to build?  It doesn't seem to me like it does.

* source files match upstream.  sha256sum:
   84d2161d1b9714d4517cef811a35fe53fdddfd5b22a2693811ccae02ed1ef779  
   dwscan-0.2.tar.bz2
* package meets naming and versioning guidelines.
 specfile is properly named, is cleanly written and uses macros consistently.
* summary is OK.
* description is OK.
* dist tag is present.
* build root is OK.
X license field does not match the actual license.
* license is open source-compatible.
* latest version is being packaged.
X BuildRequires
* %clean is present.
* package builds in mock (rawhide, x86_64).
* package installs properly.
* rpmlint is silent.
* final provides and requires are sane:
   dwscan = 0.2-2.fc11
  =
   /usr/bin/python
   python-wifi

* owns the directory it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* no generically named files
* code, not content.
* documentation is small, so no -doc subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.


The package review process needs reviewers!  If you haven't done any package
reviews recently, please consider doing one.
Comment 4 Ralf Corsepius 2009-03-10 01:56:49 EDT
(In reply to comment #3)
> I'm guessing that Ralf's concern was that the program needs privilege to run
> and so should be installed in /usr/sbin.
Right.

> Of course, the current default path
> kind of eliminates the difference, 
Well, I guess my opinion on this is known: I consider it to be a severe mistake. 

> but upstream seems to agree.
I am not familiar with this particular package's upstream, however many upstreams simply tend to not taking "system integration" consideration into account.
Comment 5 Jason Tibbitts 2009-03-10 10:35:15 EDT
My evidence that upstream seems to agree is simply that the makefile installs the binary into "sbindir".  Only the passing of sbindir="%{_bindir} in the make call gets the executable into /usr/bin.
Comment 6 Fabian Affolter 2009-03-15 10:37:26 EDT
(In reply to comment #3)
> I'm guessing that Ralf's concern was that the program needs privilege to run
> and so should be installed in /usr/sbin.  Of course, the current default path
> kind of eliminates the difference, but upstream seems to agree.  Do you have
> any thoughts on this?

I will leave the path untouched.

> The software seems confused about its license.  The actual program clearly
> indicates LGPLv2 (only) but COPYING and the included spec say GPLv2.  The
> software itself overrides, of course, but then I don't see the value in
> packaging the useless COPYING file.  Could you ping upstream and see if you can
> get them to clarify?

COPYING removed and LGPLv2 added as license tag for now till upstream release a new source tarball.

> X license field does not match the actual license.

LGPLv2 for now.

> X BuildRequires

fixed


Updated files:
Spec URL: http://fab.fedorapeople.org/packages/SRPMS/dwscan.spec
SRPM URL: http://fab.fedorapeople.org/packages/SRPMS/dwscan-0.2-3.fc10.src.rpm
Comment 7 Jason Tibbitts 2009-03-24 17:15:45 EDT
Could you explain in the spec why you wish to override upstream's placement of the binary in /usr/sbin, then?  I'm not sure there's really any reasonable justification for it, but if you have one then I'll be happy to approve the package.
Comment 8 Fabian Affolter 2009-03-24 17:30:07 EDT
(In reply to comment #7)
> Could you explain in the spec why you wish to override upstream's placement of
> the binary in /usr/sbin, then?  I'm not sure there's really any reasonable
> justification for it, but if you have one then I'll be happy to approve the
> package.  


I will not override the placement of the binary.  I reversed this in Release 3 of the package.
Comment 9 Jason Tibbitts 2009-03-24 18:28:48 EDT
Sorry, I must have misunderstood your comment.  Everything looks fine to me now.

APPROVED
Comment 10 Fabian Affolter 2009-03-24 18:37:54 EDT
Thanks again for your review.
Comment 11 Fabian Affolter 2009-03-24 18:38:43 EDT
New Package CVS Request
=======================
Package Name: dwscan 
Short Description: Displays access point information
Owners: fab
Branches: F-9 F-10
InitialCC:
Comment 12 Kevin Fenzi 2009-03-27 16:31:22 EDT
cvs done.
Comment 13 Fedora Update System 2009-03-28 17:36:24 EDT
shtool-2.0.8-2.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/shtool-2.0.8-2.fc9
Comment 14 Fedora Update System 2009-03-28 17:36:30 EDT
shtool-2.0.8-2.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/shtool-2.0.8-2.fc10
Comment 15 Fedora Update System 2009-03-28 17:38:31 EDT
dwscan-0.2-3.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/dwscan-0.2-3.fc9
Comment 16 Fedora Update System 2009-03-28 17:38:36 EDT
dwscan-0.2-3.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/dwscan-0.2-3.fc10
Comment 17 Fedora Update System 2009-03-31 16:35:41 EDT
dwscan-0.2-3.fc10 has been pushed to the Fedora 10 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update dwscan'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2009-3162
Comment 18 Fedora Update System 2009-03-31 16:36:28 EDT
dwscan-0.2-3.fc9 has been pushed to the Fedora 9 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing-newkey update dwscan'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F9/FEDORA-2009-3119
Comment 19 Fedora Update System 2009-04-09 12:16:11 EDT
dwscan-0.2-3.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 20 Fedora Update System 2009-04-09 12:16:23 EDT
dwscan-0.2-3.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.