Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 689961 - Review Request: lego-udevrules - Provide access to LEGO robots and controller boards
Summary: Review Request: lego-udevrules - Provide access to LEGO robots and controller...
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: FE-DEADREVIEW
TreeView+ depends on / blocked
 
Reported: 2011-03-22 21:17 UTC by Martin Langhoff
Modified: 2015-01-26 16:37 UTC (History)
6 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2013-01-14 17:01:44 UTC


Attachments (Terms of Use)

Description Martin Langhoff 2011-03-22 21:17:57 UTC
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 12:48:08 UTC
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 09:13:32 UTC
(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 18:17:25 UTC
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 19:36:36 UTC
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 21:06:27 UTC
Ping...?

Comment 6 Mario Blättermann 2013-01-14 17:01:44 UTC
This review request seems to be dead. I close it as FE-DEADREVIEW.


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