* [kvm-unit-tests PATCH] scripts/runtime: Replace "|&" with "2>&1 |"
@ 2020-07-31 6:09 Thomas Huth
2020-07-31 6:32 ` Andrew Jones
0 siblings, 1 reply; 6+ messages in thread
From: Thomas Huth @ 2020-07-31 6:09 UTC (permalink / raw)
To: kvm; +Cc: pbonzini, drjones
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(-)
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [kvm-unit-tests PATCH] scripts/runtime: Replace "|&" with "2>&1 |"
2020-07-31 6:09 [kvm-unit-tests PATCH] scripts/runtime: Replace "|&" with "2>&1 |" Thomas Huth
@ 2020-07-31 6:32 ` Andrew Jones
2020-07-31 7:13 ` Thomas Huth
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Jones @ 2020-07-31 6:32 UTC (permalink / raw)
To: Thomas Huth; +Cc: kvm, pbonzini
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>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [kvm-unit-tests PATCH] scripts/runtime: Replace "|&" with "2>&1 |"
2020-07-31 6:32 ` Andrew Jones
@ 2020-07-31 7:13 ` Thomas Huth
2020-07-31 7:17 ` Paolo Bonzini
0 siblings, 1 reply; 6+ messages in thread
From: Thomas Huth @ 2020-07-31 7:13 UTC (permalink / raw)
To: Andrew Jones; +Cc: kvm, pbonzini
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [kvm-unit-tests PATCH] scripts/runtime: Replace "|&" with "2>&1 |"
2020-07-31 7:13 ` Thomas Huth
@ 2020-07-31 7:17 ` Paolo Bonzini
2020-07-31 7:45 ` Andrew Jones
0 siblings, 1 reply; 6+ messages in thread
From: Paolo Bonzini @ 2020-07-31 7:17 UTC (permalink / raw)
To: Thomas Huth, Andrew Jones; +Cc: kvm
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [kvm-unit-tests PATCH] scripts/runtime: Replace "|&" with "2>&1 |"
2020-07-31 7:17 ` Paolo Bonzini
@ 2020-07-31 7:45 ` Andrew Jones
2020-07-31 8:07 ` Thomas Huth
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Jones @ 2020-07-31 7:45 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Thomas Huth, kvm
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [kvm-unit-tests PATCH] scripts/runtime: Replace "|&" with "2>&1 |"
2020-07-31 7:45 ` Andrew Jones
@ 2020-07-31 8:07 ` Thomas Huth
0 siblings, 0 replies; 6+ messages in thread
From: Thomas Huth @ 2020-07-31 8:07 UTC (permalink / raw)
To: Andrew Jones, Paolo Bonzini; +Cc: kvm
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-07-31 8:07 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-31 6:09 [kvm-unit-tests PATCH] scripts/runtime: Replace "|&" with "2>&1 |" Thomas Huth
2020-07-31 6:32 ` Andrew Jones
2020-07-31 7:13 ` Thomas Huth
2020-07-31 7:17 ` Paolo Bonzini
2020-07-31 7:45 ` Andrew Jones
2020-07-31 8:07 ` Thomas Huth
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).