| Message ID | e4c75c492dd89fd7464db2b3028b2bb9e6addbf8.1699513524.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | t: improve compatibility with NixOS | expand |
On Thu, Nov 09, 2023 at 08:09:52AM +0100, Patrick Steinhardt wrote: > -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)" \ > + "$(command -v apache2)" > do > - if test -x "$DEFAULT_HTTPD_PATH" > + if test -n "$DEFAULT_HTTPD_PATH" -a -x "$DEFAULT_HTTPD_PATH" Sorry to be a pedant, but I'm not sure if we might have portability problems with "-a". It's an XSI extension, and POSIX labels it as obsolescent because it can create parsing ambiguities. We do have a few instances, but only in corners of the test suite that probably don't get as much exposure (t/perf and valgrind/valgrind.sh). So maybe not worth worrying about, but it's easy to write it as: if test -n "$DEFAULT_HTTPD_PATH" && test -x "$DEFAULT_HTTPD_PATH" -Peff
On Thu, Nov 09, 2023 at 02:32:50AM -0500, Jeff King wrote: > On Thu, Nov 09, 2023 at 08:09:52AM +0100, Patrick Steinhardt wrote: > > > -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)" \ > > + "$(command -v apache2)" > > do > > - if test -x "$DEFAULT_HTTPD_PATH" > > + if test -n "$DEFAULT_HTTPD_PATH" -a -x "$DEFAULT_HTTPD_PATH" > > Sorry to be a pedant, but I'm not sure if we might have portability > problems with "-a". It's an XSI extension, and POSIX labels it as > obsolescent because it can create parsing ambiguities. > > We do have a few instances, but only in corners of the test suite that > probably don't get as much exposure (t/perf and valgrind/valgrind.sh). > So maybe not worth worrying about, but it's easy to write it as: Yeah, I was grepping for it in our codebase and saw other occurrences, so I assumed it was fair game. If we're going to convert it to the below, how about I send another patch on top that also converts the preexisting instances so that the next one grepping for it isn't going to repeat the same mistake? Patrick > if test -n "$DEFAULT_HTTPD_PATH" && test -x "$DEFAULT_HTTPD_PATH" > > -Peff
Patrick Steinhardt <ps@pks.im> writes: > Yeah, I was grepping for it in our codebase and saw other occurrences, > so I assumed it was fair game. If we're going to convert it to the > below, how about I send another patch on top that also converts the > preexisting instances so that the next one grepping for it isn't going > to repeat the same mistake? Yup, an independent clean-up would be fine. Now we need to find a way to give better visibility to CodingGuidelines, which already says this: - We do not write our "test" command with "-a" and "-o" and use "&&" or "||" to concatenate multiple "test" commands instead, because the use of "-a/-o" is often error-prone. E.g. test -n "$x" -a "$a" = "$b" is buggy and breaks when $x is "=", but test -n "$x" && test "$a" = "$b" does not have such a problem.
On Thu, Nov 09, 2023 at 08:36:53AM +0100, Patrick Steinhardt wrote: > > Sorry to be a pedant, but I'm not sure if we might have portability > > problems with "-a". It's an XSI extension, and POSIX labels it as > > obsolescent because it can create parsing ambiguities. > > > > We do have a few instances, but only in corners of the test suite that > > probably don't get as much exposure (t/perf and valgrind/valgrind.sh). > > So maybe not worth worrying about, but it's easy to write it as: > > Yeah, I was grepping for it in our codebase and saw other occurrences, > so I assumed it was fair game. If we're going to convert it to the > below, how about I send another patch on top that also converts the > preexisting instances so that the next one grepping for it isn't going > to repeat the same mistake? I would be very happy with that. :) -Peff
On Thu, Nov 09, 2023 at 04:46:05PM +0900, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > Yeah, I was grepping for it in our codebase and saw other occurrences, > > so I assumed it was fair game. If we're going to convert it to the > > below, how about I send another patch on top that also converts the > > preexisting instances so that the next one grepping for it isn't going > > to repeat the same mistake? > > Yup, an independent clean-up would be fine. Now we need to find a > way to give better visibility to CodingGuidelines, which already > says this: Okay, I'll send one in. Do you want me to send a v4 of this patch series or will you squash in below changes into patch 1/3? > - We do not write our "test" command with "-a" and "-o" and use "&&" > or "||" to concatenate multiple "test" commands instead, because > the use of "-a/-o" is often error-prone. E.g. > > test -n "$x" -a "$a" = "$b" > > is buggy and breaks when $x is "=", but > > test -n "$x" && test "$a" = "$b" > > does not have such a problem. I did indeed spot this part of our coding style now. I didn't bother to look farther when I found other examples where we used `-a` and `-o`, but that issue will be gone once we've dropped all of these usages. Patrick -- >8 -- diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh index 6ab8f273a3..0a74922d7f 100644 --- a/t/lib-httpd.sh +++ b/t/lib-httpd.sh @@ -60,7 +60,7 @@ for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' \ "$(command -v httpd)" \ "$(command -v apache2)" do - if test -n "$DEFAULT_HTTPD_PATH" -a -x "$DEFAULT_HTTPD_PATH" + if test -n "$DEFAULT_HTTPD_PATH" && test -x "$DEFAULT_HTTPD_PATH" then break fi @@ -78,7 +78,7 @@ for DEFAULT_HTTPD_MODULE_PATH in '/usr/libexec/apache2' \ '/usr/libexec/httpd' \ "${DETECTED_HTTPD_ROOT:+${DETECTED_HTTPD_ROOT}/modules}" do - if test -n "$DEFAULT_HTTPD_MODULE_PATH" -a -d "$DEFAULT_HTTPD_MODULE_PATH" + if test -n "$DEFAULT_HTTPD_MODULE_PATH" && test -d "$DEFAULT_HTTPD_MODULE_PATH" then break fi
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh index 5fe3c8ab69d..6ab8f273a3a 100644 --- a/t/lib-httpd.sh +++ b/t/lib-httpd.sh @@ -55,21 +55,30 @@ fi HTTPD_PARA="" -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)" \ + "$(command -v apache2)" do - if test -x "$DEFAULT_HTTPD_PATH" + if test -n "$DEFAULT_HTTPD_PATH" -a -x "$DEFAULT_HTTPD_PATH" then break fi done +if test -x "$DEFAULT_HTTPD_PATH" +then + DETECTED_HTTPD_ROOT="$("$DEFAULT_HTTPD_PATH" -V | sed -n 's/^ -D HTTPD_ROOT="\(.*\)"$/\1/p')" +fi + for DEFAULT_HTTPD_MODULE_PATH in '/usr/libexec/apache2' \ '/usr/lib/apache2/modules' \ '/usr/lib64/httpd/modules' \ '/usr/lib/httpd/modules' \ - '/usr/libexec/httpd' + '/usr/libexec/httpd' \ + "${DETECTED_HTTPD_ROOT:+${DETECTED_HTTPD_ROOT}/modules}" do - if test -d "$DEFAULT_HTTPD_MODULE_PATH" + if test -n "$DEFAULT_HTTPD_MODULE_PATH" -a -d "$DEFAULT_HTTPD_MODULE_PATH" then break fi
In order to set up the Apache httpd server, we need to locate both the httpd binary and its default module path. This is done with a hardcoded list of locations that we scan. While this works okayish with distros that more-or-less follow the Filesystem Hierarchy Standard, it falls apart on others like NixOS that don't. 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. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- t/lib-httpd.sh | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)