On Fri, Apr 20, 2018 at 04:59:58PM +0000, Peter Shier wrote:
> I have run the whole suite on 4.1 but not sure if everything in this script
> runs in that case. New patching coming right after this. Thx.
As long as you've also done a 'configure && make standalone' and run the
generated tests with bash 4.1, then I think you've exercised the majority
of the code. We'll still be missing code paths as some of them are
dependent on test return status or whether or not a signal is received.
Also powerpc uses a couple extra functions, namely run_qemu_status() and
run_migration().
Oh wait[*], you should also run with 'run_tests.sh -j <some-number>' to
test the parallel execution support, which I now see won't work with Bash
older than 4.3, as it's using the '-n' option for 'wait'[**]. We can
rework that or just special case it, i.e. do a Bash version check when
'-j' is enabled and then say it's not supported when we detect old Bash.
With that, I think we can have reasonable confidence that we're 4.1
compliant and document it. I'll try to find time to write test cases
to ensure full coverage and run them with Bash 4.1 as well.
Thanks,
drew
[*] Pun intended
[**] Now you see the pun, right? I hope you're laughing.
> On Fri, Apr 20, 2018 at 12:35 AM Andrew Jones <drjones@xxxxxxxxxx> wrote:
>
> > On Fri, Apr 20, 2018 at 12:06:30AM +0000, Peter Shier wrote:
> > > Thanks very much for the quick review. I indeed had a bug there losing
> the
> > > check for unset ERRATA_$commit.
> > > We do require Bash 4.1 and later compatibility. I would like to change
> the
> > > test -v lines to use indirection:
> > >
> > > [[ -n "${!errata}" ]] && continue
> > >
> > > I tested this on 4.1 and on 4.4. It treats unset and "" as the same
> which
> > > should be sufficient here AFAICT (while maintaining compatibility).
> > > I have a new patch ready to go. Please let me know if this works for
> you.
> > >
>
> > Yeah, that's fine. One other comment on your patch though is that you
> > were only changing the 'test -v' in env_generate_errata(). There's
> > another one in env_add_errata() that should also be changed.
>
> > If you can do exhaustive testing or some analysis of the bash code to
> > ensure it's 4.1 compatible, then we can now document that requirement
> > in the README as well. I wouldn't want to document it until we're
> > sure though.
>
> > Thanks,
> > drew
