[PATCH 05/22] tests/functional: remove duplicated 'which' function impl

Daniel P. Berrangé posted 22 patches 3 weeks, 5 days ago
There is a newer version of this series
[PATCH 05/22] tests/functional: remove duplicated 'which' function impl
Posted by Daniel P. Berrangé 3 weeks, 5 days ago
Put the 'which' function into shared code.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/functional/qemu_test/__init__.py |  2 +-
 tests/functional/qemu_test/cmd.py      | 10 ++++++++++
 tests/functional/test_acpi_bits.py     | 13 +------------
 tests/functional/test_ppc64_hv.py      | 13 +------------
 4 files changed, 13 insertions(+), 25 deletions(-)

diff --git a/tests/functional/qemu_test/__init__.py b/tests/functional/qemu_test/__init__.py
index 67f87be9c4..8fddddbe67 100644
--- a/tests/functional/qemu_test/__init__.py
+++ b/tests/functional/qemu_test/__init__.py
@@ -10,6 +10,6 @@
 from .config import BUILD_DIR
 from .cmd import has_cmd, has_cmds, run_cmd, is_readable_executable_file, \
     interrupt_interactive_console_until_pattern, wait_for_console_pattern, \
-    exec_command, exec_command_and_wait_for_pattern, get_qemu_img
+    exec_command, exec_command_and_wait_for_pattern, get_qemu_img, which
 from .testcase import QemuBaseTest, QemuUserTest, QemuSystemTest
 from .linuxkernel import LinuxKernelTest
diff --git a/tests/functional/qemu_test/cmd.py b/tests/functional/qemu_test/cmd.py
index 11c8334a7c..4106f1ee7c 100644
--- a/tests/functional/qemu_test/cmd.py
+++ b/tests/functional/qemu_test/cmd.py
@@ -18,6 +18,16 @@
 
 from .config import BUILD_DIR
 
+def which(tool):
+    """ looks up the full path for @tool, returns None if not found
+        or if @tool does not have executable permissions.
+    """
+    paths=os.getenv('PATH')
+    for p in paths.split(os.path.pathsep):
+        p = os.path.join(p, tool)
+        if os.path.exists(p) and os.access(p, os.X_OK):
+            return p
+    return None
 
 def has_cmd(name, args=None):
     """
diff --git a/tests/functional/test_acpi_bits.py b/tests/functional/test_acpi_bits.py
index 6fc49a30bc..4162faf414 100755
--- a/tests/functional/test_acpi_bits.py
+++ b/tests/functional/test_acpi_bits.py
@@ -49,7 +49,7 @@
 )
 from qemu.machine import QEMUMachine
 from unittest import skipIf
-from qemu_test import QemuBaseTest, Asset
+from qemu_test import QemuBaseTest, Asset, which
 
 deps = ["xorriso", "mformat"] # dependent tools needed in the test setup/box.
 supported_platforms = ['x86_64'] # supported test platforms.
@@ -57,17 +57,6 @@
 # default timeout of 120 secs is sometimes not enough for bits test.
 BITS_TIMEOUT = 200
 
-def which(tool):
-    """ looks up the full path for @tool, returns None if not found
-        or if @tool does not have executable permissions.
-    """
-    paths=os.getenv('PATH')
-    for p in paths.split(os.path.pathsep):
-        p = os.path.join(p, tool)
-        if os.path.exists(p) and os.access(p, os.X_OK):
-            return p
-    return None
-
 def missing_deps():
     """ returns True if any of the test dependent tools are absent.
     """
diff --git a/tests/functional/test_ppc64_hv.py b/tests/functional/test_ppc64_hv.py
index 53bae90401..cc77cd22b1 100755
--- a/tests/functional/test_ppc64_hv.py
+++ b/tests/functional/test_ppc64_hv.py
@@ -11,7 +11,7 @@
 
 from unittest import skipIf, skipUnless
 from qemu_test import (QemuSystemTest, Asset, wait_for_console_pattern,
-                       exec_command)
+                       exec_command, which)
 import os
 import time
 import subprocess
@@ -19,17 +19,6 @@
 
 deps = ["xorriso"] # dependent tools needed in the test setup/box.
 
-def which(tool):
-    """ looks up the full path for @tool, returns None if not found
-        or if @tool does not have executable permissions.
-    """
-    paths=os.getenv('PATH')
-    for p in paths.split(os.path.pathsep):
-        p = os.path.join(p, tool)
-        if os.path.exists(p) and os.access(p, os.X_OK):
-            return p
-    return None
-
 def missing_deps():
     """ returns True if any of the test dependent tools are absent.
     """
-- 
2.46.0


Re: [PATCH 05/22] tests/functional: remove duplicated 'which' function impl
Posted by Richard Henderson 3 weeks, 4 days ago
On 11/29/24 11:31, Daniel P. Berrangé wrote:
> Put the 'which' function into shared code.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   tests/functional/qemu_test/__init__.py |  2 +-
>   tests/functional/qemu_test/cmd.py      | 10 ++++++++++
>   tests/functional/test_acpi_bits.py     | 13 +------------
>   tests/functional/test_ppc64_hv.py      | 13 +------------
>   4 files changed, 13 insertions(+), 25 deletions(-)

As code movement,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

> +def which(tool):
> +    """ looks up the full path for @tool, returns None if not found
> +        or if @tool does not have executable permissions.
> +    """
> +    paths=os.getenv('PATH')
> +    for p in paths.split(os.path.pathsep):
> +        p = os.path.join(p, tool)
> +        if os.path.exists(p) and os.access(p, os.X_OK):

But surely exists() is redundant with access()?


r~

Re: [PATCH 05/22] tests/functional: remove duplicated 'which' function impl
Posted by Daniel P. Berrangé 3 weeks, 3 days ago
On Sat, Nov 30, 2024 at 09:08:21AM -0600, Richard Henderson wrote:
> On 11/29/24 11:31, Daniel P. Berrangé wrote:
> > Put the 'which' function into shared code.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >   tests/functional/qemu_test/__init__.py |  2 +-
> >   tests/functional/qemu_test/cmd.py      | 10 ++++++++++
> >   tests/functional/test_acpi_bits.py     | 13 +------------
> >   tests/functional/test_ppc64_hv.py      | 13 +------------
> >   4 files changed, 13 insertions(+), 25 deletions(-)
> 
> As code movement,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> > +def which(tool):
> > +    """ looks up the full path for @tool, returns None if not found
> > +        or if @tool does not have executable permissions.
> > +    """
> > +    paths=os.getenv('PATH')
> > +    for p in paths.split(os.path.pathsep):
> > +        p = os.path.join(p, tool)
> > +        if os.path.exists(p) and os.access(p, os.X_OK):
> 
> But surely exists() is redundant with access()?

Agreed, will simplify that.

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 :|


Re: [PATCH 05/22] tests/functional: remove duplicated 'which' function impl
Posted by Thomas Huth 3 weeks, 5 days ago
On 29/11/2024 18.31, Daniel P. Berrangé wrote:
> Put the 'which' function into shared code.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   tests/functional/qemu_test/__init__.py |  2 +-
>   tests/functional/qemu_test/cmd.py      | 10 ++++++++++
>   tests/functional/test_acpi_bits.py     | 13 +------------
>   tests/functional/test_ppc64_hv.py      | 13 +------------
>   4 files changed, 13 insertions(+), 25 deletions(-)

None of the callers really seem to be interested in the location of the 
command, only whether it is available in the $PATH or not ... so could we 
maybe rather drop this function and use the has_cmd() function everywhere 
instead?

Hmm, thinking about it twice - has_cmd() uses the "which" program 
internally, but AFAIK this program is optional in Linux installations 
nowadays ... so maybe it's still a good idea to move our Python which() to 
cmd.py, but has_cmd() should maybe rather be changed to use it, too?

  Thomas



Re: [PATCH 05/22] tests/functional: remove duplicated 'which' function impl
Posted by Daniel P. Berrangé 3 weeks, 3 days ago
On Sat, Nov 30, 2024 at 11:16:30AM +0100, Thomas Huth wrote:
> On 29/11/2024 18.31, Daniel P. Berrangé wrote:
> > Put the 'which' function into shared code.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >   tests/functional/qemu_test/__init__.py |  2 +-
> >   tests/functional/qemu_test/cmd.py      | 10 ++++++++++
> >   tests/functional/test_acpi_bits.py     | 13 +------------
> >   tests/functional/test_ppc64_hv.py      | 13 +------------
> >   4 files changed, 13 insertions(+), 25 deletions(-)
> 
> None of the callers really seem to be interested in the location of the
> command, only whether it is available in the $PATH or not ... so could we
> maybe rather drop this function and use the has_cmd() function everywhere
> instead?
> 
> Hmm, thinking about it twice - has_cmd() uses the "which" program
> internally, but AFAIK this program is optional in Linux installations
> nowadays ... so maybe it's still a good idea to move our Python which() to
> cmd.py, but has_cmd() should maybe rather be changed to use it, too?

This reminds me I meant to ask about 'has_cmd' - it looks rather
over-engineered to me to be trying to invoke the command with
args.

Perhaps there was some reason to check support for individual
args in the past, but none of the current tests need that AFAICT.

So if anything I'd be looking to delete 'has_cmd' and 'has_cmds'
entirely, and rely only on the pure python 'which'.

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 :|


Re: [PATCH 05/22] tests/functional: remove duplicated 'which' function impl
Posted by Thomas Huth 3 weeks, 2 days ago
On 02/12/2024 12.44, Daniel P. Berrangé wrote:
> On Sat, Nov 30, 2024 at 11:16:30AM +0100, Thomas Huth wrote:
>> On 29/11/2024 18.31, Daniel P. Berrangé wrote:
>>> Put the 'which' function into shared code.
>>>
>>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>>> ---
>>>    tests/functional/qemu_test/__init__.py |  2 +-
>>>    tests/functional/qemu_test/cmd.py      | 10 ++++++++++
>>>    tests/functional/test_acpi_bits.py     | 13 +------------
>>>    tests/functional/test_ppc64_hv.py      | 13 +------------
>>>    4 files changed, 13 insertions(+), 25 deletions(-)
>>
>> None of the callers really seem to be interested in the location of the
>> command, only whether it is available in the $PATH or not ... so could we
>> maybe rather drop this function and use the has_cmd() function everywhere
>> instead?
>>
>> Hmm, thinking about it twice - has_cmd() uses the "which" program
>> internally, but AFAIK this program is optional in Linux installations
>> nowadays ... so maybe it's still a good idea to move our Python which() to
>> cmd.py, but has_cmd() should maybe rather be changed to use it, too?
> 
> This reminds me I meant to ask about 'has_cmd' - it looks rather
> over-engineered to me to be trying to invoke the command with
> args.
> 
> Perhaps there was some reason to check support for individual
> args in the past, but none of the current tests need that AFAICT.
> 
> So if anything I'd be looking to delete 'has_cmd' and 'has_cmds'
> entirely, and rely only on the pure python 'which'.

Deleting has_cmd is fine for me, too! Especially since this function already 
caused confusion in the past (see 
https://gitlab.com/qemu-project/qemu/-/commit/59d100243d23451e66d2274d34edab7be6dab473 
).

  Thomas