Re: [PATCH 1/6] tests/qemu-iotests: Improve the check for GNU sed

[Philippe Mathieu-Daudé]

On 8/2/22 13:38, Thomas Huth wrote:
> On 08/02/2022 13.28, Hanna Reitz wrote:
> > On 08.02.22 13:13, Thomas Huth wrote:
> > > On 08/02/2022 12.46, Hanna Reitz wrote:
> > > > On 08.02.22 11:13, Thomas Huth wrote:
> > > > > Instead of failing the iotests if GNU sed is not available (or 
> > > > > skipping
> > > > > them completely in the check-block.sh script), it would be better to
> > > > > simply skip the bash-based tests, so that the python-based tests could
> > > > > still be run. Thus add the check for BusyBox sed to common.rc and mark
> > > > > the tests as "not run" if GNU sed is not available. Then we can also
> > > > > remove the sed checks from the check-block.sh script.
> > > > >
> > > > > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > > > > ---
> > > > >   tests/check-block.sh         | 12 ------------
> > > > >   tests/qemu-iotests/common.rc | 26 +++++++++++++-------------
> > > > >   2 files changed, 13 insertions(+), 25 deletions(-)
> > > > >
> > > > > diff --git a/tests/check-block.sh b/tests/check-block.sh
> > > > > index 720a46bc36..af0c574812 100755
> > > > > --- a/tests/check-block.sh
> > > > > +++ b/tests/check-block.sh
> > > > > @@ -52,18 +52,6 @@ if LANG=C bash --version | grep -q 'GNU bash, 
> > > > > version [123]' ; then
> > > > >       skip "bash version too old ==> Not running the qemu-iotests."
> > > > >   fi
> > > > > -if ! (sed --version | grep 'GNU sed') > /dev/null 2>&1 ; then
> > > >
> > > > This specifically tests for `sed`, whereas...
> > >
> > > There was a check for "gsed" one line later:
> > >
> > >  if ! command -v gsed >/dev/null 2>&1; then
> > >
> > > ... so the check-block.sh script ran the iotests also if "sed" was 
> > > not GNU, but gsed was available.
> >
> > Oh, right.
> >
> > > > [...]
> > > >
> > > > > diff --git a/tests/qemu-iotests/common.rc 
> > > > > b/tests/qemu-iotests/common.rc
> > > > > index 9885030b43..9ea504810c 100644
> > > > > --- a/tests/qemu-iotests/common.rc
> > > > > +++ b/tests/qemu-iotests/common.rc
> > > > > @@ -17,17 +17,27 @@
> > > > >   # along with this program.  If not, see 
> > > > > <http://www.gnu.org/licenses/>.
> > > > >   #
> > > > > +# bail out, setting up .notrun file
> > > > > +_notrun()
> > > > > +{
> > > > > +    echo "$*" >"$OUTPUT_DIR/$seq.notrun"
> > > > > +    echo "$seq not run: $*"
> > > > > +    status=0
> > > > > +    exit
> > > > > +}
> > > > > +
> > > > > +# We need GNU sed for the iotests. Make sure to not use BusyBox sed
> > > > > +# which says that "This is not GNU sed version 4.0"
> > > > >   SED=
> > > > >   for sed in sed gsed; do
> > > > > -    ($sed --version | grep 'GNU sed') > /dev/null 2>&1
> > > > > +    ($sed --version | grep -v "not GNU sed" | grep 'GNU sed') > 
> > > > > /dev/null 2>&1
> > > >
> > > > ...this will accept `gsed`, too.  The problem is that many bash 
> > > > iotests just use `sed` instead of `$SED`, so I think if we let this 
> > > > do the gatekeeping, then we should change this to just check for `sed`.
> > >
> > > I think we should be fine - at least for the tests in the "auto" 
> > > group. Otherwise we would have seen test failures on non-Linux 
> > > systems like *BSD earlier already.
> >
> > Makes sense, but I’m quite uncomfortable with this.
>
> The current code with $SED has been introduced almost three years ago 
> already...
>
> >   Can’t we just do `alias sed=gsed`?
>
> Maybe ... but let's ask Philippe and Kevin first, who Signed-off commit 
> bde36af1ab4f476 that introduced the current way with $SED: What's your 
> opinion about this?

This commit was to have check-block working on the OpenBSD VM image.