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   
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


! - 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


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

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
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:
Comment 5 Mario Blättermann 2012-10-29 17:06:27 EDT
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.