Bug 689961

Summary: Review Request: lego-udevrules - Provide access to LEGO robots and controller boards
Product: [Fedora] Fedora Reporter: Martin Langhoff <martin>
Component: Package ReviewAssignee: Nobody's working on this, feel free to take it <nobody>
Status: CLOSED NOTABUG QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: elad, fedora, fedora-package-review, mario.blaettermann, notting, theo148
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2013-01-14 12:01:44 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 Martin Langhoff 2011-03-22 17:17:57 EDT
Spec URL: http://dev.laptop.org/git/packages/robots-udevrules/tree/
SRPM URL: http://dev.laptop.org/~martin/public_rpms/lego-udevrules-1.0-1.fc14.src.rpm
Description: udev rules granting access to LEGO NXT and WeDo robots.

This trivial package will allow the existing nxt_python and nbc packages coexist. OLPC's current efforts will probably bring about a few more packages that control the same robots/boards.

I am working at the same time on revisions to nbc (with its maintainer) and I am current maintainer of nxt_python.
Comment 1 Theodore Lee 2011-05-18 08:48:08 EDT
I'm an unsponsored packager, so this review is purely informal.

MUST Items
==========

! - rpmlint must be run on all rpms

$ rpmlint lego-udevrules-1.0-1.fc15.src.rpm
lego-udevrules.src: W: spelling-error %description -l en_US udev -> devout
lego-udevrules.src: W: no-%build-section
lego-udevrules.src:8: W: mixed-use-of-spaces-and-tabs (spaces: line 8, tab: line 1)
1 packages and 0 specfiles checked; 0 errors, 3 warnings.

$ rpmlint lego-udevrules-1.0-1.fc15.noarch.rpm
lego-udevrules.noarch: W: non-conffile-in-etc /etc/udev/rules.d/30-lego.rules
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

After install:
$ rpmlint lego-udevrules
lego-udevrules.noarch: W: non-conffile-in-etc /etc/udev/rules.d/30-lego.rules
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

OK - Package must meet naming guidelines
OK - Spec file name must match base package name
OK - Package must meet packaging guidelines
OK - Package must meet licensing guidelines
OK - License tag must match actual license
N/A - Any license files must be in %doc
OK - Spec file must be in American English
OK - Spec file must be legible
N/A - Sources must match upstream
OK - Package must build on at least one primary arch

Builds in mock on x86_64.

N/A - Arches that the package doesn't build on must be excluded with a relevant bug
OK - All necessary build dependencies must be in BuildRequires
N/A - Locales must be handled properly
N/A - Binary rpms containing libraries must call ldconfig
OK - Package must not bundle system libraries
N/A - Relocatable packages must have rationalization
OK - Package must own all directories it creates
OK - Package must not list a file more than once in %files
OK - Files must have correct permissions
OK - Macros must be consistent
OK - Package must contain code or permissible content
N/A - Large documentation files must be in a -doc subpackage
OK - %doc files must not affect program operation
N/A - Header files must be in a -devel subpackage
N/A - Static libraries must be in a -static package
N/A - Library files that end in .so must go in a -devel package
N/A - -devel packages must require the base package using a fully versioned dependency
OK - Package must NOT contain any .la libtool archives
N/A - Packages containing GUI applications must include a %{name}.desktop file
OK - Packages must not own files or directories already owned by other packages
OK - All filenames in rpm packages must be valid UTF-8

SHOULD Items
============

! - If the package is missing license text in a separate file, the packager should query upstream for it
N/A - Description and summary should contain translations if available
OK - Package should build in mock
N/A - Package should build on all supported architectures
OK - Package should function as described

Tested with an NXT Intelligent Brick:

# stat /dev/bus/usb/002/006
  File: `/dev/bus/usb/002/006'
  Size: 0         	Blocks: 0          IO Block: 4096   character special file
Device: 5h/5d	Inode: 858980      Links: 1     Device type: bd,85
Access: (0664/crw-rw-r--)  Uid: (    0/    root)   Gid: (  486/    lego)
Access: 2011-05-18 20:39:42.474714231 +0800
Modify: 2011-05-18 20:39:42.474714231 +0800
Change: 2011-05-18 20:39:42.474714231 +0800
 Birth: -

The mode key doesn't seem to be applied, but I'm not sure that's an issue.

OK - Scriptlets should be sane
N/A - Non-devel subpackages should require the base package with a full version
N/A - pkgconfig files should be placed appropriately
N/A - File dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin should require package instead
N/A - Binaries/scripts should have man pages

Issues
======

1) The package should contain a %build section, even if it's empty.

2) There's some rogue indentation on line 8.

3) The package is missing a license file - it does seem like overkill for a couple of udev rules though.

I don't see any blocking issues here, so were I a package maintainer, I would declare this package APPROVED.
Comment 2 Elad Alfassa 2011-06-16 05:13:32 EDT
(In reply to comment #1)
> I don't see any blocking issues here, so were I a package maintainer, I would
> declare this package APPROVED.
Well, you are wrong. 
the package MUST contain a license file (unless it's public domain). 
It also must have a build section.

Also, The user creation in %pre is not according to the guidelines, see
https://fedoraproject.org/wiki/Packaging:UsersAndGroups

Furthermore, it seems you get the sources directly from that git repository. why? Upstream doesn't release tarballs? if so, the version tag should be according to these guidelines: https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Snapshot_packages



-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers
Comment 3 Martin Langhoff 2011-08-15 14:17:25 EDT
Thanks for the feedback. Will rework.

Note -- this isn't incporporating sources from an upstream, it's a standalone file of obvious content. I'll mark it as BSD licensed if required.
Comment 4 Mario Blättermann 2012-09-25 15:36:36 EDT
Any progress here...? As far as I can see, the package is still missing a license definition. Use BSD, MIT, whatever.

For the project URL, just use http://dev.laptop.org/git/packages/robots-udevrules/tree/ and for the source URL http://dev.laptop.org/git/packages/robots-udevrules/plain/30-lego.rules. This would clarify the situation. In any case, it would be better to build a tarball including the *.rules file, the license text and a small README which can be added to %doc.

Moreover, it would be nice to have a direct download link to your spec:
http://dev.laptop.org/git/packages/robots-udevrules/plain/lego-udevrules.spec
Comment 5 Mario Blättermann 2012-10-29 17:06:27 EDT
Ping...?
Comment 6 Mario Blättermann 2013-01-14 12:01:44 EST
This review request seems to be dead. I close it as FE-DEADREVIEW.