threads / patch / 60489

patch, 3 partst: improve compatibility with NixOS

Subject: [PATCH 0/3] t: improve compatibility with NixOS

## tl;dr

41 messages between Nov 8, 2023 and Nov 13, 2023. Diffs are folded; open one to read it.

replies: 40people: 3as markdown or json

Patrick Steinhardt· Nov 8, 2023, 07:29 UTC · lore

Hi,

this patch series improves compatibility of our test suite with NixOS.

NixOS is somewhat special compared to more conventional Linux distros because it doesn't follow the Filesystem Hierarchy Specification for most of the part. Instead, packages are installed into the Nix store in `/nix` with hashed paths that change frequently when upgrading the system to a newer generation. Consequentially, paths cannot be hardcoded and must instead be computed at runtime.

We have two such issues in our test harness right now:

    - t/lib-httpd searches Apache httpd and its modules directory in a
      list of hardcoded paths.

    - t9164 doesn't propagate PATH to a script and thus cannot find the
      basename(1) utility.

Both of these issues are fixed in this patch series. In addition, this patch series fixes an upcoming issue in httpd's passwd files caused by the deprecation of the crypt(3) function.

Patrick

Patrick Steinhardt (3):
  t/lib-httpd: dynamically detect httpd and modules path
  t/lib-httpd: stop using legacy crypt(3) for authentication
  t9164: fix inability to find basename(1) in hooks

 t/lib-httpd.sh                        | 51 ++++++++++++++++++---------
 t/lib-httpd/passwd                    |  2 +-
 t/lib-httpd/proxy-passwd              |  2 +-
 t/t9164-git-svn-dcommit-concurrent.sh | 12 +++++--
 4 files changed, 45 insertions(+), 22 deletions(-)

-- 
2.42.0

Patrick Steinhardt· Nov 8, 2023, 07:29 UTC · re: Patrick Steinhardt · lore

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

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.

Refactor the code to do so. If runtime detection of the paths fails we continue to fall back to the hardcoded list of paths.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/lib-httpd.sh | 51 +++++++++++++++++++++++++++++++++-----------------
 1 file changed, 34 insertions(+), 17 deletions(-)

Show changes to t/lib-httpd.sh +34 −17

diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 2fb1b2ae561..06038f72607 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -55,25 +55,42 @@ fi
 
 HTTPD_PARA=""
 
-for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2'
-do
-	if test -x "$DEFAULT_HTTPD_PATH"
-	then
-		break
-	fi
-done
-
-for DEFAULT_HTTPD_MODULE_PATH in '/usr/libexec/apache2' \
-				 '/usr/lib/apache2/modules' \
-				 '/usr/lib64/httpd/modules' \
-				 '/usr/lib/httpd/modules' \
-				 '/usr/libexec/httpd'
-do
-	if test -d "$DEFAULT_HTTPD_MODULE_PATH"
+DEFAULT_HTTPD_PATH="$(command -v httpd)"
+if test -z "$DEFAULT_HTTPD_PATH" -o ! -x "$DEFAULT_HTTPD_PATH"
+then
+	for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2'
+	do
+		if test -x "$DEFAULT_HTTPD_PATH"
+		then
+			break
+		fi
+	done
+fi
+
+if test -x "$DEFAULT_HTTPD_PATH"
+then
+	HTTPD_ROOT="$("$DEFAULT_HTTPD_PATH" -V | sed -n 's/^ -D HTTPD_ROOT="\(.*\)"$/\1/p')"
+	if test -n "$HTTPD_ROOT" -a -d "$HTTPD_ROOT/modules"
 	then
-		break
+		DEFAULT_HTTPD_MODULE_PATH="$HTTPD_ROOT/modules"
 	fi
-done
+	unset HTTPD_ROOT
+fi
+
+if test -z "$DEFAULT_HTTPD_MODULE_PATH" -o ! -d "$DEFAULT_HTTPD_MODULE_PATH"
+then
+	for DEFAULT_HTTPD_MODULE_PATH in '/usr/libexec/apache2' \
+					 '/usr/lib/apache2/modules' \
+					 '/usr/lib64/httpd/modules' \
+					 '/usr/lib/httpd/modules' \
+					 '/usr/libexec/httpd'
+	do
+		if test -d "$DEFAULT_HTTPD_MODULE_PATH"
+		then
+			break
+		fi
+	done
+fi
 
 case $(uname) in
 	Darwin)

-- 
2.42.0

Junio C Hamano· Nov 8, 2023, 07:59 UTC · re: Patrick Steinhardt · lore

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

Patrick Steinhardt <ps@pks.im> writes:

Show 18 quoted lines

> 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.
>
> Refactor the code to do so. If runtime detection of the paths fails we
> continue to fall back to the hardcoded list of paths.

Hmph.

I do not think we would want to punish the distros that follow the FHS that was created explicitly to help developers by standardizing locations of various things, with an approach this patch takes that throws everthing with bathwater and rely on $PATH first.

Would it be sufficient to please NixOS if we simply append $(command -v apache) or whatever after the well known candidate locations?

I know "command -v" is in POSIX, and on both bash and dash (the two shells most distros use), it works as this patch expects, but its portability is also a bit worrysome, especially because the whole point of this patch is to support a platform that is, eh, on the fringe.

So, I dunno.

Patrick Steinhardt· Nov 8, 2023, 10:42 UTC · re: Junio C Hamano · lore

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

On Wed, Nov 08, 2023 at 04:59:46PM +0900, Junio C Hamano wrote:

Show 30 quoted lines

> Patrick Steinhardt <ps@pks.im> writes:
> 
> > 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.
> >
> > Refactor the code to do so. If runtime detection of the paths fails we
> > continue to fall back to the hardcoded list of paths.
> 
> Hmph.
> 
> I do not think we would want to punish the distros that follow the
> FHS that was created explicitly to help developers by standardizing
> locations of various things, with an approach this patch takes that
> throws everthing with bathwater and rely on $PATH first.
> 
> Would it be sufficient to please NixOS if we simply append $(command
> -v apache) or whatever after the well known candidate locations?

I was a bit torn myself when writing this. I can also see a potential future where we would drop the hardcoded list of locations altogether in favor of always using PATH. After all we already rely on PATH to resolve other tools as well, so why should httpd be special there?

But in the end I opted to use the more conservative approach of using both PATH and the static list as I didn't want to break other distros. I don't mind to make this even more conservative and resolve via PATH as a last resort, only.

Patrick

Show 7 quoted lines

> I know "command -v" is in POSIX, and on both bash and dash (the two
> shells most distros use), it works as this patch expects, but its
> portability is also a bit worrysome, especially because the whole
> point of this patch is to support a platform that is, eh, on the
> fringe.
> 
> So, I dunno.

Jeff King· Nov 8, 2023, 16:44 UTC · re: Patrick Steinhardt · lore

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

On Wed, Nov 08, 2023 at 11:42:11AM +0100, Patrick Steinhardt wrote:

> I was a bit torn myself when writing this. I can also see a potential
> future where we would drop the hardcoded list of locations altogether in
> favor of always using PATH. After all we already rely on PATH to resolve
> other tools as well, so why should httpd be special there?

httpd/apache2 is often in sbin, which is not as commonly found in the PATH of regular users. E.g., this is the case on Debian (it is /sbin/apache2, and a newly created user will not have that in their PATH).

-Peff

Patrick Steinhardt· Nov 8, 2023, 07:30 UTC · re: Patrick Steinhardt · lore

[PATCH 2/3] t/lib-httpd: stop using legacy crypt(3) for authentication

When setting up httpd for our tests, we also install a passwd and proxy-passwd file that contain the test user's credentials. These credentials currently use crypt(3) as the password encryption schema.

This schema can be considered deprecated nowadays as it is not safe anymore. Quoting Apache httpd's documentation [1]:

> Unix only. Uses the traditional Unix crypt(3) function with a
> randomly-generated 32-bit salt (only 12 bits used) and the first 8
> characters of the password. Insecure.

This is starting to cause issues in modern Linux distributions. glibc has deprecated its libcrypt library that used to provide crypt(3) in favor of the libxcrypt library. This newer replacement provides a compile time switch to disable insecure password encryption schemata, which causes crypt(3) to always return `EINVAL`. The end result is that httpd tests that exercise authentication will fail on distros that use libxcrypt without these insecure encryption schematas.

Regenerate the passwd files to instead use the default password encryption schema, which is md5. While it feels kind of funny that an MD5-based encryption schema should be more secure than anything else, it is the current default and supported by all platforms. Furthermore, it really doesn't matter all that much given that these files are only used for testing purposes anyway.

[1]: https://httpd.apache.org/docs/2.4/misc/password_encryptions.html

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/lib-httpd/passwd       | 2 +-
 t/lib-httpd/proxy-passwd | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Show changes to 2 files +2 −2

t/lib-httpd/passwd, t/lib-httpd/proxy-passwd

diff --git a/t/lib-httpd/passwd b/t/lib-httpd/passwd
index 99a34d64874..d9c122f3482 100644
--- a/t/lib-httpd/passwd
+++ b/t/lib-httpd/passwd
@@ -1 +1 @@
-user@host:xb4E8pqD81KQs
+user@host:$apr1$LGPmCZWj$9vxEwj5Z5GzQLBMxp3mCx1
diff --git a/t/lib-httpd/proxy-passwd b/t/lib-httpd/proxy-passwd
index 77c25138e07..2ad7705d9a3 100644
--- a/t/lib-httpd/proxy-passwd
+++ b/t/lib-httpd/proxy-passwd
@@ -1 +1 @@
-proxuser:2x7tAukjAED5M
+proxuser:$apr1$RxS6MLkD$DYsqQdflheq4GPNxzJpx5.

-- 
2.42.0

Junio C Hamano· Nov 8, 2023, 08:01 UTC · re: Patrick Steinhardt · lore

Re: [PATCH 2/3] t/lib-httpd: stop using legacy crypt(3) for authentication

Patrick Steinhardt <ps@pks.im> writes:

Show 25 quoted lines

> When setting up httpd for our tests, we also install a passwd and
> proxy-passwd file that contain the test user's credentials. These
> credentials currently use crypt(3) as the password encryption schema.
>
> This schema can be considered deprecated nowadays as it is not safe
> anymore. Quoting Apache httpd's documentation [1]:
>
>> Unix only. Uses the traditional Unix crypt(3) function with a
>> randomly-generated 32-bit salt (only 12 bits used) and the first 8
>> characters of the password. Insecure.
>
> This is starting to cause issues in modern Linux distributions. glibc
> has deprecated its libcrypt library that used to provide crypt(3) in
> favor of the libxcrypt library. This newer replacement provides a
> compile time switch to disable insecure password encryption schemata,
> which causes crypt(3) to always return `EINVAL`. The end result is that
> httpd tests that exercise authentication will fail on distros that use
> libxcrypt without these insecure encryption schematas.
>
> Regenerate the passwd files to instead use the default password
> encryption schema, which is md5. While it feels kind of funny that an
> MD5-based encryption schema should be more secure than anything else, it
> is the current default and supported by all platforms. Furthermore, it
> really doesn't matter all that much given that these files are only used
> for testing purposes anyway.

This step makes quite a lot of sense, as we are changing this not at all for security but for portability ;-)

Show 23 quoted lines

>
> [1]: https://httpd.apache.org/docs/2.4/misc/password_encryptions.html
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  t/lib-httpd/passwd       | 2 +-
>  t/lib-httpd/proxy-passwd | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/lib-httpd/passwd b/t/lib-httpd/passwd
> index 99a34d64874..d9c122f3482 100644
> --- a/t/lib-httpd/passwd
> +++ b/t/lib-httpd/passwd
> @@ -1 +1 @@
> -user@host:xb4E8pqD81KQs
> +user@host:$apr1$LGPmCZWj$9vxEwj5Z5GzQLBMxp3mCx1
> diff --git a/t/lib-httpd/proxy-passwd b/t/lib-httpd/proxy-passwd
> index 77c25138e07..2ad7705d9a3 100644
> --- a/t/lib-httpd/proxy-passwd
> +++ b/t/lib-httpd/proxy-passwd
> @@ -1 +1 @@
> -proxuser:2x7tAukjAED5M
> +proxuser:$apr1$RxS6MLkD$DYsqQdflheq4GPNxzJpx5.

Patrick Steinhardt· Nov 8, 2023, 07:30 UTC · re: Patrick Steinhardt · lore

[PATCH 3/3] t9164: fix inability to find basename(1) in hooks

The post-commit hook in t9164 is executed with a default environment. To work around this issue we already write the current `PATH` value into the hook script. But this is done at a point where we already tried to execute non-built-in commands, namely basename(1). While this seems to work alright on most platforms, it fails on NixOS.

Set the `PATH` variable earlier to fix this issue. Note that we do this via two separate heredocs. This is done because in the first one we do want to expand variables, whereas in the second one we don't.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t9164-git-svn-dcommit-concurrent.sh | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Show changes to t/t9164-git-svn-dcommit-concurrent.sh +9 −3

diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh
index c8e6c0733f4..3b64022dc57 100755
--- a/t/t9164-git-svn-dcommit-concurrent.sh
+++ b/t/t9164-git-svn-dcommit-concurrent.sh
@@ -47,9 +47,15 @@ setup_hook()
 	echo "cnt=$skip_revs" > "$hook_type-counter"
 	rm -f "$rawsvnrepo/hooks/"*-commit # drop previous hooks
 	hook="$rawsvnrepo/hooks/$hook_type"
-	cat > "$hook" <<- 'EOF1'
+	cat >"$hook" <<-EOF
 		#!/bin/sh
 		set -e
+
+		PATH=\"$PATH\"
+		export PATH
+	EOF
+
+	cat >>"$hook" <<-'EOF'
 		cd "$1/.."  # "$1" is repository location
 		exec >> svn-hook.log 2>&1
 		hook="$(basename "$0")"
@@ -59,11 +65,11 @@ setup_hook()
 		cnt="$(($cnt - 1))"
 		echo "cnt=$cnt" > ./$hook-counter
 		[ "$cnt" = "0" ] || exit 0
-EOF1
+	EOF
+
 	if [ "$hook_type" = "pre-commit" ]; then
 		echo "echo 'commit disallowed' >&2; exit 1" >>"$hook"
 	else
-		echo "PATH=\"$PATH\"; export PATH" >>"$hook"
 		echo "svnconf=\"$svnconf\"" >>"$hook"
 		cat >>"$hook" <<- 'EOF2'
 			cd work-auto-commits.svn

-- 
2.42.0

Patrick Steinhardt· Nov 8, 2023, 14:57 UTC · re: Patrick Steinhardt · lore

[PATCH v2 0/3] t: improve compatibility with NixOS

Hi,

this is the second version of my patch series to improve compatibility of our tests with NixOS.

There's only a single change compared to v1. As suggested by Junio, we now use the executable discovered via PATH and the modules detected via `HTTPD_ROOT` as fallback values if none of our hardcoded paths were detected. This should further ensure that the refactoring doesn't cause any issues on platforms that follow the FHS more closely than NixOS does. It also has the benefit that the change is now a lot more straight forward.

Patrick

Patrick Steinhardt (3):
  t/lib-httpd: dynamically detect httpd and modules path
  t/lib-httpd: stop using legacy crypt(3) for authentication
  t9164: fix inability to find basename(1) in hooks

 t/lib-httpd.sh                        | 10 ++++++++--
 t/lib-httpd/passwd                    |  2 +-
 t/lib-httpd/proxy-passwd              |  2 +-
 t/t9164-git-svn-dcommit-concurrent.sh | 12 +++++++++---
 4 files changed, 19 insertions(+), 7 deletions(-)

Range-diff against v1:
1:  ac059db8fed ! 1:  cee8fbebf84 t/lib-httpd: dynamically detect httpd and modules path
    @@ Commit message
             - The module directory can (in many cases) be derived via the
               `HTTPD_ROOT` compile-time variable.
     
    -    Refactor the code to do so. If runtime detection of the paths fails we
    -    continue to fall back to the hardcoded list of paths.
    +    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: fi
      HTTPD_PARA=""
      
     -for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2'
    --do
    --	if test -x "$DEFAULT_HTTPD_PATH"
    --	then
    --		break
    --	fi
    --done
    --
    --for DEFAULT_HTTPD_MODULE_PATH in '/usr/libexec/apache2' \
    --				 '/usr/lib/apache2/modules' \
    --				 '/usr/lib64/httpd/modules' \
    --				 '/usr/lib/httpd/modules' \
    --				 '/usr/libexec/httpd'
    --do
    --	if test -d "$DEFAULT_HTTPD_MODULE_PATH"
    -+DEFAULT_HTTPD_PATH="$(command -v httpd)"
    -+if test -z "$DEFAULT_HTTPD_PATH" -o ! -x "$DEFAULT_HTTPD_PATH"
    -+then
    -+	for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2'
    -+	do
    -+		if test -x "$DEFAULT_HTTPD_PATH"
    -+		then
    -+			break
    -+		fi
    -+	done
    -+fi
    -+
    -+if test -x "$DEFAULT_HTTPD_PATH"
    -+then
    -+	HTTPD_ROOT="$("$DEFAULT_HTTPD_PATH" -V | sed -n 's/^ -D HTTPD_ROOT="\(.*\)"$/\1/p')"
    -+	if test -n "$HTTPD_ROOT" -a -d "$HTTPD_ROOT/modules"
    ++for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2' "$(command -v httpd)"
    + do
    + 	if test -x "$DEFAULT_HTTPD_PATH"
      	then
    --		break
    -+		DEFAULT_HTTPD_MODULE_PATH="$HTTPD_ROOT/modules"
    +@@ t/lib-httpd.sh: do
      	fi
    --done
    -+	unset HTTPD_ROOT
    -+fi
    -+
    -+if test -z "$DEFAULT_HTTPD_MODULE_PATH" -o ! -d "$DEFAULT_HTTPD_MODULE_PATH"
    + done
    + 
    ++if test -x "$DEFAULT_HTTPD_PATH"
     +then
    -+	for DEFAULT_HTTPD_MODULE_PATH in '/usr/libexec/apache2' \
    -+					 '/usr/lib/apache2/modules' \
    -+					 '/usr/lib64/httpd/modules' \
    -+					 '/usr/lib/httpd/modules' \
    -+					 '/usr/libexec/httpd'
    -+	do
    -+		if test -d "$DEFAULT_HTTPD_MODULE_PATH"
    -+		then
    -+			break
    -+		fi
    -+	done
    ++	DETECTED_HTTPD_ROOT="$("$DEFAULT_HTTPD_PATH" -V | sed -n 's/^ -D HTTPD_ROOT="\(.*\)"$/\1/p')"
     +fi
    - 
    - case $(uname) in
    - 	Darwin)
    ++
    + 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"
    + 	then
2:  23835763002 = 2:  f4c6c754f1e t/lib-httpd: stop using legacy crypt(3) for authentication
3:  b50e625f967 = 3:  361f1bd9c88 t9164: fix inability to find basename(1) in hooks

base-commit: 98009afd24e2304bf923a64750340423473809ff

-- 
2.42.0

Patrick Steinhardt· Nov 8, 2023, 14:57 UTC · re: Patrick Steinhardt · lore

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

    - 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 | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Show changes to t/lib-httpd.sh +8 −2

diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 5fe3c8ab69d..26c1632ea9a 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -55,7 +55,7 @@ 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)"
 do
 	if test -x "$DEFAULT_HTTPD_PATH"
 	then
@@ -63,11 +63,17 @@ do
 	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"
 	then

-- 
2.42.0

Jeff King· Nov 8, 2023, 16:54 UTC · re: Patrick Steinhardt · lore

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

On Wed, Nov 08, 2023 at 03:57:19PM +0100, Patrick Steinhardt wrote:

Show 12 quoted lines

> 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:

Show 5 quoted lines

> -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?

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

> +if test -x "$DEFAULT_HTTPD_PATH"
> +then
> +	DETECTED_HTTPD_ROOT="$("$DEFAULT_HTTPD_PATH" -V | sed -n 's/^ -D HTTPD_ROOT="\(.*\)"$/\1/p')"
> +fi

I was really pleased to see this and hoped it could replace the kitchen-sink list of paths in the hunk below. But sadly I think it depends on having a configured apache setup. On my Debian system, for example, I have the "apache2-bin" package installed but not "apache" (because only the latter actually sets up a system apache daemon, which I don't want). And thus there is no config:

  $ /usr/sbin/apache2 -D HTTPD_ROOT
  apache2: Could not open configuration file /etc/apache2/apache2.conf: No such file or directory

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.

-Peff

Junio C Hamano· Nov 9, 2023, 00:30 UTC · re: Jeff King · lore

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

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.

Patrick Steinhardt· Nov 9, 2023, 06:30 UTC · re: Jeff King · lore

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

On Wed, Nov 08, 2023 at 11:54:26AM -0500, Jeff King wrote:

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?

Doesn't hurt to do it.

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

Fair, will do.

Show 18 quoted lines

> > +if test -x "$DEFAULT_HTTPD_PATH"
> > +then
> > +	DETECTED_HTTPD_ROOT="$("$DEFAULT_HTTPD_PATH" -V | sed -n 's/^ -D HTTPD_ROOT="\(.*\)"$/\1/p')"
> > +fi
> 
> I was really pleased to see this and hoped it could replace the
> kitchen-sink list of paths in the hunk below. But sadly I think it
> depends on having a configured apache setup. On my Debian system, for
> example, I have the "apache2-bin" package installed but not "apache"
> (because only the latter actually sets up a system apache daemon, which
> I don't want). And thus there is no config:
> 
>   $ /usr/sbin/apache2 -D HTTPD_ROOT
>   apache2: Could not open configuration file /etc/apache2/apache2.conf: No such file or directory
> 
> 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.

Indeed. Also, paths like `/usr/libexec/apache2` wouldn't be possible to be discovered via `${HTTPD_ROOT}/modules` anyway. It's kind of a shame that there is no way (or at least no way I know of) to portably discover the default modules directory used by httpd.

Patrick

Patrick Steinhardt· Nov 8, 2023, 14:57 UTC · re: Patrick Steinhardt · lore

[PATCH v2 2/3] t/lib-httpd: stop using legacy crypt(3) for authentication

This schema can be considered deprecated nowadays as it is not safe anymore. Quoting Apache httpd's documentation [1]:

> Unix only. Uses the traditional Unix crypt(3) function with a
> randomly-generated 32-bit salt (only 12 bits used) and the first 8
> characters of the password. Insecure.

[1]: https://httpd.apache.org/docs/2.4/misc/password_encryptions.html

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/lib-httpd/passwd       | 2 +-
 t/lib-httpd/proxy-passwd | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Show changes to 2 files +2 −2

t/lib-httpd/passwd, t/lib-httpd/proxy-passwd

diff --git a/t/lib-httpd/passwd b/t/lib-httpd/passwd
index 99a34d64874..d9c122f3482 100644
--- a/t/lib-httpd/passwd
+++ b/t/lib-httpd/passwd
@@ -1 +1 @@
-user@host:xb4E8pqD81KQs
+user@host:$apr1$LGPmCZWj$9vxEwj5Z5GzQLBMxp3mCx1
diff --git a/t/lib-httpd/proxy-passwd b/t/lib-httpd/proxy-passwd
index 77c25138e07..2ad7705d9a3 100644
--- a/t/lib-httpd/proxy-passwd
+++ b/t/lib-httpd/proxy-passwd
@@ -1 +1 @@
-proxuser:2x7tAukjAED5M
+proxuser:$apr1$RxS6MLkD$DYsqQdflheq4GPNxzJpx5.

-- 
2.42.0

Jeff King· Nov 8, 2023, 17:02 UTC · re: Patrick Steinhardt · lore

Re: [PATCH v2 2/3] t/lib-httpd: stop using legacy crypt(3) for authentication

On Wed, Nov 08, 2023 at 03:57:23PM +0100, Patrick Steinhardt wrote:

Show 6 quoted lines

> Regenerate the passwd files to instead use the default password
> encryption schema, which is md5. While it feels kind of funny that an
> MD5-based encryption schema should be more secure than anything else, it
> is the current default and supported by all platforms. Furthermore, it
> really doesn't matter all that much given that these files are only used
> for testing purposes anyway.

Thanks for doing this. I died inside a little while adding the proxy-passwd one recently in 29ae2c9e74 (add basic http proxy tests, 2023-02-16). There I mused about moving to bcrypt in a separate patch, which I think is probably the least-bad option from a security perspective. But I agree that md5 is more likely to be available everywhere, and we certainly don't care about security here.

-Peff

Patrick Steinhardt· Nov 8, 2023, 14:57 UTC · re: Patrick Steinhardt · lore

[PATCH v2 3/3] t9164: fix inability to find basename(1) in hooks

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t9164-git-svn-dcommit-concurrent.sh | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Show changes to t/t9164-git-svn-dcommit-concurrent.sh +9 −3

diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh
index c8e6c0733f4..3b64022dc57 100755
--- a/t/t9164-git-svn-dcommit-concurrent.sh
+++ b/t/t9164-git-svn-dcommit-concurrent.sh
@@ -47,9 +47,15 @@ setup_hook()
 	echo "cnt=$skip_revs" > "$hook_type-counter"
 	rm -f "$rawsvnrepo/hooks/"*-commit # drop previous hooks
 	hook="$rawsvnrepo/hooks/$hook_type"
-	cat > "$hook" <<- 'EOF1'
+	cat >"$hook" <<-EOF
 		#!/bin/sh
 		set -e
+
+		PATH=\"$PATH\"
+		export PATH
+	EOF
+
+	cat >>"$hook" <<-'EOF'
 		cd "$1/.."  # "$1" is repository location
 		exec >> svn-hook.log 2>&1
 		hook="$(basename "$0")"
@@ -59,11 +65,11 @@ setup_hook()
 		cnt="$(($cnt - 1))"
 		echo "cnt=$cnt" > ./$hook-counter
 		[ "$cnt" = "0" ] || exit 0
-EOF1
+	EOF
+
 	if [ "$hook_type" = "pre-commit" ]; then
 		echo "echo 'commit disallowed' >&2; exit 1" >>"$hook"
 	else
-		echo "PATH=\"$PATH\"; export PATH" >>"$hook"
 		echo "svnconf=\"$svnconf\"" >>"$hook"
 		cat >>"$hook" <<- 'EOF2'
 			cd work-auto-commits.svn

-- 
2.42.0

Jeff King· Nov 8, 2023, 17:21 UTC · re: Patrick Steinhardt · lore

Re: [PATCH v2 3/3] t9164: fix inability to find basename(1) in hooks

On Wed, Nov 08, 2023 at 03:57:27PM +0100, Patrick Steinhardt wrote:

Show 16 quoted lines

> diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh
> index c8e6c0733f4..3b64022dc57 100755
> --- a/t/t9164-git-svn-dcommit-concurrent.sh
> +++ b/t/t9164-git-svn-dcommit-concurrent.sh
> @@ -47,9 +47,15 @@ setup_hook()
>  	echo "cnt=$skip_revs" > "$hook_type-counter"
>  	rm -f "$rawsvnrepo/hooks/"*-commit # drop previous hooks
>  	hook="$rawsvnrepo/hooks/$hook_type"
> -	cat > "$hook" <<- 'EOF1'
> +	cat >"$hook" <<-EOF
>  		#!/bin/sh
>  		set -e
> +
> +		PATH=\"$PATH\"
> +		export PATH
> +	EOF

The quoting here is weird. The original escaped the double-quotes because it was echo-ing and interpolated string:

> -		echo "PATH=\"$PATH\"; export PATH" >>"$hook"

But the here-doc interpolation does not treat double-quotes as special, and you end up with actual double-quote characters at the beginning and end of your $PATH variable, wrecking those entries (but I guess it works some of the time depending on whether those parts of your $PATH are important).

It also interpolates PATH while making the here-doc. That's what you want to convince the hook's inner shell to use the same PATH as the outer shell, but it also means that the inner shell will parse and evaluate it. I don't think anyone is going to inject '"; rm -rf /' into their own test runs, but it does mean we'd do the wrong thing if their $PATH contains double-quotes or backslashes.

Certainly the original suffered from the same issue, so it may not be worth worrying about. But the more robust way to do it is:

  # export ORIG_PATH, which presumably is not cleared the same way PATH
  # is, so that the hook can access it
  ORIG_PATH=$PATH &&
  export ORIG_PATH &&

  cat >"$hook" <<EOF
  # pull the original path from the caller
  PATH=$ORIG_PATH
  export PATH

  ...do other stuff...
  EOF

That's assuming that environment variables make it intact to the hook at all (it is not clear to me why the original $PATH doesn't). If they don't, you'd probably have to stash it in a file like:

  echo "$PATH" >orig-path
  cat >"$hook" <<EOF
  PATH="$(cat orig-path)"
  export PATH
  EOF

-Peff

Junio C Hamano· Nov 8, 2023, 17:43 UTC · re: Jeff King · lore

Re: [PATCH v2 3/3] t9164: fix inability to find basename(1) in hooks

Jeff King <peff@peff.net> writes:

Show 17 quoted lines

> ... But the more robust way to do it is:
>
>   # export ORIG_PATH, which presumably is not cleared the same way PATH
>   # is, so that the hook can access it
>   ORIG_PATH=$PATH &&
>   export ORIG_PATH &&
>
>   cat >"$hook" <<EOF
>   # pull the original path from the caller
>   PATH=$ORIG_PATH
>   export PATH
>
>   ...do other stuff...
>   EOF
>
> That's assuming that environment variables make it intact to the hook at
> all (it is not clear to me why the original $PATH doesn't).

Yeah, the parenthetical comment points at the crux of the issue. I can tell from the patch what issue the platform throws at us we are trying to work around, but it is frustrating not to know why the platform does such unpleasant things in the first place.

Thanks.

Patrick Steinhardt· Nov 9, 2023, 06:30 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 3/3] t9164: fix inability to find basename(1) in hooks

On Thu, Nov 09, 2023 at 02:43:02AM +0900, Junio C Hamano wrote:

Show 26 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > ... But the more robust way to do it is:
> >
> >   # export ORIG_PATH, which presumably is not cleared the same way PATH
> >   # is, so that the hook can access it
> >   ORIG_PATH=$PATH &&
> >   export ORIG_PATH &&
> >
> >   cat >"$hook" <<EOF
> >   # pull the original path from the caller
> >   PATH=$ORIG_PATH
> >   export PATH
> >
> >   ...do other stuff...
> >   EOF
> >
> > That's assuming that environment variables make it intact to the hook at
> > all (it is not clear to me why the original $PATH doesn't).
> 
> Yeah, the parenthetical comment points at the crux of the issue.  I
> can tell from the patch what issue the platform throws at us we are
> trying to work around, but it is frustrating not to know why the
> platform does such unpleasant things in the first place.
> 
> Thanks.

Indeed, I should've described this better in the commit message. Quoting [1]:

Show 7 quoted lines

> By default, Subversion executes hook scripts with an empty
> environment—that is, no environment variables are set at all, not even
> $PATH (or %PATH%, under Windows). Because of this, many administrators
> are baffled when their hook program runs fine by hand, but doesn't
> work when invoked by Subversion. Administrators have historically
> worked around this problem by manually setting all the environment
> variables their hook scripts need in the scripts themselves.

So it's not an issue of the environment, but rather an implementation detail of how Subversion hooks work. It's surprising that this does not fail on other platforms -- maybe the shell has a default PATH there that allow us to locate basename(1)? I dunno.

Will add to the commit message.

Patrick

[1]: https://svnbook.red-bean.com/en/1.8/svn.reposadmin.create.html#svn.reposadmin.create.hooks

Patrick Steinhardt· Nov 9, 2023, 07:02 UTC · re: Patrick Steinhardt · lore

Re: [PATCH v2 3/3] t9164: fix inability to find basename(1) in hooks

On Thu, Nov 09, 2023 at 07:30:01AM +0100, Patrick Steinhardt wrote:

> On Thu, Nov 09, 2023 at 02:43:02AM +0900, Junio C Hamano wrote:
> > Jeff King <peff@peff.net> writes:

[snip]

Show 12 quoted lines

> > By default, Subversion executes hook scripts with an empty
> > environment—that is, no environment variables are set at all, not even
> > $PATH (or %PATH%, under Windows). Because of this, many administrators
> > are baffled when their hook program runs fine by hand, but doesn't
> > work when invoked by Subversion. Administrators have historically
> > worked around this problem by manually setting all the environment
> > variables their hook scripts need in the scripts themselves.
> 
> So it's not an issue of the environment, but rather an implementation
> detail of how Subversion hooks work. It's surprising that this does not
> fail on other platforms -- maybe the shell has a default PATH there that
> allow us to locate basename(1)? I dunno.

Yup, that's in fact it. Bash falls back to a default PATH that can be queried via getconf(1p), and this works alright on most platforms. Not on NixOS though.

Patrick

Patrick Steinhardt· Nov 9, 2023, 07:09 UTC · re: Patrick Steinhardt · lore

[PATCH v3 0/3] t: improve compatibility with NixOS

Hi,

this is the third version of my patch series to improve compatibility of our tests with NixOS.

Changes compared to v2:

    - Patch 1: We now check for both httpd and apache2 binaries via
      PATH.

    - Patch 1: We're a bit more defensive and will check whether
      variables are empty before passing them to either `test -d` or
      `test -x`.

    - Patch 3: Clarified why PATH is unset.

    - Patch 3: Instead of writing PATH into the hook we now write it
      into the `hook-env` configuration file read by Subversion.

Patrick

Patrick Steinhardt (3):
  t/lib-httpd: dynamically detect httpd and modules path
  t/lib-httpd: stop using legacy crypt(3) for authentication
  t9164: fix inability to find basename(1) in Subversion hooks

 t/lib-httpd.sh                        | 17 +++++++++++++----
 t/lib-httpd/passwd                    |  2 +-
 t/lib-httpd/proxy-passwd              |  2 +-
 t/t9164-git-svn-dcommit-concurrent.sh |  9 ++++++++-
 4 files changed, 23 insertions(+), 7 deletions(-)

Range-diff against v2:
1:  cee8fbebf84 ! 1:  e4c75c492dd t/lib-httpd: dynamically detect httpd and modules path
    @@ t/lib-httpd.sh: 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)"
    ++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 -x "$DEFAULT_HTTPD_PATH"
    ++	if test -n "$DEFAULT_HTTPD_PATH" -a -x "$DEFAULT_HTTPD_PATH"
      	then
    -@@ t/lib-httpd.sh: do
    + 		break
      	fi
      done
      
    @@ t/lib-httpd.sh: do
     +				 '/usr/libexec/httpd' \
     +				 "${DETECTED_HTTPD_ROOT:+${DETECTED_HTTPD_ROOT}/modules}"
      do
    - 	if test -d "$DEFAULT_HTTPD_MODULE_PATH"
    +-	if test -d "$DEFAULT_HTTPD_MODULE_PATH"
    ++	if test -n "$DEFAULT_HTTPD_MODULE_PATH" -a -d "$DEFAULT_HTTPD_MODULE_PATH"
      	then
    + 		break
    + 	fi
2:  f4c6c754f1e = 2:  3dce76da477 t/lib-httpd: stop using legacy crypt(3) for authentication
3:  361f1bd9c88 ! 3:  6891e254155 t9164: fix inability to find basename(1) in hooks
    @@ Metadata
     Author: Patrick Steinhardt <ps@pks.im>
     
      ## Commit message ##
    -    t9164: fix inability to find basename(1) in hooks
    +    t9164: fix inability to find basename(1) in Subversion hooks
     
    -    The post-commit hook in t9164 is executed with a default environment.
    -    To work around this issue we already write the current `PATH` value into
    -    the hook script. But this is done at a point where we already tried to
    -    execute non-built-in commands, namely basename(1). While this seems to
    -    work alright on most platforms, it fails on NixOS.
    +    Hooks executed by Subversion are spawned with an empty environment. By
    +    default, not even variables like PATH will be propagated to them. In
    +    order to ensure that we're still able to find required executables, we
    +    thus write the current PATH variable into the hook script itself and
    +    then re-export it in t9164.
     
    -    Set the `PATH` variable earlier to fix this issue. Note that we do this
    -    via two separate heredocs. This is done because in the first one we do
    -    want to expand variables, whereas in the second one we don't.
    +    This happens too late in the script though, as we already tried to
    +    execute the basename(1) utility before exporting the PATH variable. This
    +    tends to work on most platforms as the fallback value of PATH for Bash
    +    (see `getconf PATH`) is likely to contain this binary. But on more
    +    exotic platforms like NixOS this is not the case, and thus the test
    +    fails.
    +
    +    While we could work around this issue by simply setting PATH earlier, it
    +    feels fragile to inject a user-controlled value into the script and have
    +    the shell interpret it. Instead, we can refactor the hook setup to write
    +    a `hooks-env` file that configures PATH for us. Like this, Subversion
    +    will know to set up the environment as expected for all hooks.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
      ## t/t9164-git-svn-dcommit-concurrent.sh ##
     @@ t/t9164-git-svn-dcommit-concurrent.sh: setup_hook()
    + 			"passed to setup_hook" >&2 ; return 1; }
      	echo "cnt=$skip_revs" > "$hook_type-counter"
      	rm -f "$rawsvnrepo/hooks/"*-commit # drop previous hooks
    - 	hook="$rawsvnrepo/hooks/$hook_type"
    --	cat > "$hook" <<- 'EOF1'
    -+	cat >"$hook" <<-EOF
    - 		#!/bin/sh
    - 		set -e
    -+
    -+		PATH=\"$PATH\"
    -+		export PATH
    -+	EOF
     +
    -+	cat >>"$hook" <<-'EOF'
    - 		cd "$1/.."  # "$1" is repository location
    - 		exec >> svn-hook.log 2>&1
    - 		hook="$(basename "$0")"
    -@@ t/t9164-git-svn-dcommit-concurrent.sh: setup_hook()
    - 		cnt="$(($cnt - 1))"
    - 		echo "cnt=$cnt" > ./$hook-counter
    - 		[ "$cnt" = "0" ] || exit 0
    --EOF1
    ++	# Subversion hooks run with an empty environment by default. We thus
    ++	# need to propagate PATH so that we can find executables.
    ++	cat >"$rawsvnrepo/conf/hooks-env" <<-EOF
    ++	[default]
    ++	PATH = ${PATH}
     +	EOF
     +
    + 	hook="$rawsvnrepo/hooks/$hook_type"
    + 	cat > "$hook" <<- 'EOF1'
    + 		#!/bin/sh
    +@@ t/t9164-git-svn-dcommit-concurrent.sh: EOF1
      	if [ "$hook_type" = "pre-commit" ]; then
      		echo "echo 'commit disallowed' >&2; exit 1" >>"$hook"
      	else

base-commit: dadef801b365989099a9929e995589e455c51fed

-- 
2.42.0

Patrick Steinhardt· Nov 9, 2023, 07:09 UTC · re: Patrick Steinhardt · lore

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

    - 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(-)

Show changes to t/lib-httpd.sh +13 −4

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

-- 
2.42.0

Jeff King· Nov 9, 2023, 07:32 UTC · re: Patrick Steinhardt · lore

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

On Thu, Nov 09, 2023 at 08:09:52AM +0100, Patrick Steinhardt wrote:

Show 8 quoted lines

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

Patrick Steinhardt· Nov 9, 2023, 07:36 UTC · re: Jeff King · lore

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

On Thu, Nov 09, 2023 at 02:32:50AM -0500, Jeff King wrote:

Show 18 quoted lines

> 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

Junio C Hamano· Nov 9, 2023, 07:46 UTC · re: Patrick Steinhardt · lore

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

Patrick Steinhardt <ps@pks.im> writes:

Show 5 quoted lines

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

Patrick Steinhardt· Nov 9, 2023, 07:57 UTC · re: Junio C Hamano · lore

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

On Thu, Nov 09, 2023 at 04:46:05PM +0900, Junio C Hamano wrote:

Show 11 quoted lines

> 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?

Show 11 quoted lines

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

Show changes to t/lib-httpd.sh +2 −2

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

Jeff King· Nov 9, 2023, 07:48 UTC · re: Patrick Steinhardt · lore

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

On Thu, Nov 09, 2023 at 08:36:53AM +0100, Patrick Steinhardt wrote:

Show 13 quoted lines

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

Patrick Steinhardt· Nov 9, 2023, 07:09 UTC · re: Patrick Steinhardt · lore

[PATCH v3 2/3] t/lib-httpd: stop using legacy crypt(3) for authentication

This schema can be considered deprecated nowadays as it is not safe anymore. Quoting Apache httpd's documentation [1]:

> Unix only. Uses the traditional Unix crypt(3) function with a
> randomly-generated 32-bit salt (only 12 bits used) and the first 8
> characters of the password. Insecure.

[1]: https://httpd.apache.org/docs/2.4/misc/password_encryptions.html

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/lib-httpd/passwd       | 2 +-
 t/lib-httpd/proxy-passwd | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Show changes to 2 files +2 −2

t/lib-httpd/passwd, t/lib-httpd/proxy-passwd

diff --git a/t/lib-httpd/passwd b/t/lib-httpd/passwd
index 99a34d64874..d9c122f3482 100644
--- a/t/lib-httpd/passwd
+++ b/t/lib-httpd/passwd
@@ -1 +1 @@
-user@host:xb4E8pqD81KQs
+user@host:$apr1$LGPmCZWj$9vxEwj5Z5GzQLBMxp3mCx1
diff --git a/t/lib-httpd/proxy-passwd b/t/lib-httpd/proxy-passwd
index 77c25138e07..2ad7705d9a3 100644
--- a/t/lib-httpd/proxy-passwd
+++ b/t/lib-httpd/proxy-passwd
@@ -1 +1 @@
-proxuser:2x7tAukjAED5M
+proxuser:$apr1$RxS6MLkD$DYsqQdflheq4GPNxzJpx5.

-- 
2.42.0

Patrick Steinhardt· Nov 9, 2023, 07:10 UTC · re: Patrick Steinhardt · lore

[PATCH v3 3/3] t9164: fix inability to find basename(1) in Subversion hooks

Hooks executed by Subversion are spawned with an empty environment. By default, not even variables like PATH will be propagated to them. In order to ensure that we're still able to find required executables, we thus write the current PATH variable into the hook script itself and then re-export it in t9164.

This happens too late in the script though, as we already tried to execute the basename(1) utility before exporting the PATH variable. This tends to work on most platforms as the fallback value of PATH for Bash (see `getconf PATH`) is likely to contain this binary. But on more exotic platforms like NixOS this is not the case, and thus the test fails.

While we could work around this issue by simply setting PATH earlier, it feels fragile to inject a user-controlled value into the script and have the shell interpret it. Instead, we can refactor the hook setup to write a `hooks-env` file that configures PATH for us. Like this, Subversion will know to set up the environment as expected for all hooks.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t9164-git-svn-dcommit-concurrent.sh | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Show changes to t/t9164-git-svn-dcommit-concurrent.sh +8 −1

diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh
index c8e6c0733f4..d1dec89c3b7 100755
--- a/t/t9164-git-svn-dcommit-concurrent.sh
+++ b/t/t9164-git-svn-dcommit-concurrent.sh
@@ -46,6 +46,14 @@ setup_hook()
 			"passed to setup_hook" >&2 ; return 1; }
 	echo "cnt=$skip_revs" > "$hook_type-counter"
 	rm -f "$rawsvnrepo/hooks/"*-commit # drop previous hooks
+
+	# Subversion hooks run with an empty environment by default. We thus
+	# need to propagate PATH so that we can find executables.
+	cat >"$rawsvnrepo/conf/hooks-env" <<-EOF
+	[default]
+	PATH = ${PATH}
+	EOF
+
 	hook="$rawsvnrepo/hooks/$hook_type"
 	cat > "$hook" <<- 'EOF1'
 		#!/bin/sh
@@ -63,7 +71,6 @@ EOF1
 	if [ "$hook_type" = "pre-commit" ]; then
 		echo "echo 'commit disallowed' >&2; exit 1" >>"$hook"
 	else
-		echo "PATH=\"$PATH\"; export PATH" >>"$hook"
 		echo "svnconf=\"$svnconf\"" >>"$hook"
 		cat >>"$hook" <<- 'EOF2'
 			cd work-auto-commits.svn

-- 
2.42.0

Jeff King· Nov 9, 2023, 07:35 UTC · re: Patrick Steinhardt · lore

Re: [PATCH v3 3/3] t9164: fix inability to find basename(1) in Subversion hooks

On Thu, Nov 09, 2023 at 08:10:01AM +0100, Patrick Steinhardt wrote:

Show 6 quoted lines

> +	# Subversion hooks run with an empty environment by default. We thus
> +	# need to propagate PATH so that we can find executables.
> +	cat >"$rawsvnrepo/conf/hooks-env" <<-EOF
> +	[default]
> +	PATH = ${PATH}
> +	EOF

This is so much less ugly than the shell shenanigans discussed in the earlier round. Thanks for finding it. :)

The ${PATH} here is interpolated by the here-doc. I guess it's possible somebody's exotic PATH could break the svn conf syntax, but it's probably not worth worrying about.

-Peff

Jeff King· Nov 9, 2023, 07:36 UTC · re: Patrick Steinhardt · lore

Re: [PATCH v3 0/3] t: improve compatibility with NixOS

On Thu, Nov 09, 2023 at 08:09:47AM +0100, Patrick Steinhardt wrote:

Show 16 quoted lines

> this is the third version of my patch series to improve compatibility of
> our tests with NixOS.
> 
> Changes compared to v2:
> 
>     - Patch 1: We now check for both httpd and apache2 binaries via
>       PATH.
> 
>     - Patch 1: We're a bit more defensive and will check whether
>       variables are empty before passing them to either `test -d` or
>       `test -x`.
> 
>     - Patch 3: Clarified why PATH is unset.
> 
>     - Patch 3: Instead of writing PATH into the hook we now write it
>       into the `hook-env` configuration file read by Subversion.

Thanks. This looks good to me, modulo a minor shell nit I mentioned in the first patch.

-Peff

Patrick Steinhardt· Nov 10, 2023, 08:16 UTC · re: Patrick Steinhardt · lore

[PATCH v4 0/3] t: improve compatibility with NixOS

Hi,

this is the fourth version of my patch series to improve compatibility of our test suite with NixOS.

Three changes compared to v3:

    - Switched from `test <expr> -a <expr>` to `test <expr> && test
      <expr>`.

    - Improved the commit message to explain why the new
      runtime-detected paths are only used as a fallback.

    - Rebased on top of 0e3b67e2aa (ci: add support for GitLab CI,
      2023-11-09), which has been merged to `next` and caused conflicts.

Technically speaking this series also depends on 0763c3a2c4 (http: update curl http/2 info matching for curl 8.3.0, 2023-09-15), without which the tests will fail on NixOS machines with a recent libcurl.

Patrick

Patrick Steinhardt (3):
  t/lib-httpd: dynamically detect httpd and modules path
  t/lib-httpd: stop using legacy crypt(3) for authentication
  t9164: fix inability to find basename(1) in Subversion hooks

 t/lib-httpd.sh                        | 17 +++++++++++++----
 t/lib-httpd/passwd                    |  2 +-
 t/lib-httpd/proxy-passwd              |  2 +-
 t/t9164-git-svn-dcommit-concurrent.sh |  9 ++++++++-
 4 files changed, 23 insertions(+), 7 deletions(-)

Range-diff against v3:
1:  e4c75c492dd ! 1:  41b9dada2e0 t/lib-httpd: dynamically detect httpd and modules path
    @@ Commit message
             - 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.
    +    Amend the code to do so.
    +
    +    Note that the new runtime-detected paths will only be used as a fallback
    +    in case none of the hardcoded paths are usable. For the PATH lookup this
    +    is because httpd is typically installed into "/usr/sbin", which is often
    +    not included in the user's PATH variable. And the module path detection
    +    relies on a configured httpd installation and may thus not work in all
    +    cases, either.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
    @@ t/lib-httpd.sh: fi
     +			  "$(command -v apache2)"
      do
     -	if test -x "$DEFAULT_HTTPD_PATH"
    -+	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
    @@ t/lib-httpd.sh: fi
      				 '/usr/lib/apache2/modules' \
      				 '/usr/lib64/httpd/modules' \
      				 '/usr/lib/httpd/modules' \
    --				 '/usr/libexec/httpd'
    -+				 '/usr/libexec/httpd' \
    + 				 '/usr/libexec/httpd' \
    +-				 '/usr/lib/apache2'
    ++				 '/usr/lib/apache2' \
     +				 "${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"
    ++	if test -n "$DEFAULT_HTTPD_MODULE_PATH" && test -d "$DEFAULT_HTTPD_MODULE_PATH"
      	then
      		break
      	fi
2:  3dce76da477 = 2:  7d28c32feaa t/lib-httpd: stop using legacy crypt(3) for authentication
3:  6891e254155 = 3:  6a7773aadba t9164: fix inability to find basename(1) in Subversion hooks

base-commit: 0e3b67e2aa25edb7e1a5c999c87b52a7b3a7649a

-- 
2.42.0

Patrick Steinhardt· Nov 10, 2023, 08:17 UTC · re: Patrick Steinhardt · lore

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

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

Note that the new runtime-detected paths will only be used as a fallback in case none of the hardcoded paths are usable. For the PATH lookup this is because httpd is typically installed into "/usr/sbin", which is often not included in the user's PATH variable. And the module path detection relies on a configured httpd installation and may thus not work in all cases, either.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/lib-httpd.sh | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

Show changes to t/lib-httpd.sh +13 −4

diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 9ea74927c40..f69d0da51d1 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -55,22 +55,31 @@ 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" && test -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/lib/apache2'
+				 '/usr/lib/apache2' \
+				 "${DETECTED_HTTPD_ROOT:+${DETECTED_HTTPD_ROOT}/modules}"
 do
-	if test -d "$DEFAULT_HTTPD_MODULE_PATH"
+	if test -n "$DEFAULT_HTTPD_MODULE_PATH" && test -d "$DEFAULT_HTTPD_MODULE_PATH"
 	then
 		break
 	fi

-- 
2.42.0

Junio C Hamano· Nov 11, 2023, 00:00 UTC · re: Patrick Steinhardt · lore

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

Patrick Steinhardt <ps@pks.im> writes:

> +if test -x "$DEFAULT_HTTPD_PATH"
> +then
> +	DETECTED_HTTPD_ROOT="$("$DEFAULT_HTTPD_PATH" -V | sed -n 's/^ -D HTTPD_ROOT="\(.*\)"$/\1/p')"
> +fi

With this patch, my test run starts like so:

    rm -f -r 'test-results'
    *** prove ***
    apache2: Could not open configuration file /etc/apache2/apache2.conf: No such file or directory
    ...

I find the error message leaking mildly annoying, and would suggest doing something like the following on top.

Show changes to diff +1 −1

diff --git c/t/lib-httpd.sh w/t/lib-httpd.sh
index 0a74922d7f..03493ee72b 100644
--- c/t/lib-httpd.sh
+++ w/t/lib-httpd.sh
@@ -68,7 +68,7 @@ done
 
 if test -x "$DEFAULT_HTTPD_PATH"
 then
-	DETECTED_HTTPD_ROOT="$("$DEFAULT_HTTPD_PATH" -V | sed -n 's/^ -D HTTPD_ROOT="\(.*\)"$/\1/p')"
+	DETECTED_HTTPD_ROOT="$("$DEFAULT_HTTPD_PATH" -V 2>/dev/null | sed -n 's/^ -D HTTPD_ROOT="\(.*\)"$/\1/p')"
 fi
 
 for DEFAULT_HTTPD_MODULE_PATH in '/usr/libexec/apache2' \

Patrick Steinhardt· Nov 13, 2023, 07:15 UTC · re: Junio C Hamano · lore

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

On Sat, Nov 11, 2023 at 09:00:09AM +0900, Junio C Hamano wrote:

Show 30 quoted lines

> Patrick Steinhardt <ps@pks.im> writes:
> 
> > +if test -x "$DEFAULT_HTTPD_PATH"
> > +then
> > +	DETECTED_HTTPD_ROOT="$("$DEFAULT_HTTPD_PATH" -V | sed -n 's/^ -D HTTPD_ROOT="\(.*\)"$/\1/p')"
> > +fi
> 
> With this patch, my test run starts like so:
> 
>     rm -f -r 'test-results'
>     *** prove ***
>     apache2: Could not open configuration file /etc/apache2/apache2.conf: No such file or directory
>     ...
> 
> I find the error message leaking mildly annoying, and would suggest
> doing something like the following on top.
> 
> diff --git c/t/lib-httpd.sh w/t/lib-httpd.sh
> index 0a74922d7f..03493ee72b 100644
> --- c/t/lib-httpd.sh
> +++ w/t/lib-httpd.sh
> @@ -68,7 +68,7 @@ done
>  
>  if test -x "$DEFAULT_HTTPD_PATH"
>  then
> -	DETECTED_HTTPD_ROOT="$("$DEFAULT_HTTPD_PATH" -V | sed -n 's/^ -D HTTPD_ROOT="\(.*\)"$/\1/p')"
> +	DETECTED_HTTPD_ROOT="$("$DEFAULT_HTTPD_PATH" -V 2>/dev/null | sed -n 's/^ -D HTTPD_ROOT="\(.*\)"$/\1/p')"
>  fi
>  
>  for DEFAULT_HTTPD_MODULE_PATH in '/usr/libexec/apache2' \

Yup, makes sense. Thanks for the tweak.

Patrick

Patrick Steinhardt· Nov 10, 2023, 08:17 UTC · re: Patrick Steinhardt · lore

[PATCH v4 2/3] t/lib-httpd: stop using legacy crypt(3) for authentication

This schema can be considered deprecated nowadays as it is not safe anymore. Quoting Apache httpd's documentation [1]:

> Unix only. Uses the traditional Unix crypt(3) function with a
> randomly-generated 32-bit salt (only 12 bits used) and the first 8
> characters of the password. Insecure.

[1]: https://httpd.apache.org/docs/2.4/misc/password_encryptions.html

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/lib-httpd/passwd       | 2 +-
 t/lib-httpd/proxy-passwd | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Show changes to 2 files +2 −2

t/lib-httpd/passwd, t/lib-httpd/proxy-passwd

diff --git a/t/lib-httpd/passwd b/t/lib-httpd/passwd
index 99a34d64874..d9c122f3482 100644
--- a/t/lib-httpd/passwd
+++ b/t/lib-httpd/passwd
@@ -1 +1 @@
-user@host:xb4E8pqD81KQs
+user@host:$apr1$LGPmCZWj$9vxEwj5Z5GzQLBMxp3mCx1
diff --git a/t/lib-httpd/proxy-passwd b/t/lib-httpd/proxy-passwd
index 77c25138e07..2ad7705d9a3 100644
--- a/t/lib-httpd/proxy-passwd
+++ b/t/lib-httpd/proxy-passwd
@@ -1 +1 @@
-proxuser:2x7tAukjAED5M
+proxuser:$apr1$RxS6MLkD$DYsqQdflheq4GPNxzJpx5.

-- 
2.42.0

Patrick Steinhardt· Nov 10, 2023, 08:17 UTC · re: Patrick Steinhardt · lore

[PATCH v4 3/3] t9164: fix inability to find basename(1) in Subversion hooks

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t9164-git-svn-dcommit-concurrent.sh | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Show changes to t/t9164-git-svn-dcommit-concurrent.sh +8 −1

diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh
index c8e6c0733f4..d1dec89c3b7 100755
--- a/t/t9164-git-svn-dcommit-concurrent.sh
+++ b/t/t9164-git-svn-dcommit-concurrent.sh
@@ -46,6 +46,14 @@ setup_hook()
 			"passed to setup_hook" >&2 ; return 1; }
 	echo "cnt=$skip_revs" > "$hook_type-counter"
 	rm -f "$rawsvnrepo/hooks/"*-commit # drop previous hooks
+
+	# Subversion hooks run with an empty environment by default. We thus
+	# need to propagate PATH so that we can find executables.
+	cat >"$rawsvnrepo/conf/hooks-env" <<-EOF
+	[default]
+	PATH = ${PATH}
+	EOF
+
 	hook="$rawsvnrepo/hooks/$hook_type"
 	cat > "$hook" <<- 'EOF1'
 		#!/bin/sh
@@ -63,7 +71,6 @@ EOF1
 	if [ "$hook_type" = "pre-commit" ]; then
 		echo "echo 'commit disallowed' >&2; exit 1" >>"$hook"
 	else
-		echo "PATH=\"$PATH\"; export PATH" >>"$hook"
 		echo "svnconf=\"$svnconf\"" >>"$hook"
 		cat >>"$hook" <<- 'EOF2'
 			cd work-auto-commits.svn

-- 
2.42.0

Jeff King· Nov 10, 2023, 21:41 UTC · re: Patrick Steinhardt · lore

Re: [PATCH v4 0/3] t: improve compatibility with NixOS

On Fri, Nov 10, 2023 at 09:16:56AM +0100, Patrick Steinhardt wrote:

Show 13 quoted lines

> this is the fourth version of my patch series to improve compatibility
> of our test suite with NixOS.
> 
> Three changes compared to v3:
> 
>     - Switched from `test <expr> -a <expr>` to `test <expr> && test
>       <expr>`.
> 
>     - Improved the commit message to explain why the new
>       runtime-detected paths are only used as a fallback.
> 
>     - Rebased on top of 0e3b67e2aa (ci: add support for GitLab CI,
>       2023-11-09), which has been merged to `next` and caused conflicts.

Thanks, these all look good to me.

-Peff

Junio C Hamano· Nov 11, 2023, 00:10 UTC · re: Patrick Steinhardt · lore

Re: [PATCH v4 0/3] t: improve compatibility with NixOS

Patrick Steinhardt <ps@pks.im> writes:

Show 10 quoted lines

> Three changes compared to v3:
>
>     - Switched from `test <expr> -a <expr>` to `test <expr> && test
>       <expr>`.
>
>     - Improved the commit message to explain why the new
>       runtime-detected paths are only used as a fallback.
>
>     - Rebased on top of 0e3b67e2aa (ci: add support for GitLab CI,
>       2023-11-09), which has been merged to `next` and caused conflicts.

Please, no. "The other topic has been merged to 'next'" is *not* a good reason to do this. Before that, the other topic was in 'seen' and causing conflicts already, so getting into 'next' did not create any new reason for rebasing.

I'll manage this time, but please do not do such a rebase unless you are asked to do so. The rebase will force me to do (1) detach from 'next' and apply these, (2) discard the result and detach from the base of where the previous iteration is queued, (3) apply the new iteration with "am -3" to undo the rebase, before I can compare the new iteration with the old iteration.

> Technically speaking this series also depends on 0763c3a2c4 (http:
> update curl http/2 info matching for curl 8.3.0, 2023-09-15), without
> which the tests will fail on NixOS machines with a recent libcurl.

Thanks for that note. This topic has been queued on top of v2.43.0-rc1 which has 0763c3a2c4, so we'd be safe.

Will queue.

Patrick Steinhardt· Nov 13, 2023, 07:15 UTC · re: Junio C Hamano · lore

Re: [PATCH v4 0/3] t: improve compatibility with NixOS

On Sat, Nov 11, 2023 at 09:10:23AM +0900, Junio C Hamano wrote:

Show 24 quoted lines

> Patrick Steinhardt <ps@pks.im> writes:
> 
> > Three changes compared to v3:
> >
> >     - Switched from `test <expr> -a <expr>` to `test <expr> && test
> >       <expr>`.
> >
> >     - Improved the commit message to explain why the new
> >       runtime-detected paths are only used as a fallback.
> >
> >     - Rebased on top of 0e3b67e2aa (ci: add support for GitLab CI,
> >       2023-11-09), which has been merged to `next` and caused conflicts.
> 
> Please, no.  "The other topic has been merged to 'next'" is *not* a
> good reason to do this.  Before that, the other topic was in 'seen'
> and causing conflicts already, so getting into 'next' did not create
> any new reason for rebasing.
> 
> I'll manage this time, but please do not do such a rebase unless you
> are asked to do so.  The rebase will force me to do (1) detach from
> 'next' and apply these, (2) discard the result and detach from the
> base of where the previous iteration is queued, (3) apply the new
> iteration with "am -3" to undo the rebase, before I can compare the
> new iteration with the old iteration.

Fair enough. I assumed that it would ease your workload instead of creating more work for you. But I'll keep in mind that it doesn't and refrain from doing this in the future.

Show 8 quoted lines

> > Technically speaking this series also depends on 0763c3a2c4 (http:
> > update curl http/2 info matching for curl 8.3.0, 2023-09-15), without
> > which the tests will fail on NixOS machines with a recent libcurl.
> 
> Thanks for that note.  This topic has been queued on top of
> v2.43.0-rc1 which has 0763c3a2c4, so we'd be safe.
> 
> Will queue.

Thanks.

Patrick

Junio C Hamano· Nov 13, 2023, 23:42 UTC · re: Patrick Steinhardt · lore

Re: [PATCH v4 0/3] t: improve compatibility with NixOS

Patrick Steinhardt <ps@pks.im> writes:

> Fair enough. I assumed that it would ease your workload instead of
> creating more work for you. But I'll keep in mind that it doesn't and
> refrain from doing this in the future.

Thanks.

With or without conflicts, basing your work on 'next' would not be a good idea to begin with, as we never merge 'next' down to 'master' (we only merge individual topics).

The following applies not specifically to you but to all contributors.

What is helpful is in a tricky case is to start your topic branch development at an appropriate base (often the tip of 'master', but for a bugfix the tip of 'maint'), merge in other topics in flight you depend on that are not in the target base (which limits what you can depend on if you are writing a bugfix on 'maint'), and then start building on top of the merge result. Then communicate how you built this (artificial) base clearly in your cover letter.

Whether you have "other topics in flight" merged to your base or not, creating a trial merge of your topic to seen to see how others' work-in-progress may interact with your work would be a valuable way to keep aware of what is cooking and how you will be affected.

Thanks.