Look who's a committer now

[Joe Marcus Clarke]

On Sat, 2006-05-13 at 18:42 -0400, David Zeuthen wrote:
> On Sat, 2006-05-13 at 15:19 -0400, Joe Marcus Clarke wrote:
> > Here is my first pass at cleaning up tools.  
> 
> Thanks.
> 
> > The patch adds a freebsd
> > subdirectory under tools that stores the OS-dependent portions of the
> > scripts.  The tools/ versions of the scripts have been modified to check
> > HALD_UNAME_S, and exec the appropriate backend (if available).  I have
> > also populated the linux subdirectory with the backend scripts.
> > hald_runner.c has been updated to pass utsname.sysname to all scripts
> > via the HALD_UNAME_S environment variable (this value is always in
> > lowercase).  Finally, hal-storage-mount.c has been updated with code
> > from both Jean-Yves and myself to support FreeBSD.
> > 
> > If this is approved, I will submit an fdi patch.
> > 
> > http://www.marcuscom.com/downloads/hal.diff
> 
> Looks good, only a few comments;
> 
> > +++ tools/freebsd/Makefile.am	Sat May 13 14:58:07 2006
> > @@ -0,0 +1,17 @@
> > +## Process this file with automake to produce Makefile.in
> > +
> > +transform += ; s,$$,-freebsd,
> ...
> > +
> > +script_SCRIPTS =				\
> > +	hal-system-power-suspend		\
> ...
> 
> 
> Ought to give the OS specific scripts the -$(HALD_UNAME_S) suffix in the
> source directory too; just makes it easier if one is updating multiple
> scripts at a time. So in tools/freebsd/, hal-system-power-suspend would
> be hal-system-power-suspend-freebsd, in tools/linux it would be
> hal-system-power-suspend-linux and so forth.
> 
> 
> 
> > +++ tools/freebsd/hal-system-lcd-set-brightness	Sat May 13 14:02:06 2006
> ...
> > +# Check for values outside range
> > +if [ ${value} -lt 0 ] || [ ${value} -ge $HAL_PROP_LAPTOP_PANEL_NUM_LEVELS ]; then
> > +	echo "org.freedesktop.Hal.Device.LaptopPanel.Invalid" >&2
> > +	echo "Brightness has to be between 0 and $HAL_PROP_LAPTOP_PANEL_NUM_LEVELS!" >&2
> > +	exit 1
> > +fi
> > 
> 
> This ought to be in tools/hal-system-lcd-set-brightness as it's a check
> on general semantics specified by hal.
> 
> > +++ tools/freebsd/hal-system-power-suspend	Sat May 13 14:08:49 2006
> > @@ -0,0 +1,39 @@
> > +#!/bin/sh
> > +
> > +POWERSAVED_SUSPEND2RAM="dbus-send --system --dest=com.novell.powersave \
> > +                        --print-reply /com/novell/powersave \
> > +                        com.novell.powersave.action.SuspendToRam"
> 
> You probably don't have Novell's powersave stuff on FreeBSD so no need
> to carry this over :-)
> 
> Apart from that it looks good. Please go ahead and commit it to CVS HEAD
> and remember to add entries to the ChangeLog. Thanks!

Done.  However, something that just struck me, which I didn't encounter
due to the way I was testing, is that I believe all scripts (both linux
and freebsd) will be installed regardless of platform.  What is the best
way to conditionalize the list of SUBDIRS?  I could use the hald backed,
but that will be "linux2" for Linux and "solaris" for Solaris (when the
associated tools/ directories will be linux and sunos).  I suppose I
could do some GNU make fu.  Something like:

SUBDIRS += $(shell uname -s | tr "[:upper:]" "[:lower:]")

Thoughts?

-- 
Joe Marcus Clarke
FreeBSD GNOME Team      ::      gnome at FreeBSD.org
FreeNode / #freebsd-gnome
http://www.FreeBSD.org/gnome
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 187 bytes
Desc: This is a digitally signed message part
Url : http://lists.freedesktop.org/archives/hal/attachments/20060514/dc493262/attachment.pgp