Re: [PATCH v2 1/3] t/lib-httpd: dynamically detect httpd and modules path

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 9, 2023, 00:30 UTC
Message-ID: <xmqqsf5f4vl8.fsf@gitster.g>
In-Reply-To: <20231108165426.GB1028115@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 28 quoted lines

> On Wed, Nov 08, 2023 at 03:57:19PM +0100, Patrick Steinhardt wrote:
>
>> While it is possible to specify these paths via `LIB_HTTPD_PATH` and
>> `LIB_HTTPD_MODULE_PATH`, it is not a nice experience for the developer
>> to figure out how to set those up. And in fact we can do better by
>> dynamically detecting both httpd and its module path at runtime:
>> 
>>     - The httpd binary can be located via PATH.
>> 
>>     - The module directory can (in many cases) be derived via the
>>       `HTTPD_ROOT` compile-time variable.
>> 
>> Amend the code to do so. The new runtime-detected paths will only be
>> used in case none of the hardcoded paths are usable.
>
> If these improve detection on your platform, I think that is a good
> thing and they are worth doing. But as a generic mechanism, I have two
> comments:
>
>> -for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2'
>> +for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2' "$(command -v httpd)"
>>  do
>>  	if test -x "$DEFAULT_HTTPD_PATH"
>>  	then
>
> The binary goes under the name "httpd", but also "apache2". But the PATH
> search only looks for "httpd". Should we check "command -v apache2" as
> well?

Interesting. If $() were on the right hand side of an assignment, the command that fails may also something to watch for, but I think $? from $(command -v no-such-httpd) would be discarded, so it would be fine in this case. A trivia I found out today: dash exits with 127 and bash exits with 1 when doing

    $shell -c 'command -v no-such-httpd'

> This also means we may run "test -x" on an empty string, but that is
> probably OK in practice (we could guard it with "test -n", though).

Good thinking. We are not worried about exotic or misbehaving shells in this thread, so hopefully shell built-in 'test' would say "no" when asked if "" is executable (in other words, "" is not mistaken as "."). But an explicit "test -n" would save the readers by removing the worry.

> ... So without a system config file to act as a template for our custom
> config, I think we are stuck with guessing where the installer might
> have put them.

Right.  Thanks for a careful analysis.