We tried to evade usage of eval in commit 6214ae55f6a, but trying to
use I/O redirections with a command doesn't have the desired effect,
because when Shell eats the redirection it is applied to anything
inside the run_cmd function, even the print command we use for
debugging purposes. In order to print all commands and use the
redirection only on the actual execution of a given command, let's
adopt eval on "$@" and allow passing redirections as strings later on.
Future patches will demonstrate this.

Signed-off-by: Erik Skultety <eskultet@redhat.com>
---
 ci/jobs.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ci/jobs.sh b/ci/jobs.sh
index f4e83dda2e..3a89cb1a69 100644
--- a/ci/jobs.sh
+++ b/ci/jobs.sh
@@ -15,7 +15,7 @@ fi
 GIT_ROOT="$(git rev-parse --show-toplevel)"
 run_cmd() {
     printf "\e[32m[RUN COMMAND]: '%s'\e[0m\n" "$*"
-    "$@"
+    eval "$@"
 }
 
 run_meson_setup() {
@@ -70,7 +70,7 @@ run_rpmbuild() {
     run_cmd rpmbuild \
                 --clean \
                 --nodeps \
-                --define "_without_mingw 1" \
+                --define "'_without_mingw 1'" \
                 -ta build/meson-dist/libvirt-*.tar.xz
 }
 
-- 
2.41.0

On Mon, Sep 18, 2023 at 12:22:45PM +0200, Erik Skultety wrote:
> We tried to evade usage of eval in commit 6214ae55f6a, but trying to
> use I/O redirections with a command doesn't have the desired effect,
> because when Shell eats the redirection it is applied to anything
> inside the run_cmd function, even the print command we use for
> debugging purposes. In order to print all commands and use the
> redirection only on the actual execution of a given command, let's
> adopt eval on "$@" and allow passing redirections as strings later on.
> Future patches will demonstrate this.
> 
> Signed-off-by: Erik Skultety <eskultet@redhat.com>
> ---
>  ci/jobs.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/ci/jobs.sh b/ci/jobs.sh
> index f4e83dda2e..3a89cb1a69 100644
> --- a/ci/jobs.sh
> +++ b/ci/jobs.sh
> @@ -15,7 +15,7 @@ fi
>  GIT_ROOT="$(git rev-parse --show-toplevel)"
>  run_cmd() {
>      printf "\e[32m[RUN COMMAND]: '%s'\e[0m\n" "$*"
> -    "$@"
> +    eval "$@"
>  }

IMHO, we'd be better just creating a "run_cmd_quiet" variant which does

   "$@" 1>/dev/null 2>&1

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On Mon, Sep 18, 2023 at 11:31:53AM +0100, Daniel P. Berrangé wrote:
> On Mon, Sep 18, 2023 at 12:22:45PM +0200, Erik Skultety wrote:
> > We tried to evade usage of eval in commit 6214ae55f6a, but trying to
> > use I/O redirections with a command doesn't have the desired effect,
> > because when Shell eats the redirection it is applied to anything
> > inside the run_cmd function, even the print command we use for
> > debugging purposes. In order to print all commands and use the
> > redirection only on the actual execution of a given command, let's
> > adopt eval on "$@" and allow passing redirections as strings later on.
> > Future patches will demonstrate this.
> > 
> > Signed-off-by: Erik Skultety <eskultet@redhat.com>
> > ---
> >  ci/jobs.sh | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/ci/jobs.sh b/ci/jobs.sh
> > index f4e83dda2e..3a89cb1a69 100644
> > --- a/ci/jobs.sh
> > +++ b/ci/jobs.sh
> > @@ -15,7 +15,7 @@ fi
> >  GIT_ROOT="$(git rev-parse --show-toplevel)"
> >  run_cmd() {
> >      printf "\e[32m[RUN COMMAND]: '%s'\e[0m\n" "$*"
> > -    "$@"
> > +    eval "$@"
> >  }
> 
> IMHO, we'd be better just creating a "run_cmd_quiet" variant which does
> 
>    "$@" 1>/dev/null 2>&1

I don't have a problem with ^this in principle, but eval is more flexible (and
hence more dangerous) in what we can pass as parameters in the future.

Erik

On Mon, Sep 18, 2023 at 01:47:03PM +0200, Erik Skultety wrote:
> On Mon, Sep 18, 2023 at 11:31:53AM +0100, Daniel P. Berrangé wrote:
> > On Mon, Sep 18, 2023 at 12:22:45PM +0200, Erik Skultety wrote:
> > > We tried to evade usage of eval in commit 6214ae55f6a, but trying to
> > > use I/O redirections with a command doesn't have the desired effect,
> > > because when Shell eats the redirection it is applied to anything
> > > inside the run_cmd function, even the print command we use for
> > > debugging purposes. In order to print all commands and use the
> > > redirection only on the actual execution of a given command, let's
> > > adopt eval on "$@" and allow passing redirections as strings later on.
> > > Future patches will demonstrate this.
> > > 
> > > Signed-off-by: Erik Skultety <eskultet@redhat.com>
> > > ---
> > >  ci/jobs.sh | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/ci/jobs.sh b/ci/jobs.sh
> > > index f4e83dda2e..3a89cb1a69 100644
> > > --- a/ci/jobs.sh
> > > +++ b/ci/jobs.sh
> > > @@ -15,7 +15,7 @@ fi
> > >  GIT_ROOT="$(git rev-parse --show-toplevel)"
> > >  run_cmd() {
> > >      printf "\e[32m[RUN COMMAND]: '%s'\e[0m\n" "$*"
> > > -    "$@"
> > > +    eval "$@"
> > >  }
> > 
> > IMHO, we'd be better just creating a "run_cmd_quiet" variant which does
> > 
> >    "$@" 1>/dev/null 2>&1
> 
> I don't have a problem with ^this in principle, but eval is more flexible (and
> hence more dangerous) in what we can pass as parameters in the future.

I really dislike the use of eval because it forces the callers to worry
about nested quoting of parameters, which is one of the worst aspects
of shell.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|