[PATCH] instaweb: if no httpd is specified and lighttpd doesn't exist, fall back on apache2.

[PATCH] instaweb: if no httpd is specified and lighttpd doesn't exist, fall back on apache2.

[nathan spindel]

Signed-off-by: nathan spindel <nathans@gmail.com>
---
 git-instaweb.sh |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/git-instaweb.sh b/git-instaweb.sh
index 6f91c8f..b744133 100755
--- a/git-instaweb.sh
+++ b/git-instaweb.sh
@@ -31,8 +31,16 @@ conf="$GIT_DIR/gitweb/httpd.conf"
 
 # Defaults:
 
-# if installed, it doesn't need further configuration (module_path)
-test -z "$httpd" && httpd='lighttpd -f'
+# use lighttpd if it exists, otherwise use apache2
+if test -z "$httpd"
+then
+	if type "lighttpd" > /dev/null 2>&1;
+	then
+		httpd="lighttpd -f"
+	else
+		httpd="apache2 -f"
+	fi
+fi
 
 # any untaken local port will do...
 test -z "$port" && port=1234
-- 
1.5.5.1.179.g2fe4.dirty

[PATCH] instaweb: make it compatible with Mac OS X 10.5's apache installation.

[nathan spindel]

When in apache2 mode if there isn't an apache2 binary on the system but
there is a httpd command in /usr/sbin/ (like there is on Mac OS X)
use that instead.

When in apache2 mode and there isn't a module_path specified, look for
module paths in /usr/lib/apache2/modules _and_ /usr/libexec/apache2,
in that order.

Added a LockFile directive to the apache2 config because the default
location of /private/var/run is only root-writeable on Mac OS X.

Signed-off-by: nathan spindel <nathans@gmail.com>
---
 git-instaweb.sh |   19 ++++++++++++++++++-
 1 files changed, 18 insertions(+), 1 deletions(-)

diff --git a/git-instaweb.sh b/git-instaweb.sh
index b744133..1c9412c 100755
--- a/git-instaweb.sh
+++ b/git-instaweb.sh
@@ -180,7 +180,23 @@ EOF
 }
 
 apache2_conf () {
-	test -z "$module_path" && module_path=/usr/lib/apache2/modules
+	# if there isn't an apache2 command on the system but there is a httpd
+	# command in /usr/sbin/ use that instead for Mac OS X compatibility.
+	httpd_only="`echo $httpd | cut -f1 -d' '`"
+	type $httpd_only > /dev/null 2>&1;
+	test $? != 0 && test -x /usr/sbin/httpd && httpd=${httpd/apache2/httpd}
+
+	if test -z "$module_path"
+	then
+		for path in /usr/lib/apache2/modules /usr/libexec/apache2; do
+			if test -d "$path"
+			then
+				module_path="$path"
+				break
+			fi
+		done
+	fi
+
 	mkdir -p "$GIT_DIR/gitweb/logs"
 	bind=
 	test x"$local" = xtrue && bind='127.0.0.1:'
@@ -190,6 +206,7 @@ ServerName "git-instaweb"
 ServerRoot "$fqgitdir/gitweb"
 DocumentRoot "$fqgitdir/gitweb"
 PidFile "$fqgitdir/pid"
+LockFile "$fqgitdir/gitweb/logs/accept.lock"
 Listen $bind$port
 EOF
 
-- 
1.5.5.1.179.g2fe4.dirty

[PATCH] Documentation/git-instaweb.txt: Updated defaults to match my last two git-instaweb.sh changes

[nathan spindel]

Signed-off-by: nathan spindel <nathans@gmail.com>
---
 Documentation/git-instaweb.txt |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-instaweb.txt b/Documentation/git-instaweb.txt
index 51f1532..aae5334 100644
--- a/Documentation/git-instaweb.txt
+++ b/Documentation/git-instaweb.txt
@@ -28,11 +28,12 @@ OPTIONS
 	Command-line options may be specified here, and the
 	configuration file will be added at the end of the command-line.
 	Currently lighttpd, apache2 and webrick are supported.
-	(Default: lighttpd)
+	(Default: lighttpd if it exists, otherwise apache2)
 
 -m|--module-path::
 	The module path (only needed if httpd is Apache).
-	(Default: /usr/lib/apache2/modules)
+	(Default: /usr/lib/apache2/modules or /usr/libexec/apache2, 
+	whichever exists)
 
 -p|--port::
 	The port number to bind the httpd to.  (Default: 1234)
-- 
1.5.5.1.179.g2fe4.dirty

Re: [PATCH] instaweb: make it compatible with Mac OS X 10.5's apache installation.

[Junio C Hamano]

nathan spindel <nathans@gmail.com> writes:

> When in apache2 mode if there isn't an apache2 binary on the system but
> there is a httpd command in /usr/sbin/ (like there is on Mac OS X)
> use that instead.

How would you ensure that httpd is actually Apache and not something else?

> When in apache2 mode and there isn't a module_path specified, look for
> module paths in /usr/lib/apache2/modules _and_ /usr/libexec/apache2,
> in that order.

This one sounds Ok.

>  apache2_conf () {
> -	test -z "$module_path" && module_path=/usr/lib/apache2/modules
> +	# if there isn't an apache2 command on the system but there is a httpd
> +	# command in /usr/sbin/ use that instead for Mac OS X compatibility.


> +	httpd_only="`echo $httpd | cut -f1 -d' '`"
> +	type $httpd_only > /dev/null 2>&1;
> +	test $? != 0 && test -x /usr/sbin/httpd && httpd=${httpd/apache2/httpd}

I see the same "type" issue as I mentioned, but I see that we use them in
mergetool and web--browse and we haven't heard breakages so perhaps this
is portable enough ;-)

Please avoid ${parameter/pattern/string} expansion, which is not even in
POSIX.  It is bashism and unportable.

> +	if test -z "$module_path"
> +	then
> +		for path in /usr/lib/apache2/modules /usr/libexec/apache2; do

Hmm.  If you do discovery like this, maybe you would want to do discovery
for "httpd" the same way?  After all, why look for it only in /usr/sbin?

> @@ -190,6 +206,7 @@ ServerName "git-instaweb"
>  ServerRoot "$fqgitdir/gitweb"
>  DocumentRoot "$fqgitdir/gitweb"
>  PidFile "$fqgitdir/pid"
> +LockFile "$fqgitdir/gitweb/logs/accept.lock"
>  Listen $bind$port

We've seen this elsewhere and this part should be Ok.

Re: [PATCH] instaweb: make it compatible with Mac OS X 10.5's apache installation.

[nathan spindel]

On May 10, 2008, at 11:58 PM, Junio C Hamano wrote:

> nathan spindel <nathans@gmail.com> writes:
>
>> When in apache2 mode if there isn't an apache2 binary on the system  
>> but
>> there is a httpd command in /usr/sbin/ (like there is on Mac OS X)
>> use that instead.
>
> How would you ensure that httpd is actually Apache and not something  
> else?

Good point. How about running it with -v and searching for Apache in  
the first line?

>> +	httpd_only="`echo $httpd | cut -f1 -d' '`"
>> +	type $httpd_only > /dev/null 2>&1;
>> +	test $? != 0 && test -x /usr/sbin/httpd && httpd=${httpd/apache2/ 
>> httpd}
>
> I see the same "type" issue as I mentioned, but I see that we use  
> them in
> mergetool and web--browse and we haven't heard breakages so perhaps  
> this
> is portable enough ;-)

Yes, inspection of mergetool and web--browse were what led me to use  
type.  I will see if the other method you suggested can work and use  
that here if so.

> Please avoid ${parameter/pattern/string} expansion, which is not  
> even in
> POSIX.  It is bashism and unportable.

I didn't know that was just a bashism. I'll fix that usage.

>> +	if test -z "$module_path"
>> +	then
>> +		for path in /usr/lib/apache2/modules /usr/libexec/apache2; do
>
> Hmm.  If you do discovery like this, maybe you would want to do  
> discovery
> for "httpd" the same way?  After all, why look for it only in /usr/ 
> sbin?

Yes, it should look in multiple locations.

Re: [PATCH] instaweb: if no httpd is specified and lighttpd doesn't exist, fall back on apache2.

[Junio C Hamano]

nathan spindel <nathans@gmail.com> writes:

> Signed-off-by: nathan spindel <nathans@gmail.com>
> ---
>  git-instaweb.sh |   12 ++++++++++--
>  1 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/git-instaweb.sh b/git-instaweb.sh
> index 6f91c8f..b744133 100755
> --- a/git-instaweb.sh
> +++ b/git-instaweb.sh
> @@ -31,8 +31,16 @@ conf="$GIT_DIR/gitweb/httpd.conf"
>  
>  # Defaults:
>  
> -# if installed, it doesn't need further configuration (module_path)
> -test -z "$httpd" && httpd='lighttpd -f'
> +# use lighttpd if it exists, otherwise use apache2
> +if test -z "$httpd"
> +then
> +	if type "lighttpd" > /dev/null 2>&1;

The exit code from "type" is very loosely defined by POSIX which just says
it exits >0 to signal that "an error occured".  Presumably it means there
is no such command that is executable on the $PATH, and it may be more
portable and reliable than using "which", but this still worries me.

Doesn't "lighttpd" have an option that reports "I am here" and exit 0,
e.g. "--version"?  Then we could instead say:

	if lighttpd --version >/dev/null
        then
        	... use it ...

and that would be much nicer...

Re: [PATCH] instaweb: if no httpd is specified and lighttpd doesn't exist, fall back on apache2.

[nathan spindel]

On May 10, 2008, at 11:44 PM, Junio C Hamano wrote:

> nathan spindel <nathans@gmail.com> writes:
>
>> Signed-off-by: nathan spindel <nathans@gmail.com>
>> ---
>> git-instaweb.sh |   12 ++++++++++--
>> 1 files changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/git-instaweb.sh b/git-instaweb.sh
>> index 6f91c8f..b744133 100755
>> --- a/git-instaweb.sh
>> +++ b/git-instaweb.sh
>> @@ -31,8 +31,16 @@ conf="$GIT_DIR/gitweb/httpd.conf"
>>
>> # Defaults:
>>
>> -# if installed, it doesn't need further configuration (module_path)
>> -test -z "$httpd" && httpd='lighttpd -f'
>> +# use lighttpd if it exists, otherwise use apache2
>> +if test -z "$httpd"
>> +then
>> +	if type "lighttpd" > /dev/null 2>&1;
>
> The exit code from "type" is very loosely defined by POSIX which  
> just says
> it exits >0 to signal that "an error occured".  Presumably it means  
> there
> is no such command that is executable on the $PATH, and it may be more
> portable and reliable than using "which", but this still worries me.
>
> Doesn't "lighttpd" have an option that reports "I am here" and exit 0,
> e.g. "--version"?  Then we could instead say:
>
> 	if lighttpd --version >/dev/null
>        then
>        	... use it ...
>
> and that would be much nicer...

I didn't know that the portability of "type" was questionable.  That's  
a good idea.  Thanks!

Re: [PATCH] instaweb: if no httpd is specified and lighttpd doesn't exist, fall back on apache2.

[nathan spindel]

On May 10, 2008, at 11:44 PM, Junio C Hamano wrote:

> nathan spindel <nathans@gmail.com> writes:
>
>> Signed-off-by: nathan spindel <nathans@gmail.com>
>> ---
>> git-instaweb.sh |   12 ++++++++++--
>> 1 files changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/git-instaweb.sh b/git-instaweb.sh
>> index 6f91c8f..b744133 100755
>> --- a/git-instaweb.sh
>> +++ b/git-instaweb.sh
>> @@ -31,8 +31,16 @@ conf="$GIT_DIR/gitweb/httpd.conf"
>>
>> # Defaults:
>>
>> -# if installed, it doesn't need further configuration (module_path)
>> -test -z "$httpd" && httpd='lighttpd -f'
>> +# use lighttpd if it exists, otherwise use apache2
>> +if test -z "$httpd"
>> +then
>> +	if type "lighttpd" > /dev/null 2>&1;
>
> The exit code from "type" is very loosely defined by POSIX which  
> just says
> it exits >0 to signal that "an error occured".  Presumably it means  
> there
> is no such command that is executable on the $PATH, and it may be more
> portable and reliable than using "which", but this still worries me.
>
> Doesn't "lighttpd" have an option that reports "I am here" and exit 0,
> e.g. "--version"?  Then we could instead say:
>
> 	if lighttpd --version >/dev/null
>        then
>        	... use it ...
>
> and that would be much nicer...

Actually, this approach has the same problem that's worked around  
here: <http://repo.or.cz/w/git.git?a=blob;f=git- 
instaweb.sh;hb=HEAD#l46>. (You might have lighttpd installed but not  
be on your $PATH, which the script should handle for you).

It seems the best approach would be to function-ize the aforementioned  
httpd-path-checking logic [that currently uses "which"] and use it for  
(a) detecting if lighttpd is installed and (b) looking for apache2  
binaries named httpd.

I think for now these particular patches will continue to use "type"  
as you somewhat suggested earlier today.  :)

Re: [PATCH] instaweb: if no httpd is specified and lighttpd doesn't exist, fall back on apache2.

[David Symonds]

On Sun, May 11, 2008 at 3:26 PM, nathan spindel <nathans@gmail.com> wrote:

> -# if installed, it doesn't need further configuration (module_path)
> -test -z "$httpd" && httpd='lighttpd -f'
> +# use lighttpd if it exists, otherwise use apache2
> +if test -z "$httpd"
> +then
> +       if type "lighttpd" > /dev/null 2>&1;
> +       then
> +               httpd="lighttpd -f"
> +       else
> +               httpd="apache2 -f"
> +       fi
> +fi

I personally would prefer to use webrick over apache2, since it's much
more lightweight. That's just my two cents.


Dave.

Re: [PATCH] instaweb: if no httpd is specified and lighttpd doesn't exist, fall back on apache2.

[nathan spindel]

On May 11, 2008, at 4:58 AM, David Symonds wrote:

> On Sun, May 11, 2008 at 3:26 PM, nathan spindel <nathans@gmail.com>  
> wrote:
>
>> -# if installed, it doesn't need further configuration (module_path)
>> -test -z "$httpd" && httpd='lighttpd -f'
>> +# use lighttpd if it exists, otherwise use apache2
>> +if test -z "$httpd"
>> +then
>> +       if type "lighttpd" > /dev/null 2>&1;
>> +       then
>> +               httpd="lighttpd -f"
>> +       else
>> +               httpd="apache2 -f"
>> +       fi
>> +fi
>
> I personally would prefer to use webrick over apache2, since it's much
> more lightweight. That's just my two cents.

I am apathetic about which web servers are preferred, I would just  
like to see git-instaweb work well on Mac OS X.  Kevin Ballard's  
suggestion was to fall back on apache2.

We could also modify the default to use [lighttpd, webrick, apache2]  
in that order (whichever is available). Would the community prefer  
that approach?

Re: [PATCH] instaweb: if no httpd is specified and lighttpd doesn't exist, fall back on apache2.

[Junio C Hamano]

nathan spindel <nathans@gmail.com> writes:

> I am apathetic about which web servers are preferred, I would just
> like to see git-instaweb work well on Mac OS X.  Kevin Ballard's
> suggestion was to fall back on apache2.
>
> We could also modify the default to use [lighttpd, webrick, apache2]
> in that order (whichever is available). Would the community prefer
> that approach?

I think apache is the most well known and widely used among the three, and
find Kevin's suggestion sensible.