| Message ID | 20200731060909.1163-1-thuth@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-tests] scripts/runtime: Replace "|&" with "2>&1 |" | expand |
On Fri, Jul 31, 2020 at 08:09:09AM +0200, Thomas Huth wrote: > 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.) > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > scripts/runtime.bash | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/scripts/runtime.bash b/scripts/runtime.bash > index c88e246..35689a7 100644 > --- a/scripts/runtime.bash > +++ b/scripts/runtime.bash > @@ -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 > -- > 2.18.1 > Anyway Reviewed-by: Andrew Jones <drjones@redhat.com>
On 31/07/2020 08.32, Andrew Jones wrote: > On Fri, Jul 31, 2020 at 08:09:09AM +0200, Thomas Huth wrote: >> 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... > 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 ;-) > 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
On 31/07/20 09:13, Thomas Huth wrote:
> 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
On Fri, Jul 31, 2020 at 09:17:53AM +0200, Paolo Bonzini wrote: > On 31/07/20 09:13, Thomas Huth wrote: > > 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...) > 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
On 31/07/2020 09.45, Andrew Jones wrote: > On Fri, Jul 31, 2020 at 09:17:53AM +0200, Paolo Bonzini wrote: >> On 31/07/20 09:13, Thomas Huth wrote: >>> 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...) > >> 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
diff --git a/scripts/runtime.bash b/scripts/runtime.bash index c88e246..35689a7 100644 --- a/scripts/runtime.bash +++ b/scripts/runtime.bash @@ -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
The "|&" only works with newer versions of the bash. For compatibility with older versions, we should use "2>&1 |" instead. Signed-off-by: Thomas Huth <thuth@redhat.com> --- scripts/runtime.bash | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)