[Bug 495902] Review Request: olpc-kbdshim - grab key and better rotation support for the XO laptop
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=495902 --- Comment #31 from Fedora Update System upda...@fedoraproject.org 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. -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 495902] Review Request: olpc-kbdshim - grab key and better rotation support for the XO laptop
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=495902 Fedora Update System upda...@fedoraproject.org changed: What|Removed |Added Fixed In Version|6-4.fc11|6-4.fc9 -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 495902] Review Request: olpc-kbdshim - grab key and better rotation support for the XO laptop
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=495902 --- Comment #29 from Fedora Update System upda...@fedoraproject.org 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 -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 495902] Review Request: olpc-kbdshim - grab key and better rotation support for the XO laptop
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=495902 --- Comment #30 from Fedora Update System upda...@fedoraproject.org 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. -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 495902] Review Request: olpc-kbdshim - grab key and better rotation support for the XO laptop
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=495902 Fedora Update System upda...@fedoraproject.org changed: What|Removed |Added Status|ASSIGNED|CLOSED Fixed In Version||6-4.fc11 Resolution||NEXTRELEASE -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 495902] Review Request: olpc-kbdshim - grab key and better rotation support for the XO laptop
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=495902 Kevin Fenzi ke...@tummy.com changed: What|Removed |Added Flag|fedora-cvs? |fedora-cvs+ --- Comment #28 from Kevin Fenzi ke...@tummy.com 2009-06-14 14:56:47 EDT --- cvs done. -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 495902] Review Request: olpc-kbdshim - grab key and better rotation support for the XO laptop
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=495902 Paul Fox p...@laptop.org changed: What|Removed |Added Flag|fedora-cvs+ |fedora-cvs? --- Comment #25 from Paul Fox p...@laptop.org 2009-06-12 13:48:47 EDT --- setting cvs?, since fesco #163 was approved. -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 495902] Review Request: olpc-kbdshim - grab key and better rotation support for the XO laptop
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=495902 --- Comment #27 from Christoph Wickert fed...@christoph-wickert.de 2009-06-12 17:21:43 EDT --- Correct package name is olpc-kbdshim, sorry. -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 495902] Review Request: olpc-kbdshim - grab key and better rotation support for the XO laptop
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=495902 --- Comment #26 from Christoph Wickert fed...@christoph-wickert.de 2009-06-12 17:20:33 EDT --- Package Change Request == Package Name: olpc-kbd-shim New Branches: F-9 Owners: pgf cwickert -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 495902] Review Request: olpc-kbdshim - grab key and better rotation support for the XO laptop
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=495902 --- Comment #23 from Fedora Update System upda...@fedoraproject.org 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 -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 495902] Review Request: olpc-kbdshim - grab key and better rotation support for the XO laptop
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=495902 --- Comment #24 from Christoph Wickert fed...@christoph-wickert.de 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 -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 495902] Review Request: olpc-kbdshim - grab key and better rotation support for the XO laptop
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=495902 Christoph Wickert fed...@christoph-wickert.de changed: What|Removed |Added Flag||fedora-cvs? --- Comment #21 from Christoph Wickert fed...@christoph-wickert.de 2009-06-09 17:25:26 EDT --- Setting cvs? because Paul seems to lack privileges. -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 495902] Review Request: olpc-kbdshim - grab key and better rotation support for the XO laptop
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=495902 Kevin Fenzi ke...@tummy.com changed: What|Removed |Added Flag|fedora-cvs? |fedora-cvs+ --- Comment #22 from Kevin Fenzi ke...@tummy.com 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/ -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 495902] Review Request: olpc-kbdshim - grab key and better rotation support for the XO laptop
Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=495902
--- Comment #17 from Paul Fox p...@laptop.org 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.
--
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are on the CC list for the bug.
___
Fedora-package-review mailing list
Fedora-package-review@redhat.com
http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 495902] Review Request: olpc-kbdshim - grab key and better rotation support for the XO laptop
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=495902 --- Comment #18 from Christoph Wickert fed...@christoph-wickert.de 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. -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 495902] Review Request: olpc-kbdshim - grab key and better rotation support for the XO laptop
Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=495902
--- Comment #19 from Paul Fox p...@laptop.org 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
--
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are on the CC list for the bug.
___
Fedora-package-review mailing list
Fedora-package-review@redhat.com
http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 495902] Review Request: olpc-kbdshim - grab key and better rotation support for the XO laptop
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=495902 --- Comment #20 from Paul Fox p...@laptop.org 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.) -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 495902] Review Request: olpc-kbdshim - grab key and better rotation support for the XO laptop
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=495902 Christoph Wickert fed...@christoph-wickert.de changed: What|Removed |Added Status|NEW |ASSIGNED AssignedTo|nob...@fedoraproject.org|fed...@christoph-wickert.de Flag||fedora-review? --- Comment #11 from Christoph Wickert fed...@christoph-wickert.de 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. -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 495902] Review Request: olpc-kbdshim - grab key and better rotation support for the XO laptop
Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=495902
--- Comment #12 from Christoph Wickert fed...@christoph-wickert.de
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}-XX)
see https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag
(minor, but for a new package we should do this properly I think)
- MAJOR:
[Bug 495902] Review Request: olpc-kbdshim - grab key and better rotation support for the XO laptop
Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=495902
--- Comment #13 from Paul Fox p...@laptop.org 2009-06-05 09:50:45 EDT ---
many thanks!
Issues:
- MINOR: BuildRoot: should be
%(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XX)
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=6olpc-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.
--
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are on the CC list for the bug.
___
Fedora-package-review mailing list
Fedora-package-review@redhat.com
http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 495902] Review Request: olpc-kbdshim - grab key and better rotation support for the XO laptop
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=495902 --- Comment #14 from Christoph Wickert fed...@christoph-wickert.de 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. -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 495902] Review Request: olpc-kbdshim - grab key and better rotation support for the XO laptop
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=495902 --- Comment #15 from Paul Fox p...@laptop.org 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 -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 495902] Review Request: olpc-kbdshim - grab key and better rotation support for the XO laptop
Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=495902
Christoph Wickert fed...@christoph-wickert.de changed:
What|Removed |Added
Flag|fedora-review? |fedora-review+
--- Comment #16 from Christoph Wickert fed...@christoph-wickert.de
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.
--
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are on the CC list for the bug.
___
Fedora-package-review mailing list
Fedora-package-review@redhat.com
http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 495902] Review Request: olpc-kbdshim - grab key and better rotation support for the XO laptop
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=495902 --- Comment #10 from Paul Fox p...@laptop.org 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. -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 495902] Review Request: olpc-kbdshim - grab key and better rotation support for the XO laptop
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=495902 --- Comment #9 from Paul Fox p...@laptop.org 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 -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 495902] Review Request: olpc-kbdshim - grab key and better rotation support for the XO laptop
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=495902 Alexander Boström a...@kth.se changed: What|Removed |Added CC||a...@kth.se --- Comment #7 from Alexander Boström a...@kth.se 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. -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 495902] Review Request: olpc-kbdshim - grab key and better rotation support for the XO laptop
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=495902 --- Comment #8 from Paul Fox p...@laptop.org 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 -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 495902] Review Request: olpc-kbdshim - grab key and better rotation support for the XO laptop
Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=495902
Fabian Affolter fab...@bernewireless.net changed:
What|Removed |Added
CC||fab...@bernewireless.net
--- Comment #1 from Fabian Affolter fab...@bernewireless.net 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
--
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are on the CC list for the bug.
___
Fedora-package-review mailing list
Fedora-package-review@redhat.com
http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 495902] Review Request: olpc-kbdshim - grab key and better rotation support for the XO laptop
Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=495902
--- Comment #2 from Paul Fox p...@laptop.org 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
--
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are on the CC list for the bug.
___
Fedora-package-review mailing list
Fedora-package-review@redhat.com
http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 495902] Review Request: olpc-kbdshim - grab key and better rotation support for the XO laptop
Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=495902
--- Comment #3 from Christoph Wickert fed...@christoph-wickert.de 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.
--
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are on the CC list for the bug.
___
Fedora-package-review mailing list
Fedora-package-review@redhat.com
http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 495902] Review Request: olpc-kbdshim - grab key and better rotation support for the XO laptop
Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=495902
--- Comment #4 from Paul Fox p...@laptop.org 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
--
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are on the CC list for the bug.
___
Fedora-package-review mailing list
Fedora-package-review@redhat.com
http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 495902] Review Request: olpc-kbdshim - grab key and better rotation support for the XO laptop
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=495902 --- Comment #5 from Paul Fox p...@laptop.org 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. -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 495902] Review Request: olpc-kbdshim - grab key and better rotation support for the XO laptop
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=495902 --- Comment #6 from Paul Fox p...@laptop.org 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.) -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
