t: improve compatibility with NixOS
cover t: improve compatibility with NixOS
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(-)
Signature · 1 line
2.42.0
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.
--- t/lib-httpd.sh | 51 +++++++++++++++++++++++++++++++++----------------- 1 file changed, 34 insertions(+), 17 deletions(-)
t/lib-httpd.sh +34 -17
@@ -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)
Signature · 1 line
2.42.0
Re: [PATCH 1/3] t/lib-httpd: dynamically detect httpd and modules path
Quoted 11 lines · Patrick Steinhardt
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_PATHandLIB_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:
Quoted 7 lines
- The httpd binary can be located via PATH.
- The module directory can (in many cases) be derived via the
HTTPD_ROOTcompile-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.
Re: [PATCH 1/3] t/lib-httpd: dynamically detect httpd and modules path
Quoted 23 lines · Junio C Hamano
Quoted 11 lines · Patrick Steinhardt
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_PATHandLIB_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:Quoted 7 lines
- The httpd binary can be located via PATH.
- The module directory can (in many cases) be derived via the
HTTPD_ROOTcompile-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.
Quoted 7 lines
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
Quoted 7 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.
Re: [PATCH 1/3] t/lib-httpd: dynamically detect httpd and modules path
Quoted 4 lines · Patrick Steinhardt
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
PATCH 2/3 t/lib-httpd: stop using legacy crypt(3) for authentication
In reply to Patrick SteinhardtWhen 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]:
Quoted 3 lines
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
--- t/lib-httpd/passwd | 2 +- t/lib-httpd/proxy-passwd | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
t/lib-httpd/passwd +1 -1
@@ -1 +1 @@ -user@host:xb4E8pqD81KQs +user@host:$apr1$LGPmCZWj$9vxEwj5Z5GzQLBMxp3mCx1
t/lib-httpd/proxy-passwd +1 -1
@@ -1 +1 @@ -proxuser:2x7tAukjAED5M +proxuser:$apr1$RxS6MLkD$DYsqQdflheq4GPNxzJpx5.
Signature · 1 line
2.42.0
Re: [PATCH 2/3] t/lib-httpd: stop using legacy crypt(3) for authentication
Quoted 19 lines · Patrick Steinhardt
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]:
Quoted 3 lines
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.
Quoted 6 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.
This step makes quite a lot of sense, as we are changing this not at all for security but for portability ;-)
Quoted 23 lines
[1]: https://httpd.apache.org/docs/2.4/misc/password_encryptions.html
Signed-off-by Patrick Steinhardt <[email protected]>--- t/lib-httpd/passwd | 2 +- t/lib-httpd/proxy-passwd | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)t/lib-httpd/passwd +1
-1@@ -1 +1 @@ -user@host:xb4E8pqD81KQs +user@host:$apr1$LGPmCZWj$9vxEwj5Z5GzQLBMxp3mCx1t/lib-httpd/proxy-passwd +1
-1@@ -1 +1 @@ -proxuser:2x7tAukjAED5M +proxuser:$apr1$RxS6MLkD$DYsqQdflheq4GPNxzJpx5.
PATCH 3/3 t9164: fix inability to find basename(1) in hooks
In reply to Patrick SteinhardtThe 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.
--- t/t9164-git-svn-dcommit-concurrent.sh | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)
t/t9164-git-svn-dcommit-concurrent.sh +9 -3
@@ -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
Signature · 1 line
2.42.0
v2 cover t: improve compatibility with NixOS
In reply to Patrick SteinhardtHi,
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 <[email protected]>
@@ 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 hooksbase-commit: 98009afd24e2304bf923a64750340423473809ff
Signature · 1 line
2.42.0
PATCH v2 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.
Amend the code to do so. The new runtime-detected paths will only be used in case none of the hardcoded paths are usable.
--- t/lib-httpd.sh | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
t/lib-httpd.sh +8 -2
@@ -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
Signature · 1 line
2.42.0
Re: [PATCH v2 1/3] t/lib-httpd: dynamically detect httpd and modules path
Quoted 5 lines · Patrick Steinhardt
While it is possible to specify these paths via
LIB_HTTPD_PATHandLIB_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:
Quoted 7 lines
- The httpd binary can be located via PATH.
- The module directory can (in many cases) be derived via the
HTTPD_ROOTcompile-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:
t/lib-httpd.sh fi · line 61
Quoted 5 lines
t/lib-httpd.sh +1
-1-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).
t/lib-httpd.sh do · line 69
Quoted 4 lines
t/lib-httpd.sh +4
-0+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
Re: [PATCH v2 1/3] t/lib-httpd: dynamically detect httpd and modules path
Quoted 19 lines · Jeff King
Quoted 5 lines · Patrick Steinhardt
While it is possible to specify these paths via
LIB_HTTPD_PATHandLIB_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:Quoted 7 lines
- The httpd binary can be located via PATH.
- The module directory can (in many cases) be derived via the
HTTPD_ROOTcompile-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:
Quoted 9 lines
t/lib-httpd.sh fi · line 61
Quoted 5 lines
t/lib-httpd.sh +1
-1-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" thenThe 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'
Quoted 2 lines
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.
Quoted 3 lines
... 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.
Re: [PATCH v2 1/3] t/lib-httpd: dynamically detect httpd and modules path
In reply to Jeff KingQuoted 19 lines · Jeff King
Quoted 5 lines · Patrick Steinhardt
While it is possible to specify these paths via
LIB_HTTPD_PATHandLIB_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:Quoted 7 lines
- The httpd binary can be located via PATH.
- The module directory can (in many cases) be derived via the
HTTPD_ROOTcompile-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:
Quoted 9 lines
t/lib-httpd.sh fi · line 61
Quoted 5 lines
t/lib-httpd.sh +1
-1-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" thenThe 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.
Quoted 2 lines
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.
Quoted 12 lines
t/lib-httpd.sh do · line 69
Quoted 4 lines
t/lib-httpd.sh +4
-0+if test -x "$DEFAULT_HTTPD_PATH" +then + DETECTED_HTTPD_ROOT="$("$DEFAULT_HTTPD_PATH" -V | sed -n 's/^ -D HTTPD_ROOT="\(.*\)"$/\1/p')" +fiI 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:
Quoted 6 lines
$ /usr/sbin/apache2 -D HTTPD_ROOT apache2: Could not open configuration file /etc/apache2/apache2.conf: No such file or directorySo 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
PATCH v2 2/3 t/lib-httpd: stop using legacy crypt(3) for authentication
In reply to Patrick SteinhardtWhen 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]:
Quoted 3 lines
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
--- t/lib-httpd/passwd | 2 +- t/lib-httpd/proxy-passwd | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
t/lib-httpd/passwd +1 -1
@@ -1 +1 @@ -user@host:xb4E8pqD81KQs +user@host:$apr1$LGPmCZWj$9vxEwj5Z5GzQLBMxp3mCx1
t/lib-httpd/proxy-passwd +1 -1
@@ -1 +1 @@ -proxuser:2x7tAukjAED5M +proxuser:$apr1$RxS6MLkD$DYsqQdflheq4GPNxzJpx5.
Signature · 1 line
2.42.0
Re: [PATCH v2 2/3] t/lib-httpd: stop using legacy crypt(3) for authentication
Quoted 6 lines · Patrick Steinhardt
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
PATCH v2 3/3 t9164: fix inability to find basename(1) in hooks
In reply to Patrick SteinhardtThe 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.
--- t/t9164-git-svn-dcommit-concurrent.sh | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)
t/t9164-git-svn-dcommit-concurrent.sh +9 -3
@@ -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
Signature · 1 line
2.42.0
Re: [PATCH v2 3/3] t9164: fix inability to find basename(1) in hooks
Quoted 6 lines · Patrick Steinhardt
t/t9164-git-svn-dcommit-concurrent.sh +0
-0@@ -47,9 +47,15 @@ setup_hook() echo "cnt=$skip_revs" > "$hook_type-counter"
t/t9164-git-svn-dcommit-concurrent.sh setup_hook() · line 56
Quoted 10 lines
t/t9164-git-svn-dcommit-concurrent.sh +5
-1rm -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:
t/t9164-git-svn-dcommit-concurrent.sh setup_hook() · line 66
Quoted 1 line
t/t9164-git-svn-dcommit-concurrent.sh +0
-1- 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
Re: [PATCH v2 3/3] t9164: fix inability to find basename(1) in hooks
Quoted 12 lines · Jeff King
... 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
Quoted 5 lines
...do other stuff...
EOFThat'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.
Re: [PATCH v2 3/3] t9164: fix inability to find basename(1) in hooks
Quoted 20 lines · Junio C Hamano
Quoted 12 lines · Jeff King
... 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 PATHQuoted 5 lines
...do other stuff...
EOFThat'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).
Quoted 6 lines
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]:
Quoted 7 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
Re: [PATCH v2 3/3] t9164: fix inability to find basename(1) in hooks
Quoted 2 lines · Patrick Steinhardt
Quoted 1 line · Junio C Hamano
Jeff King <[email protected]> writes:
[snip]
Quoted 8 lines
Quoted 7 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.
Quoted 4 lines
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
v3 cover t: improve compatibility with NixOS
In reply to Patrick SteinhardtHi,
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 <[email protected]>
## 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.
## 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
Signature · 1 line
2.42.0
PATCH v3 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.
Amend the code to do so. The new runtime-detected paths will only be used in case none of the hardcoded paths are usable.
--- t/lib-httpd.sh | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)
t/lib-httpd.sh +13 -4
@@ -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
Signature · 1 line
2.42.0
Re: [PATCH v3 1/3] t/lib-httpd: dynamically detect httpd and modules path
t/lib-httpd.sh fi · line 63
Quoted 8 lines · Patrick Steinhardt
t/lib-httpd.sh +5
-2-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
Re: [PATCH v3 1/3] t/lib-httpd: dynamically detect httpd and modules path
Quoted 15 lines · Jeff King
t/lib-httpd.sh fi · line 63
Quoted 8 lines · Patrick Steinhardt
t/lib-httpd.sh +5
-2-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.
Quoted 3 lines
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
Quoted 3 lines
if test -n "$DEFAULT_HTTPD_PATH" && test -x "$DEFAULT_HTTPD_PATH"-Peff
Re: [PATCH v3 1/3] t/lib-httpd: dynamically detect httpd and modules path
Quoted 5 lines · Patrick Steinhardt
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.
Re: [PATCH v3 1/3] t/lib-httpd: dynamically detect httpd and modules path
Quoted 11 lines · Junio C Hamano
Quoted 5 lines · Patrick Steinhardt
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?
Quoted 11 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 --
t/lib-httpd.sh +2 -2
@@ -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
Re: [PATCH v3 1/3] t/lib-httpd: dynamically detect httpd and modules path
In reply to Patrick SteinhardtQuoted 8 lines · Patrick Steinhardt
Quoted 7 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:
Quoted 5 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?
I would be very happy with that. :)
-Peff
PATCH v3 2/3 t/lib-httpd: stop using legacy crypt(3) for authentication
In reply to Patrick SteinhardtWhen 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]:
Quoted 3 lines
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
--- t/lib-httpd/passwd | 2 +- t/lib-httpd/proxy-passwd | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
t/lib-httpd/passwd +1 -1
@@ -1 +1 @@ -user@host:xb4E8pqD81KQs +user@host:$apr1$LGPmCZWj$9vxEwj5Z5GzQLBMxp3mCx1
t/lib-httpd/proxy-passwd +1 -1
@@ -1 +1 @@ -proxuser:2x7tAukjAED5M +proxuser:$apr1$RxS6MLkD$DYsqQdflheq4GPNxzJpx5.
Signature · 1 line
2.42.0
PATCH v3 3/3 t9164: fix inability to find basename(1) in Subversion hooks
In reply to Patrick SteinhardtHooks 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.
--- t/t9164-git-svn-dcommit-concurrent.sh | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
t/t9164-git-svn-dcommit-concurrent.sh +8 -1
@@ -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
Signature · 1 line
2.42.0
Re: [PATCH v3 3/3] t9164: fix inability to find basename(1) in Subversion hooks
t/t9164-git-svn-dcommit-concurrent.sh setup_hook() · line 55
Quoted 6 lines · Patrick Steinhardt
t/t9164-git-svn-dcommit-concurrent.sh +6
-0+ # 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
Quoted 8 lines · Patrick Steinhardt
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.
Quoted 8 lines
- Patch 1: We're a bit more defensive and will check whether variables are empty before passing them to either
test -dortest -x.- Patch 3: Clarified why PATH is unset.
- Patch 3: Instead of writing PATH into the hook we now write it into the
hook-envconfiguration file read by Subversion.
Thanks. This looks good to me, modulo a minor shell nit I mentioned in the first patch.
-Peff
v4 cover t: improve compatibility with NixOS
In reply to Patrick SteinhardtHi,
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.
@@ 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
Signature · 1 line
2.42.0
PATCH v4 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.
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.
--- t/lib-httpd.sh | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)
t/lib-httpd.sh +13 -4
@@ -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
Signature · 1 line
2.42.0
Re: [PATCH v4 1/3] t/lib-httpd: dynamically detect httpd and modules path
Review
Quoted 4 lines · Patrick Steinhardt
(quoted diff) +4
-0+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.
t/lib-httpd.sh +1 -1
@@ -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' \
Re: [PATCH v4 1/3] t/lib-httpd: dynamically detect httpd and modules path
Quoted 22 lines · Junio C Hamano
Review
Quoted 4 lines · Patrick Steinhardt
(quoted diff) +4
-0+if test -x "$DEFAULT_HTTPD_PATH" +then + DETECTED_HTTPD_ROOT="$("$DEFAULT_HTTPD_PATH" -V | sed -n 's/^ -D HTTPD_ROOT="\(.*\)"$/\1/p')" +fiWith 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.
t/lib-httpd.sh +0
-0
t/lib-httpd.sh done · line 74
Quoted 8 lines
t/lib-httpd.sh +1
-1@@ -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
PATCH v4 2/3 t/lib-httpd: stop using legacy crypt(3) for authentication
In reply to Patrick SteinhardtWhen 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]:
Quoted 3 lines
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
--- t/lib-httpd/passwd | 2 +- t/lib-httpd/proxy-passwd | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
t/lib-httpd/passwd +1 -1
@@ -1 +1 @@ -user@host:xb4E8pqD81KQs +user@host:$apr1$LGPmCZWj$9vxEwj5Z5GzQLBMxp3mCx1
t/lib-httpd/proxy-passwd +1 -1
@@ -1 +1 @@ -proxuser:2x7tAukjAED5M +proxuser:$apr1$RxS6MLkD$DYsqQdflheq4GPNxzJpx5.
Signature · 1 line
2.42.0
PATCH v4 3/3 t9164: fix inability to find basename(1) in Subversion hooks
In reply to Patrick SteinhardtHooks 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.
--- t/t9164-git-svn-dcommit-concurrent.sh | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
t/t9164-git-svn-dcommit-concurrent.sh +8 -1
@@ -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
Signature · 1 line
2.42.0
Quoted 5 lines · Patrick Steinhardt
this is the fourth version of my patch series to improve compatibility of our test suite with NixOS.
Three changes compared to v3:
Quoted 8 lines
- Switched from
test <expr> -a <expr>totest <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 tonextand caused conflicts.
Thanks, these all look good to me.
-Peff
Quoted 10 lines · Patrick Steinhardt
Three changes compared to v3:
- Switched from
test <expr> -a <expr>totest <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 tonextand 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.
Quoted 3 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.
Quoted 18 lines · Junio C Hamano
Quoted 10 lines · Patrick Steinhardt
Three changes compared to v3:
- Switched from
test <expr> -a <expr>totest <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 tonextand 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.
Quoted 6 lines
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.
Quoted 8 lines
Quoted 3 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
Quoted 3 lines · Patrick Steinhardt
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.