scripts/runtime: Replace "|&" with "2>&1 |"
PATCH 1/1 scripts/runtime: Replace "|&" with "2>&1 |"
The "|&" only works with newer versions of the bash. For compatibility with older versions, we should use "2>&1 |" instead.
--- scripts/runtime.bash | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
scripts/runtime.bash +2 -2
@@ -172,7 +172,7 @@ function run() # "arm/arm64: KVM: Remove 'config KVM_ARM_MAX_VCPUS'". So, at some # point when maintaining the while loop gets too tiresome, we can # just remove it... -while $RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP \ - |& grep -qi 'exceeds max CPUs'; do +while $RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP 2>&1 \ + | grep -qi 'exceeds max CPUs'; do MAX_SMP=$((MAX_SMP >> 1)) done
Signature · 1 line
2.18.1
Quoted 2 lines · Thomas Huth
The "|&" only works with newer versions of the bash. For compatibility with older versions, we should use "2>&1 |" instead.
Hi Thomas,
Which bash version are you targeting with this change?
I think it's time we pick a bash version that we want to support (thoroughly test all the scripts with it) and then document it. As part of the CI we should test with both that version and with the latest released version (394d1421 ("run_migration: Implement our own wait") is an example of why only testing with our supported version wouldn't be sufficient, unless we required everyone to use that version when running the tests, and I don't want to do that.)
Quoted 23 lines
Signed-off-by Thomas Huth <[email protected]>--- scripts/runtime.bash | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)scripts/runtime.bash +2
-2@@ -172,7 +172,7 @@ function run() # "arm/arm64: KVM: Remove 'config KVM_ARM_MAX_VCPUS'". So, at some # point when maintaining the while loop gets too tiresome, we can # just remove it... -while $RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP \ - |& grep -qi 'exceeds max CPUs'; do +while $RUNTIME_arch_run _NO_FILE_4Uhere_ -smp $MAX_SMP 2>&1 \ + | grep -qi 'exceeds max CPUs'; do MAX_SMP=$((MAX_SMP >> 1)) doneSignature · 1 line
2.18.1
Anyway
Quoted 7 lines · Andrew Jones
Quoted 2 lines · Thomas Huth
The "|&" only works with newer versions of the bash. For compatibility with older versions, we should use "2>&1 |" instead.
Hi Thomas,
Which bash version are you targeting with this change?
I played a little bit with the macOS containers on Travis, to see whether we could easily get some CI coverage for that after commit 7edd698ed003e introduced hvf support... and the bash version that Apple ships is incredibly old (version 3). But it seems to break in some other spots, too, so I think it likely makes more sense to install a newer version of the bash with homebrew there...
Quoted 2 lines
I think it's time we pick a bash version that we want to support (thoroughly test all the scripts with it) and then document it.
Makes sense. Version 3 is definitely too old ;-)
Quoted 3 lines
As
part of the CI we should test with both that version and with the latest released version
I think we should already have a good test coverage for newer versions by using the latest version of Fedora in the CI.
For old versions, we could add a CI job based on CentOS 7 maybe? That would be Bash v4.2 if I got that right...
Thomas
Paolo Bonzini
maintainerQuoted 1 line · Thomas Huth
the bash version that Apple ships is incredibly old (version 3).
Yes, due to GPLv3. :( I think either we rewrite the whole thing in Python (except for the "shar"-like code in mkstandalone.sh) or we keep bash 4 as the minimum supported version.
Paolo
Quoted 5 lines · Paolo Bonzini
Quoted 1 line · Thomas Huth
the bash version that Apple ships is incredibly old (version 3).
Yes, due to GPLv3. :( I think either we rewrite the whole thing in Python (except for the "shar"-like code in
mkstandalone.sh)
I once suggested Python (or anything less awkward than Bash) be used for our harness, but ARM people told me that they like Bash because then they can install the unit tests on minimal images that they use on the ARM models. There may other "embedded" cases for kvm-unit-tests in the future too, if we were to introduce bare-metal targets, etc., so I think the minimal language (Bash) requirement makes sense to maintain (not to mention we already wrote it...)
Quoted 2 lines
or we keep
bash 4 as the minimum supported version.
Is 4.2 OK? That would allow Thomas' CI to get the coverage we need by using CentOS, without having to install a specific Bash.
Thanks,
drew
Quoted 15 lines · Andrew Jones
Quoted 5 lines · Paolo Bonzini
Quoted 1 line · Thomas Huth
the bash version that Apple ships is incredibly old (version 3).
Yes, due to GPLv3. :( I think either we rewrite the whole thing in Python (except for the "shar"-like code in
mkstandalone.sh)I once suggested Python (or anything less awkward than Bash) be used for our harness, but ARM people told me that they like Bash because then they can install the unit tests on minimal images that they use on the ARM models. There may other "embedded" cases for kvm-unit-tests in the future too, if we were to introduce bare-metal targets, etc., so I think the minimal language (Bash) requirement makes sense to maintain (not to mention we already wrote it...)
Quoted 5 lines
Quoted 2 lines
or we keep
bash 4 as the minimum supported version.Is 4.2 OK? That would allow Thomas' CI to get the coverage we need by using CentOS, without having to install a specific Bash.
Bash v4.2 has been released in February 2011, so that's more than 9 years already. I don't think that we should support any older version than this.
Thomas