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

From: Junio C Hamano <gitster@pobox.com>
Date: May 11, 2008, 06:58 UTC
Message-ID: <7vhcd572e3.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <1210483565-12415-2-git-send-email-nathans@gmail.com>

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?

Show 6 quoted lines

> @@ -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.