Bug 495902

Summary: Review Request: olpc-kbdshim - grab key and better rotation support for the XO laptop
Product: [Fedora] Fedora Reporter: Paul Fox <pgf>
Component: Package ReviewAssignee: Christoph Wickert <cwickert>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: rawhideCC: abo, cwickert, fedora-package-review, mail, notting, sascha-web-bugzilla.redhat.com, sebastian
Target Milestone: ---Flags: cwickert: fedora‑review+
kevin: fedora‑cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: 6-4.fc9 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2009-06-15 22:16:55 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---

Description Paul Fox 2009-04-15 10:07:03 EDT
Spec URL: http://dev.laptop.org/~pgf/rpms/srpms/olpc-kbdshim.spec
SRPM URL: http://dev.laptop.org/~pgf/rpms/srpms/olpc-kbdshim-4-1.src.rpm
Description: The olpc-kbdshim-hal daemon integrates with hal to monitor the keyboard and touchpad, enabling the XO "grab" keys and touchpad rotation (to match screen rotation), and reporting user (in)activity.  The (in)activity reports are used by olpc-powerd, a companion package.

This package is aimed specifically at the OLPC XO laptop.

This is my first review submission: sponsor needed.
Comment 1 Fabian Affolter 2009-05-02 14:20:17 EDT
Just some quick comments on your spec file.

- The license is GPLv2+ (see source header, 'either version 2 of the License, or
 * (at your option) any later version.') not GPLv2
- 'Release: 1' should be 'Release: 1%{?dist}'
- 'Source0' should point to the upstream location of the source tarball if possible
  https://fedoraproject.org/wiki/Packaging/SourceURL
- There is 'gcc' mentioned in the 'BuildRequires' this is not needed
  https://fedoraproject.org/wiki/Packaging:Guidelines#Exceptions_2
- Please replace 'BuildArch' with 'ExcludeArch'
  https://fedoraproject.org/wiki/Architectures#ExcludeArch_.26_ExclusiveArch
- 'Provides: olpc-kbdshim = 4' is not needed
  https://fedoraproject.org/wiki/How_to_create_an_RPM_package#Other_tags
- README is missing in %doc
- You must use macros in the %files section
  https://fedoraproject.org/wiki/Packaging:Guidelines#Macros
Comment 2 Paul Fox 2009-05-02 19:34:42 EDT
(In reply to comment #1)
> Just some quick comments on your spec file.
> 
thanks!

> - The license is GPLv2+ (see source header, 'either version 2 of the License,
> or
>  * (at your option) any later version.') not GPLv2

yup.

> - 'Release: 1' should be 'Release: 1%{?dist}'

okay.  i can't find a reference for %{dist}.  when is it set?

> - 'Source0' should point to the upstream location of the source tarball if
> possible
>   https://fedoraproject.org/wiki/Packaging/SourceURL

there won't normally be an upstream tarball location.  the srpm comes from the same place the tarball would -- i.e., git.  so i commented Source0 as in:
    https://fedoraproject.org/wiki/Packaging/SourceURL#Using_Revision_Control
is that okay?

> - There is 'gcc' mentioned in the 'BuildRequires' this is not needed
>   https://fedoraproject.org/wiki/Packaging:Guidelines#Exceptions_2

okay.

> - Please replace 'BuildArch' with 'ExcludeArch'
>   https://fedoraproject.org/wiki/Architectures#ExcludeArch_.26_ExclusiveArch

i suppose.  but this package really is intended just for the XO laptop -- it specifically supports the "Grab" keys on the XO keyboard, and handles rotation of the touchpad and local bezel-mounted arrow pad when the XO "rotate" button is used.  i didn't see a point in building for non-i386 platforms.  should i simply remove the BuildArch line?  i confess i won't be much interested in fixing build problems for architectures on which this will never run.

> - 'Provides: olpc-kbdshim = 4' is not needed
>   https://fedoraproject.org/wiki/How_to_create_an_RPM_package#Other_tags

okay

> - README is missing in %doc

yup.

> - You must use macros in the %files section
>   https://fedoraproject.org/wiki/Packaging:Guidelines#Macros  

oops.  rpmlint told me about libdir, but it didn't occur to me to look for the others.

what's the next step?  after fixing, do i build new rpms for you (or someone else) to look at?  or would just a spec file be enough?

-paul
Comment 3 Christoph Wickert 2009-05-02 20:49:43 EDT
(In reply to comment #2)
> 
> okay.  i can't find a reference for %{dist}.  when is it set?

in rpmmmacros, see http://fedoraproject.org/wiki/Packaging:DistTag


> there won't normally be an upstream tarball location.  the srpm comes from the
> same place the tarball would -- i.e., git.  so i commented Source0 as in:
>     https://fedoraproject.org/wiki/Packaging/SourceURL#Using_Revision_Control
> is that okay?

Yes, but add a comment how the tarball was generated.
> 
> > - Please replace 'BuildArch' with 'ExcludeArch'
> >   https://fedoraproject.org/wiki/Architectures#ExcludeArch_.26_ExclusiveArch
> 
> i suppose.  but this package really is intended just for the XO laptop -- it
> specifically supports the "Grab" keys on the XO keyboard, and handles rotation
> of the touchpad and local bezel-mounted arrow pad when the XO "rotate" button
> is used.  i didn't see a point in building for non-i386 platforms.  should i
> simply remove the BuildArch line?  i confess i won't be much interested in
> fixing build problems for architectures on which this will never run.


> what's the next step?  after fixing, do i build new rpms for you (or someone
> else) to look at?  or would just a spec file be enough?

We are reviewing rpms, not specs, so please build a new package for me to review. I can also take over sponsorship. And of course I have to test this on my XO first.

URL does not work. 

And worst of all this patching thing wont work. How is one supposed to uninstall it? You need at least make a backup of the file that is restored when the package is uninstalled. Even this is *very* dirty, i never want to see anything like that in Fedora, but maybe we can make an exception for OLPC. Alternatives would be much better.
Comment 4 Paul Fox 2009-05-03 00:16:13 EDT
(In reply to comment #3)
> (In reply to comment #2)
> > 
> > okay.  i can't find a reference for %{dist}.  when is it set?
> 
> in rpmmmacros, see http://fedoraproject.org/wiki/Packaging:DistTag
>

thanks.  i seem to have a lot of trouble googling for this kind of thing in the fedora docs.

> 
> > there won't normally be an upstream tarball location.  the srpm comes from the
> > same place the tarball would -- i.e., git.  so i commented Source0 as in:
> >     https://fedoraproject.org/wiki/Packaging/SourceURL#Using_Revision_Control
> > is that okay?
> 
> Yes, but add a comment how the tarball was generated.

oh!  sorry.  iadded just such a comment, but after i released the version you're reviewing.  my mistake.  it now says:  "the source tarball is created by "make tarball" from within a clone of this git tree: git://dev.laptop.org/users/pgf/olpc-kbdshim"

> > what's the next step?  after fixing, do i build new rpms for you (or someone
> > else) to look at?  or would just a spec file be enough?
> 
> We are reviewing rpms, not specs, so please build a new package for me to
> review. I can also take over sponsorship.

okay.  i'll rebuild after cleaning up the next two issues.

> And of course I have to test this on
> my XO first.
> 
> URL does not work.

it doesn't?  what result do you get?
  URL: http://dev.laptop.org/git/users/pgf/olpc-kbdshim/tree/README
my understanding is that the URL: should lead to more information describing the package.  since this package doesn't have a home page, a link to the README seemed appropriate.  not acceptable?
  
> 
> And worst of all this patching thing wont work. How is one supposed to
> uninstall it? You need at least make a backup of the file that is restored when
> the package is uninstalled. Even this is *very* dirty, i never want to see
> anything like that in Fedora, but maybe we can make an exception for OLPC.
> Alternatives would be much better.  

i expected some issue with that. in defense, the code added by the patch is conditioned on a check for the existence of a script which is installed by my package, and so is benign after olpc-kbdshim is uninstalled. if this is insufficient, i guess i can make a backup, but that feels even uglier to me.

paul
Comment 5 Paul Fox 2009-05-03 00:23:56 EDT
> > URL does not work.
> 
> it doesn't?  what result do you get?
>   URL: http://dev.laptop.org/git/users/pgf/olpc-kbdshim/tree/README
> my understanding is that the URL: should lead to more information describing
> the package.  since this package doesn't have a home page, a link to the README
> seemed appropriate.  not acceptable?

argh!  i seem to have fixed the URL after releasing as well.  sigh.  you're
right -- it was certainly broken before.

i'll rebuild.
Comment 6 Paul Fox 2009-05-03 00:51:59 EDT
new spec file and rpm are here:

Spec URL: http://dev.laptop.org/~pgf/rpms/srpms/olpc-kbdshim.spec-5-1
SRPM URL: http://dev.laptop.org/~pgf/rpms/srpms/olpc-kbdshim-5-1.src.rpm

(the Version changed because this picked up a recent code change.)
Comment 7 Alexander Boström 2009-05-03 04:58:55 EDT
(In reply to comment #3)

> Even this is *very* dirty, i never want to see
> anything like that in Fedora, but maybe we can make an exception for OLPC.

Don't do it. Just file a bug against the sugar package with the patch in it. 

Though I have a feeling the patch should also contain code to actually check if it's XO1 hardware, since olpc-rotate could be installed for some other reason.
Comment 8 Paul Fox 2009-05-03 10:14:18 EDT
okay -- i get the message.  :-)

a sugar bug was filed some time ago, and the patch mechanism was always intended as an interim solution.  i have some new ideas on how things should work, and since olpc-powerd (the next package i'll be submitting for review) has a similar patching issue, i'll solve them both before continuing.

many thanks for the comments thus far!

paul
Comment 9 Paul Fox 2009-05-04 13:52:30 EDT
i've now eliminated the need for the patch to sugar without a lot of disruption.  olpc-kbdshim-hal now takes care of the rotation and brightness bindings itself.

the new spec file and rpm are here.  i think this catches me up with the current set of review comments.

Spec URL: http://dev.laptop.org/~pgf/rpms/srpms/olpc-kbdshim.spec-6-1
SRPM URL: http://dev.laptop.org/~pgf/rpms/srpms/olpc-kbdshim-6-1.src.rpm

(and binary rpm in the parent)

-paul
Comment 10 Paul Fox 2009-05-27 13:20:02 EDT
this review has been stalled for several weeks.  i'm hoping that soon some of the pressure will ease on fedora folk, and that perhaps we can pick it up again?

the above URLs for the 6-1 version of the package are still valid, but on checking just now, there was a discrepancy between the sizes of what was on the web vs. what was in my build tree, so i've rebuilt and re-uploaded to dev.laptop.org, just in case.
Comment 11 Christoph Wickert 2009-06-05 03:13:14 EDT
Sorry it took so long, but I wonder why I did not get notified about your latest comment.
Comment 12 Christoph Wickert 2009-06-05 05:26:03 EDT
REVIEW FOR a114ac5ea879e928955eec89863f9009  olpc-kbdshim-6-1.src.rpm


OK - MUST: rpmlint must be run on every package.
$ rpmlint /var/lib/mock/fedora-rawhide-i386/result/olpc-kbdshim-*
3 packages and 0 specfiles checked; 0 errors, 0 warnings.
OK - MUST: The package is named according to the Package Naming Guidelines.
OK - MUST: The spec file name matches the base package %{name}, in the format %{name}.spec.
FIX - MUST: The package does not meet the Packaging Guidelines, explained below.
OK - MUST: The package is licensed with a Fedora approved license and meets the Licensing Guidelines: GPLv2+
OK - MUST: The License field in the package spec file matches the actual license.
OK - MUST: The license file from the source package is included in %doc.
OK - MUST: The spec file is in American English.
OK - MUST: The spec file for the package is legible (could be a little better though)
N/A - MUST: The sources used to build the package match the upstream source by MD5
OK - MUST: The package successfully compiles and builds into binary rpms on i386
N/A - MUST: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch.
OK - MUST: All build dependencies are listed in BuildRequires.
N/A - MUST: The spec file handles locales properly with the %find_lang macro.
N/A - MUST: Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun.
N/A - MUST: If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the rationalization for relocation of that specific package.
OK - MUST: The package owns all directories that it creates: none except in docdir.
OK - MUST: The package does not contain any duplicate files in the %files listing.
OK - MUST: Permissions on files are set properly. Every %files section includes a %defattr(...) line.
OK - MUST: The package has a %clean section, which contains rm -rf $RPM_BUILD_ROOT.
OK - MUST: The package consistently uses macros, as described in the macros section of Packaging Guidelines.
OK - MUST: The package contains code, or permissable content.
N/A - MUST: Large documentation files should go in a -doc subpackage.
OK - MUST: Files included as %doc do not affect the runtime of the application.
N/A - MUST: Header files must be in a -devel package.
N/A - MUST: Static libraries must be in a -static package.
N/A - MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'.
N/A - MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package.
N/A - MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release}
OK - MUST: The package does not contain any .la libtool archives.
N/A - MUST: Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section.
OK - MUST: The packages does not own files or directories already owned by other packages.
OK - MUST: At the beginning of %install, the package runs rm -rf $RPM_BUILD_ROOT.
OK - MUST: All filenames in rpm packages are valid UTF-8.


SHOULD Items:
N/A - SHOULD: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it.
N/A - SHOULD: The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available.
OK - SHOULD: The the package builds in mock.
OK - SHOULD: The package should compile and build into binary rpms on all supported architectures.
TBD - SHOULD: The package functions as described.
N/A - SHOULD: If scriptlets are used, those scriptlets must be sane.
N/A - SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency.
N/A - SHOULD: The placement of pkgconfig(.pc) files depends on their usecase, and this is usually for development purposes, so should be placed in a -devel pkg.
N/A - SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself.


Issues:
- MINOR: BuildRoot: should be
%(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)
see https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag
(minor, but for a new package we should do this properly I think)

- MAJOR: BuildArch: i386 is wrong, I think you want %{ix86} or no BuildArch at all.

- MAJOR: Requires: hal is missing for proper function and dir ownership

- MINOR: Description: line breaks at 80 characters

- MAJOR: RPM_OPT_FLAGS not honored:
cc -Wall -O2 -g -DVERSION=6 $(pkg-config --cflags hal) $(pkg-config --cflags glib-2.0) $(pkg-config --cflags dbus-glib-1)     olpc-kbdshim-hal.c  $(pkg-config --libs hal) $(pkg-config --libs glib-2.0) $(pkg-config --libs dbus-glib-1) -o olpc-kbdshim-hal
cc -Wall -O2 -g -DVERSION=6    olpc-kbdshim.c   -o olpc-kbdshim

- MAJOR: Preserve timestamps during install by adding "-p", see
https://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps


Questions:
- Is the non-hal version still developed? If so, this package should be named olpc-kbdshim-hal, so that both version could be packaged

- Is there any way to tag or mark the checkouts, so we can later get a specific version and verify it's md5? For a git snapshot this package is not named properly, see http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Snapshot_packages

- How can I test the program's functionality? I'm especially interested in the brightness because this is something I need for LXDE and Xfce as well.
Comment 13 Paul Fox 2009-06-05 09:50:45 EDT

many thanks!

> 
> Issues:
> - MINOR: BuildRoot: should be
> %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)
> see https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag
> (minor, but for a new package we should do this properly I think)

fixed.

> 
> - MAJOR: BuildArch: i386 is wrong, I think you want %{ix86} or no BuildArch at
> all.

fixed.

> 
> - MAJOR: Requires: hal is missing for proper function and dir ownership

fixed.

> 
> - MINOR: Description: line breaks at 80 characters

i'm not sure what you mean.  are you saying my line breaks shouldn't be there?  won't that make the spec file unreadable?

> 
> - MAJOR: RPM_OPT_FLAGS not honored:
> cc -Wall -O2 -g -DVERSION=6 $(pkg-config --cflags hal) $(pkg-config --cflags
> glib-2.0) $(pkg-config --cflags dbus-glib-1)     olpc-kbdshim-hal.c 
> $(pkg-config --libs hal) $(pkg-config --libs glib-2.0) $(pkg-config --libs
> dbus-glib-1) -o olpc-kbdshim-hal
> cc -Wall -O2 -g -DVERSION=6    olpc-kbdshim.c   -o olpc-kbdshim

fixed.

> 
> - MAJOR: Preserve timestamps during install by adding "-p", see
> https://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps

fixed, (though i'm opposed on principle.  i think the usefulness of timestamps is subverted almost entirely by making it appear as if files are older than they actually are.  but i'll adapt, i guess.  :-)

> 
> 
> Questions:
> - Is the non-hal version still developed? If so, this package should be named
> olpc-kbdshim-hal, so that both version could be packaged

no, only the hal version is being maintained.  i've removed the commented lines in the spec file, and added a note to the README noting the existence of the non-HAL version, just in case someone wants it.

> 
> - Is there any way to tag or mark the checkouts, so we can later get a specific
> version and verify it's md5? For a git snapshot this package is not named
> properly, see
> http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Snapshot_packages

i'll research this.  i confess it's a little confusing as the maintainer to be doing a release based on a sandbox, rather than on a tarball.  can you perhaps point me at a (simple) git-based package that does this correctly?  a template would help me here.

> 
> - How can I test the program's functionality? I'm especially interested in the
> brightness because this is something I need for LXDE and Xfce as well.  

there's no configuration:  after installation and a reboot, the following things should work:
 - with any grab key pressed, both the touchpad and the arrow keys should
    cause scrolling.  on a USB keyboard, the "windows" keys will act as grab
    keys.
 - the rotate and brightness keys should "just work".  these keys cause the
    olpc-rotate and olpc-brightness scripts in /usr/bin to be invoked.
 - when the screen is rotated, the action of the touchpad will be rotated
    as well.  the action of the "d-pad" arrow keys on the screen bezel will
    also be rotated to match.  it's possible for the touchpad and arrow actions
    to get out of sync with the screen if X crashes, but the next use of the
    rotate button will fix this.

that's it.  you should feel free to test on the current package (URL above), since none of the current review comments have affected its operation.  i'll do a new package when i've resolved the "description" and tagging issues still open above.  okay?

thanks again.
Comment 14 Christoph Wickert 2009-06-05 10:23:06 EDT
(In reply to comment #13)
> i'm not sure what you mean.  are you saying my line breaks shouldn't be there? 

No, you should use the lines up to 80 characters, yours are shorter currently.

> i think the usefulness of timestamps
> is subverted almost entirely by making it appear as if files are older than
> they actually are.  but i'll adapt, i guess.  :-)

timstamps are important for rpm to see if a file has actually changed. This is especially important on multiarch systems (so not on the XO), where you can install two versions of a package in parallel. If two files have same size and date, they will be treated as one file, if they differ, rpm will say they conflict.

> i'll research this.  i confess it's a little confusing as the maintainer to be
> doing a release based on a sandbox, rather than on a tarball.  can you perhaps
> point me at a (simple) git-based package that does this correctly?  a template
> would help me here.

Version is 6
Release is 2 (pls increase even during review)
To mark the git checkout: 20090506git
All together it becomes: olpc-kbdshim-6-2.20090506git

> there's no configuration:  after installation and a reboot, the following
> things should work:
>  - with any grab key pressed, both the touchpad and the arrow keys should
>     cause scrolling.  on a USB keyboard, the "windows" keys will act as grab
>     keys.
>  - the rotate and brightness keys should "just work".  these keys cause the
>     olpc-rotate and olpc-brightness scripts in /usr/bin to be invoked.

I don't have /usr/bin/olpc-brightness. What package is it from?

> that's it.  you should feel free to test on the current package (URL above),
> since none of the current review comments have affected its operation.  i'll do
> a new package when i've resolved the "description" and tagging issues still
> open above.  okay?

Fine with me, will test tonight.
Comment 15 Paul Fox 2009-06-05 12:45:38 EDT
(In reply to comment #14)
> (In reply to comment #13)
> > i'm not sure what you mean.  are you saying my line breaks shouldn't be there? 
> 
> No, you should use the lines up to 80 characters, yours are shorter currently.
> 

oh!  sure, i can fix that.  :-)

> > i think the usefulness of timestamps
> > is subverted almost entirely by making it appear as if files are older than
> > they actually are.  but i'll adapt, i guess.  :-)
> 
> timstamps are important for rpm to see if a file has actually changed. This is
> especially important on multiarch systems (so not on the XO), where you can
> install two versions of a package in parallel. If two files have same size and
> date, they will be treated as one file, if they differ, rpm will say they
> conflict.

okay -- i thought it was just for users.

> 
> > i'll research this.  i confess it's a little confusing as the maintainer to be
> > doing a release based on a sandbox, rather than on a tarball.  can you perhaps
> > point me at a (simple) git-based package that does this correctly?  a template
> > would help me here.
> 
> Version is 6
> Release is 2 (pls increase even during review)
> To mark the git checkout: 20090506git
> All together it becomes: olpc-kbdshim-6-2.20090506git

okay.  from now, the package includes the date and git hash (abbreviated).

> 
> > there's no configuration:  after installation and a reboot, the following
> > things should work:
> >  - with any grab key pressed, both the touchpad and the arrow keys should
> >     cause scrolling.  on a USB keyboard, the "windows" keys will act as grab
> >     keys.
> >  - the rotate and brightness keys should "just work".  these keys cause the
> >     olpc-rotate and olpc-brightness scripts in /usr/bin to be invoked.
> 
> I don't have /usr/bin/olpc-brightness. What package is it from?

yikes!  good catch.  there used to be good reason for olpc-brightness to be packaged with olpc-powerd (unreviewed), but that's no longer the case.  since i usually install both packages for testing, i hadn't noticed that olpc-brightness wasn't being installed by olpc-kbdshim.  i've fixed this, and now olpc-kbdshim installs both the olpc-rotate and olpc-brightness scripts.

please review:
http://dev.laptop.org/~pgf/rpms/srpms/olpc-kbdshim.spec-6-2.20090605git98f5b2c
http://dev.laptop.org/~pgf/rpms/srpms/olpc-kbdshim-6-2.20090605git98f5b2c.src.rpm
and test:
http://dev.laptop.org/~pgf/rpms/olpc-kbdshim-6-2.20090605git98f5b2c.fc9.i386.rpm
Comment 16 Christoph Wickert 2009-06-05 16:29:09 EDT
(In reply to comment #15)
> okay.  from now, the package includes the date and git hash (abbreviated).

excellent

> > I don't have /usr/bin/olpc-brightness. What package is it from?
> 
> yikes!  good catch.  

The whole time I had the feeling that something was missing. ;) I'm very keen on the brightness script because I need it for LXDE and Xfce as well. But to be honest I'm a little disappointed, the script could be more sophisticated IMHO, e. g. it could read max brightness from /sys/class/backlight/dcon-bl/max_brightness. Take a look at http://www.catmoran.com/olpc/#xfcebrvo

olpc-brightness is being run as root because of the permissions of /sys/class/backlight/dcon-bl/brightness, right? Is there no better way? Can we use hal to give user write permission?

Anyway, testing was positive, everything works as you described. So let's check the outstanding issues:

OK - MINOR: BuildRoot tag
OK - MAJOR: BuildArch tag
OK - MAJOR: Requires: hal added
OK - MINOR: Description: line breaks are at 80 characters
OK - MAJOR: RPM_OPT_FLAGS are honored
OK - MAJOR: Timestamps preserved

I just realized that "BuildArch: %{ix86}" is not a good idea because the buildsys will then build for i386, i486, i586, i686 and athlon. Better use 
ExclusiveArch: %{ix86}

One last thing: during build I see:
+ make
fatal: Not a git repository
fatal: Not a git repository
fatal: Not a git repository
fatal: Not a git repository
fatal: Not a git repository

I wouldn't call this a blocker, but please fix it. 


olpc-kbdshim-6-2.20090605git98f5b2c.src.rpm is APPROVED

P.S.: Please cc me if you submit olpc-powerd for review.
Comment 17 Paul Fox 2009-06-06 09:33:59 EDT
(In reply to comment #16)
> I'm a little disappointed, the script could be more sophisticated IMHO,
> e. g. it could read max brightness from
> /sys/class/backlight/dcon-bl/max_brightness. Take a look at
> http://www.catmoran.com/olpc/#xfcebrvo

i would have thought that a script that doesn't require bash, invokes no external processes (catmoran's invokes two) would be more sophisticated, not less.  :-)  i understand your point about max_brightness, but the values i hard-coded in the hardware, and i see little reason to ask the kernel for the value, in that case.

> 
> olpc-brightness is being run as root because of the permissions of
> /sys/class/backlight/dcon-bl/brightness, right? Is there no better way? Can we
> use hal to give user write permission?

we could -- in fact, the daemon could simply change the permissions itself.  are you concerned that users can't change the brightness themselves?  or that the script runs as root?

> 
> Anyway, testing was positive, everything works as you described. So let's check
> the outstanding issues:
> 
> OK - MINOR: BuildRoot tag
> OK - MAJOR: BuildArch tag
> OK - MAJOR: Requires: hal added
> OK - MINOR: Description: line breaks are at 80 characters
> OK - MAJOR: RPM_OPT_FLAGS are honored
> OK - MAJOR: Timestamps preserved
> 
> I just realized that "BuildArch: %{ix86}" is not a good idea because the
> buildsys will then build for i386, i486, i586, i686 and athlon. Better use 
> ExclusiveArch: %{ix86}

should this still use the macro in that case?

> One last thing: during build I see:
> + make
> fatal: Not a git repository
> fatal: Not a git repository
> fatal: Not a git repository
> fatal: Not a git repository
> fatal: Not a git repository
> 

fixed.

thanks again for your help.


> I wouldn't call this a blocker, but please fix it. 
> 
> 
> olpc-kbdshim-6-2.20090605git98f5b2c.src.rpm is APPROVED
> 
> P.S.: Please cc me if you submit olpc-powerd for review.
Comment 18 Christoph Wickert 2009-06-06 14:31:40 EDT
(In reply to comment #17)
> i would have thought that a script that doesn't require bash, invokes no
> external processes (catmoran's invokes two) would be more sophisticated, not
> less.  :-)  i understand your point about max_brightness, but the values i
> hard-coded in the hardware, and i see little reason to ask the kernel for the
> value, in that case.

Agreed, all valid points.

> we could -- in fact, the daemon could simply change the permissions itself. 
> are you concerned that users can't change the brightness themselves?  or that
> the script runs as root?

The former.

> should this still use the macro in that case?

The macro is useful if we decide to change our arch again, but as long as you are following Fedora's development you can also use BuildArch: i386 again, I don't really mind.
Comment 19 Paul Fox 2009-06-06 14:40:42 EDT
(In reply to comment #18)
>
> 
> > we could -- in fact, the daemon could simply change the permissions itself. 
> > are you concerned that users can't change the brightness themselves?  or that
> > the script runs as root?
> 
> The former.

okay.  i pretty much agree.  i'll think about how best to open up those permissions.

> > should this still use the macro in that case?
> 
> The macro is useful if we decide to change our arch again, but as long as you
> are following Fedora's development you can also use BuildArch: i386 again, I
> don't really mind.  

i guess i thought that if the objection to BuildArch: %{ix86} was that it would build for too many archs, that the same would be true for ExclusiveArch: %{ix86}.  i think i don't understand the different clearly enough.

paul
Comment 20 Paul Fox 2009-06-06 16:52:38 EDT
New Package CVS Request
=======================
Package Name: olpc-kbdshim
Short Description: grab key and better rotation support for the XO laptop
Owners: pgf
Branches: F-9 F-11
InitialCC: 


(i suspect that the deadline for getting packages into F-9 may have passed.  if it's possible to make an exception, i think it would be a good thing:  it would make this package available to all current owners of XO laptops immediately.)
Comment 21 Christoph Wickert 2009-06-09 17:25:26 EDT
Setting "cvs?" because Paul seems to lack privileges.
Comment 22 Kevin Fenzi 2009-06-10 00:54:10 EDT
Yeah, F-9 branches are no longer done. 
You can ask for an exception, I guess from FESCo?

File a ticket:  
http://fedorahosted.org/fesco/
Comment 23 Fedora Update System 2009-06-11 16:40:13 EDT
olpc-kbdshim-6-4.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/olpc-kbdshim-6-4.fc11
Comment 24 Christoph Wickert 2009-06-11 20:00:00 EDT
(In reply to comment #22)
> Yeah, F-9 branches are no longer done. 
> You can ask for an exception, I guess from FESCo?

Ticket filed as https://fedorahosted.org/fesco/ticket/163
Comment 25 Paul Fox 2009-06-12 13:48:47 EDT
setting cvs?, since fesco #163 was approved.
Comment 26 Christoph Wickert 2009-06-12 17:20:33 EDT
Package Change Request
======================
Package Name: olpc-kbd-shim
New Branches: F-9
Owners: pgf cwickert
Comment 27 Christoph Wickert 2009-06-12 17:21:43 EDT
Correct package name is olpc-kbdshim, sorry.
Comment 28 Kevin Fenzi 2009-06-14 14:56:47 EDT
cvs done.
Comment 29 Fedora Update System 2009-06-15 19:05:02 EDT
olpc-kbdshim-6-4.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/olpc-kbdshim-6-4.fc9
Comment 30 Fedora Update System 2009-06-15 22:16:49 EDT
olpc-kbdshim-6-4.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 31 Fedora Update System 2009-06-18 07:44:24 EDT
olpc-kbdshim-6-4.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.