[Qemu-devel] [PATCH] Acceptance tests: host arch to target arch name mapping

[Qemu-devel] [PATCH] Acceptance tests: host arch to target arch name mapping

[Cleber Rosa]

The host arch name is not always the target arch name, so it's
necessary to have a mapping.

The configure scripts contains what is the authoritative and failproof
mapping, but, reusing it is not straightforward, so it's replicated in
the acceptance tests supporting code.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 configure                                 |  2 ++
 tests/acceptance/avocado_qemu/__init__.py | 23 +++++++++++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/configure b/configure
index 8af2be959f..e029b756d4 100755
--- a/configure
+++ b/configure
@@ -6992,6 +6992,8 @@ TARGET_ARCH="$target_name"
 TARGET_BASE_ARCH=""
 TARGET_ABI_DIR=""
 
+# When updating target_name => TARGET_ARCH, please also update the
+# HOST_TARGET_ARCH mapping in tests/acceptance/avocado_qemu/__init__.py
 case "$target_name" in
   i386)
     mttcg="yes"
diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
index 1e54fd5932..d9bc4736ec 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -19,6 +19,28 @@ sys.path.append(os.path.join(SRC_ROOT_DIR, 'scripts'))
 
 from qemu import QEMUMachine
 
+
+#: Mapping of host arch names to target arch names.  It's expected that the
+#: arch identification on the host, using os.uname()[4], would return the
+#: key (LHS).  The QEMU target name, and consequently the target binary, would
+#: be based on the name on the value (RHS).
+HOST_TARGET_ARCH = {
+    'armeb': 'arm',
+    'aarch64_be': 'aarch64',
+    'microblazeel': 'microblaze',
+    'mipsel': 'mips',
+    'mipsn32el' : 'mips64',
+    'mips64el': 'mips64',
+    'or1k': 'openrisc',
+    'ppc64le': 'ppc64',
+    'ppc64abi32': 'ppc64',
+    'riscv64': 'riscv',
+    'sh4eb': 'sh4',
+    'sparc32plus': 'sparc64',
+    'xtensaeb': 'xtensa'
+    }
+
+
 def is_readable_executable_file(path):
     return os.path.isfile(path) and os.access(path, os.R_OK | os.X_OK)
 
@@ -29,6 +51,7 @@ def pick_default_qemu_bin():
     directory or in the source tree root directory.
     """
     arch = os.uname()[4]
+    arch = HOST_TARGET_ARCH.get(arch, arch)
     qemu_bin_relative_path = os.path.join("%s-softmmu" % arch,
                                           "qemu-system-%s" % arch)
     if is_readable_executable_file(qemu_bin_relative_path):
-- 
2.17.1

Re: [Qemu-devel] [PATCH] Acceptance tests: host arch to target arch name mapping

[Philippe Mathieu-Daudé]

Hi Cleber,

On 17/10/2018 01:22, Cleber Rosa wrote:
> The host arch name is not always the target arch name, so it's
> necessary to have a mapping.
> 
> The configure scripts contains what is the authoritative and failproof
> mapping, but, reusing it is not straightforward, so it's replicated in
> the acceptance tests supporting code.
> 
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>  configure                                 |  2 ++
>  tests/acceptance/avocado_qemu/__init__.py | 23 +++++++++++++++++++++++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/configure b/configure
> index 8af2be959f..e029b756d4 100755
> --- a/configure
> +++ b/configure
> @@ -6992,6 +6992,8 @@ TARGET_ARCH="$target_name"
>  TARGET_BASE_ARCH=""
>  TARGET_ABI_DIR=""
>  
> +# When updating target_name => TARGET_ARCH, please also update the
> +# HOST_TARGET_ARCH mapping in tests/acceptance/avocado_qemu/__init__.py
>  case "$target_name" in
>    i386)
>      mttcg="yes"
> diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
> index 1e54fd5932..d9bc4736ec 100644
> --- a/tests/acceptance/avocado_qemu/__init__.py
> +++ b/tests/acceptance/avocado_qemu/__init__.py

I'd put this in scripts/qemu.py

> @@ -19,6 +19,28 @@ sys.path.append(os.path.join(SRC_ROOT_DIR, 'scripts'))
>  
>  from qemu import QEMUMachine
>  
> +
> +#: Mapping of host arch names to target arch names.  It's expected that the
> +#: arch identification on the host, using os.uname()[4], would return the
> +#: key (LHS).  The QEMU target name, and consequently the target binary, would
> +#: be based on the name on the value (RHS).
> +HOST_TARGET_ARCH = {
> +    'armeb': 'arm',
> +    'aarch64_be': 'aarch64',

Since you add this, I'd start directly with an exhaustive list:

       'aarch64_be': {'arch': 'aarch64', 'endian': 'big', 'wordsize':
64, 'abi' = 'arm'},

> +    'microblazeel': 'microblaze',
> +    'mipsel': 'mips',
> +    'mipsn32el' : 'mips64',
> +    'mips64el': 'mips64',
> +    'or1k': 'openrisc',
> +    'ppc64le': 'ppc64',
> +    'ppc64abi32': 'ppc64',
> +    'riscv64': 'riscv',
> +    'sh4eb': 'sh4',
> +    'sparc32plus': 'sparc64',
> +    'xtensaeb': 'xtensa'
> +    }

Then a function such:

def target_normalize(key, arch):
    return table[arch][key]

> +
> +
>  def is_readable_executable_file(path):
>      return os.path.isfile(path) and os.access(path, os.R_OK | os.X_OK)
>  
> @@ -29,6 +51,7 @@ def pick_default_qemu_bin():
>      directory or in the source tree root directory.
>      """
>      arch = os.uname()[4]
> +    arch = HOST_TARGET_ARCH.get(arch, arch)

       arch = target_normalize('arch', os.uname()[4])

>      qemu_bin_relative_path = os.path.join("%s-softmmu" % arch,
>                                            "qemu-system-%s" % arch)
>      if is_readable_executable_file(qemu_bin_relative_path):
> 

What do you think?

Thanks,

Phil.

Re: [Qemu-devel] [PATCH] Acceptance tests: host arch to target arch name mapping

[Cleber Rosa]

On 10/17/18 6:09 AM, Philippe Mathieu-Daudé wrote:
> Hi Cleber,
> 
> On 17/10/2018 01:22, Cleber Rosa wrote:
>> The host arch name is not always the target arch name, so it's
>> necessary to have a mapping.
>>
>> The configure scripts contains what is the authoritative and failproof
>> mapping, but, reusing it is not straightforward, so it's replicated in
>> the acceptance tests supporting code.
>>
>> Signed-off-by: Cleber Rosa <crosa@redhat.com>
>> ---
>>  configure                                 |  2 ++
>>  tests/acceptance/avocado_qemu/__init__.py | 23 +++++++++++++++++++++++
>>  2 files changed, 25 insertions(+)
>>
>> diff --git a/configure b/configure
>> index 8af2be959f..e029b756d4 100755
>> --- a/configure
>> +++ b/configure
>> @@ -6992,6 +6992,8 @@ TARGET_ARCH="$target_name"
>>  TARGET_BASE_ARCH=""
>>  TARGET_ABI_DIR=""
>>  
>> +# When updating target_name => TARGET_ARCH, please also update the
>> +# HOST_TARGET_ARCH mapping in tests/acceptance/avocado_qemu/__init__.py
>>  case "$target_name" in
>>    i386)
>>      mttcg="yes"
>> diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
>> index 1e54fd5932..d9bc4736ec 100644
>> --- a/tests/acceptance/avocado_qemu/__init__.py
>> +++ b/tests/acceptance/avocado_qemu/__init__.py
> 
> I'd put this in scripts/qemu.py
> 

I chose avocado_qemu/__init__.py because this mapping is currently of no
use to scripts/qemu.py.

But, I'm fine with moving it to scripts/qemu.py if most people agree.

>> @@ -19,6 +19,28 @@ sys.path.append(os.path.join(SRC_ROOT_DIR, 'scripts'))
>>  
>>  from qemu import QEMUMachine
>>  
>> +
>> +#: Mapping of host arch names to target arch names.  It's expected that the
>> +#: arch identification on the host, using os.uname()[4], would return the
>> +#: key (LHS).  The QEMU target name, and consequently the target binary, would
>> +#: be based on the name on the value (RHS).
>> +HOST_TARGET_ARCH = {
>> +    'armeb': 'arm',
>> +    'aarch64_be': 'aarch64',
> 
> Since you add this, I'd start directly with an exhaustive list:
> 
>        'aarch64_be': {'arch': 'aarch64', 'endian': 'big', 'wordsize':
> 64, 'abi' = 'arm'},
> 

Forgive my lack of QEMU culture, but the modus operandi I've been
following is to start with the simplest possible approach, as
justifiable by an existing requirement.

TBH, I lack knowledge about an use case for these other fields.

>> +    'microblazeel': 'microblaze',
>> +    'mipsel': 'mips',
>> +    'mipsn32el' : 'mips64',
>> +    'mips64el': 'mips64',
>> +    'or1k': 'openrisc',
>> +    'ppc64le': 'ppc64',
>> +    'ppc64abi32': 'ppc64',
>> +    'riscv64': 'riscv',
>> +    'sh4eb': 'sh4',
>> +    'sparc32plus': 'sparc64',
>> +    'xtensaeb': 'xtensa'
>> +    }
> 
> Then a function such:
> 
> def target_normalize(key, arch):
>     return table[arch][key]
> 
>> +
>> +
>>  def is_readable_executable_file(path):
>>      return os.path.isfile(path) and os.access(path, os.R_OK | os.X_OK)
>>  
>> @@ -29,6 +51,7 @@ def pick_default_qemu_bin():
>>      directory or in the source tree root directory.
>>      """
>>      arch = os.uname()[4]
>> +    arch = HOST_TARGET_ARCH.get(arch, arch)
> 
>        arch = target_normalize('arch', os.uname()[4])
> 
>>      qemu_bin_relative_path = os.path.join("%s-softmmu" % arch,
>>                                            "qemu-system-%s" % arch)
>>      if is_readable_executable_file(qemu_bin_relative_path):
>>
> 
> What do you think?
> 

Python dictionaries are so rich that can and are encouraged to be used
in place of such functions and even "switch/case" constructs.  I would
add such a function only if there's extra logic no present in a dict.

Since we're more or less building a coding style here, it'd be useful to
hear what other people think.

Thanks for the review!
- Cleber.

> Thanks,
> 
> Phil.
> 

-- 
Cleber Rosa
[ Sr Software Engineer - Virtualization Team - Red Hat ]
[ Avocado Test Framework - avocado-framework.github.io ]
[  7ABB 96EB 8B46 B94D 5E0F  E9BB 657E 8D33 A5F2 09F3  ]

Re: [Qemu-devel] [PATCH] Acceptance tests: host arch to target arch name mapping

[Peter Maydell]

On 17 October 2018 at 00:22, Cleber Rosa <crosa@redhat.com> wrote:
> The host arch name is not always the target arch name, so it's
> necessary to have a mapping.
>
> The configure scripts contains what is the authoritative and failproof
> mapping, but, reusing it is not straightforward, so it's replicated in
> the acceptance tests supporting code.

So, why does the test code need to care? It's not clear
from the patch... My expectation would be that you'd
just test all the testable target architectures,
regardless of what the host architecture is.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] Acceptance tests: host arch to target arch name mapping

[Eduardo Habkost]

On Wed, Oct 17, 2018 at 01:34:41PM +0100, Peter Maydell wrote:
> On 17 October 2018 at 00:22, Cleber Rosa <crosa@redhat.com> wrote:
> > The host arch name is not always the target arch name, so it's
> > necessary to have a mapping.
> >
> > The configure scripts contains what is the authoritative and failproof
> > mapping, but, reusing it is not straightforward, so it's replicated in
> > the acceptance tests supporting code.
> 
> So, why does the test code need to care? It's not clear
> from the patch... My expectation would be that you'd
> just test all the testable target architectures,
> regardless of what the host architecture is.

I tend to agree.  Maybe the right solution is to get rid of the
os.uname().  I think the default should be testing all QEMU
binaries that were built, and the host architecture shouldn't
matter.

That said, I think the dictionary is a nice temporary workaround
until we fix that.

-- 
Eduardo

Re: [Qemu-devel] [PATCH] Acceptance tests: host arch to target arch name mapping

[Cleber Rosa]

On 10/17/18 12:29 PM, Eduardo Habkost wrote:
> On Wed, Oct 17, 2018 at 01:34:41PM +0100, Peter Maydell wrote:
>> On 17 October 2018 at 00:22, Cleber Rosa <crosa@redhat.com> wrote:
>>> The host arch name is not always the target arch name, so it's
>>> necessary to have a mapping.
>>>
>>> The configure scripts contains what is the authoritative and failproof
>>> mapping, but, reusing it is not straightforward, so it's replicated in
>>> the acceptance tests supporting code.
>>
>> So, why does the test code need to care? It's not clear
>> from the patch... My expectation would be that you'd
>> just test all the testable target architectures,
>> regardless of what the host architecture is.
> 
> I tend to agree.  Maybe the right solution is to get rid of the
> os.uname().  I think the default should be testing all QEMU
> binaries that were built, and the host architecture shouldn't
> matter.
> 

I'm in favor of exercising all built targets, but that seems to me to be
on another layer, above the test themselves. This change is about the
behavior of a test when not told about the target arch (and thus binary)
it should use.

> That said, I think the dictionary is a nice temporary workaround
> until we fix that.
> 

I think even with a switch towards "use the target(s) built", we'd still
need to pick one at the test execution level.  A job may include running
the same test with different targets, though.  So, I'm not sure this
would be just temporary.

- Cleber.

Re: [Qemu-devel] [PATCH] Acceptance tests: host arch to target arch name mapping

[Peter Maydell]

On 17 October 2018 at 18:38, Cleber Rosa <crosa@redhat.com> wrote:
>
>
> On 10/17/18 12:29 PM, Eduardo Habkost wrote:
>> On Wed, Oct 17, 2018 at 01:34:41PM +0100, Peter Maydell wrote:
>>> So, why does the test code need to care? It's not clear
>>> from the patch... My expectation would be that you'd
>>> just test all the testable target architectures,
>>> regardless of what the host architecture is.
>>
>> I tend to agree.  Maybe the right solution is to get rid of the
>> os.uname().  I think the default should be testing all QEMU
>> binaries that were built, and the host architecture shouldn't
>> matter.

Yes, looking at os.uname() also seems like an odd thing
for the tests to be doing here. The test framework
should be as far as possible host-architecture agnostic.
(For some of the KVM cases there probably is a need to
care, but those are exceptions, not the rule.)

> I'm in favor of exercising all built targets, but that seems to me to be
> on another layer, above the test themselves. This change is about the
> behavior of a test when not told about the target arch (and thus binary)
> it should use.

At that level, I think the right answer is "tell the user
they need to specify the qemu executable they are trying to test".
In particular, there is no guarantee that the user has actually
built the executable for the target that corresponds to the
host, so it doesn't work to try to default to that anyway.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] Acceptance tests: host arch to target arch name mapping

[Cleber Rosa]

On 10/17/18 2:40 PM, Peter Maydell wrote:
> On 17 October 2018 at 18:38, Cleber Rosa <crosa@redhat.com> wrote:
>>
>>
>> On 10/17/18 12:29 PM, Eduardo Habkost wrote:
>>> On Wed, Oct 17, 2018 at 01:34:41PM +0100, Peter Maydell wrote:
>>>> So, why does the test code need to care? It's not clear
>>>> from the patch... My expectation would be that you'd
>>>> just test all the testable target architectures,
>>>> regardless of what the host architecture is.
>>>
>>> I tend to agree.  Maybe the right solution is to get rid of the
>>> os.uname().  I think the default should be testing all QEMU
>>> binaries that were built, and the host architecture shouldn't
>>> matter.
> 
> Yes, looking at os.uname() also seems like an odd thing
> for the tests to be doing here. The test framework
> should be as far as possible host-architecture agnostic.
> (For some of the KVM cases there probably is a need to
> care, but those are exceptions, not the rule.)
> 
>> I'm in favor of exercising all built targets, but that seems to me to be
>> on another layer, above the test themselves. This change is about the
>> behavior of a test when not told about the target arch (and thus binary)
>> it should use.
> 
> At that level, I think the right answer is "tell the user
> they need to specify the qemu executable they are trying to test".
> In particular, there is no guarantee that the user has actually
> built the executable for the target that corresponds to the
> host, so it doesn't work to try to default to that anyway.
> 

I think your "right answer" is the ideal, but I'm afraid it's not
practical.  For instance, look at how a qemu-iotest is run:

$ ./check 001
QEMU          --
"/tmp/qemu-build/tests/qemu-iotests/../../ppc64-softmmu/qemu-system-ppc64"
-nodefaults -machine accel=qtest
...

IIRC, you're suggesting that check SHOULD abort and require a QEMU
binary.  The command line would then become something like:

$ ./check --qemu-bin=../../ppc64-softmmut/qemu-system-ppc64 001

The rules that "check" uses for finding a "default" QEMU binary are a
lot harder to describe (are "smarter") than the ones we currently have
on the acceptance tests.  IMO some compromises like these are necessary,
or else, tests may be run much much less than they could and should.

When it comes to trying to run the acceptance tests without an existing
QEMU binary built, the current behavior is to CANCEL the tests.

Regards,
- Cleber.

> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH] Acceptance tests: host arch to target arch name mapping

[Peter Maydell]

On 17 October 2018 at 20:05, Cleber Rosa <crosa@redhat.com> wrote:
>
>
> On 10/17/18 2:40 PM, Peter Maydell wrote:
>> At that level, I think the right answer is "tell the user
>> they need to specify the qemu executable they are trying to test".
>> In particular, there is no guarantee that the user has actually
>> built the executable for the target that corresponds to the
>> host, so it doesn't work to try to default to that anyway.
>>
>
> I think your "right answer" is the ideal, but I'm afraid it's not
> practical.  For instance, look at how a qemu-iotest is run:
>
> $ ./check 001
> QEMU          --
> "/tmp/qemu-build/tests/qemu-iotests/../../ppc64-softmmu/qemu-system-ppc64"
> -nodefaults -machine accel=qtest
> ...

I don't know how the iotests stuff works (they're odd
and not part of the stock "make check"), but the main
tests/Makefile.include handles tests which run for any
QEMU binary, tests which run only on certain target
architectures, and tests which don't use a QEMU binary
and just run once.

> The rules that "check" uses for finding a "default" QEMU binary are a
> lot harder to describe (are "smarter") than the ones we currently have
> on the acceptance tests.  IMO some compromises like these are necessary,
> or else, tests may be run much much less than they could and should.

I'm not sure what that check script is trying to do,
but my first impression is that it's probably not a
good model to follow.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] Acceptance tests: host arch to target arch name mapping

[Eduardo Habkost]

On Wed, Oct 17, 2018 at 07:40:51PM +0100, Peter Maydell wrote:
> On 17 October 2018 at 18:38, Cleber Rosa <crosa@redhat.com> wrote:
> >
> >
> > On 10/17/18 12:29 PM, Eduardo Habkost wrote:
> >> On Wed, Oct 17, 2018 at 01:34:41PM +0100, Peter Maydell wrote:
> >>> So, why does the test code need to care? It's not clear
> >>> from the patch... My expectation would be that you'd
> >>> just test all the testable target architectures,
> >>> regardless of what the host architecture is.
> >>
> >> I tend to agree.  Maybe the right solution is to get rid of the
> >> os.uname().  I think the default should be testing all QEMU
> >> binaries that were built, and the host architecture shouldn't
> >> matter.
> 
> Yes, looking at os.uname() also seems like an odd thing
> for the tests to be doing here. The test framework
> should be as far as possible host-architecture agnostic.
> (For some of the KVM cases there probably is a need to
> care, but those are exceptions, not the rule.)
> 
> > I'm in favor of exercising all built targets, but that seems to me to be
> > on another layer, above the test themselves. This change is about the
> > behavior of a test when not told about the target arch (and thus binary)
> > it should use.
> 
> At that level, I think the right answer is "tell the user
> they need to specify the qemu executable they are trying to test".
> In particular, there is no guarantee that the user has actually
> built the executable for the target that corresponds to the
> host, so it doesn't work to try to default to that anyway.

Agreed.  However, I don't see when exactly this message would be
triggered.  Cleber, on which use cases do you expect
pick_default_qemu_bin() to be called?

In an ideal world, any testing runner/tool should be able to
automatically test all binaries by default.  Can Avocado help us
do that?  (If not, we could just do it inside a
./tests/acceptance/run script).

-- 
Eduardo

Re: [Qemu-devel] [PATCH] Acceptance tests: host arch to target arch name mapping

[Cleber Rosa]

On 10/17/18 3:09 PM, Eduardo Habkost wrote:
> On Wed, Oct 17, 2018 at 07:40:51PM +0100, Peter Maydell wrote:
>> On 17 October 2018 at 18:38, Cleber Rosa <crosa@redhat.com> wrote:
>>>
>>>
>>> On 10/17/18 12:29 PM, Eduardo Habkost wrote:
>>>> On Wed, Oct 17, 2018 at 01:34:41PM +0100, Peter Maydell wrote:
>>>>> So, why does the test code need to care? It's not clear
>>>>> from the patch... My expectation would be that you'd
>>>>> just test all the testable target architectures,
>>>>> regardless of what the host architecture is.
>>>>
>>>> I tend to agree.  Maybe the right solution is to get rid of the
>>>> os.uname().  I think the default should be testing all QEMU
>>>> binaries that were built, and the host architecture shouldn't
>>>> matter.
>>
>> Yes, looking at os.uname() also seems like an odd thing
>> for the tests to be doing here. The test framework
>> should be as far as possible host-architecture agnostic.
>> (For some of the KVM cases there probably is a need to
>> care, but those are exceptions, not the rule.)
>>
>>> I'm in favor of exercising all built targets, but that seems to me to be
>>> on another layer, above the test themselves. This change is about the
>>> behavior of a test when not told about the target arch (and thus binary)
>>> it should use.
>>
>> At that level, I think the right answer is "tell the user
>> they need to specify the qemu executable they are trying to test".
>> In particular, there is no guarantee that the user has actually
>> built the executable for the target that corresponds to the
>> host, so it doesn't work to try to default to that anyway.
> 
> Agreed.  However, I don't see when exactly this message would be
> triggered.  Cleber, on which use cases do you expect
> pick_default_qemu_bin() to be called?
> 

When a test is run ad-hoc.  You even suggested that tests could/should
be executable.

> In an ideal world, any testing runner/tool should be able to
> automatically test all binaries by default.  Can Avocado help us
> do that?  (If not, we could just do it inside a
> ./tests/acceptance/run script).
> 

Avocado can do that indeed.  But I'm afraid that's not the main issue.
Think of the qemu-iotests: do we want a "check" command to run  all
tests with all binaries?

- Cleber.

Re: [Qemu-devel] [PATCH] Acceptance tests: host arch to target arch name mapping

[Eduardo Habkost]

On Wed, Oct 17, 2018 at 03:25:34PM -0400, Cleber Rosa wrote:
> 
> 
> On 10/17/18 3:09 PM, Eduardo Habkost wrote:
> > On Wed, Oct 17, 2018 at 07:40:51PM +0100, Peter Maydell wrote:
> >> On 17 October 2018 at 18:38, Cleber Rosa <crosa@redhat.com> wrote:
> >>>
> >>>
> >>> On 10/17/18 12:29 PM, Eduardo Habkost wrote:
> >>>> On Wed, Oct 17, 2018 at 01:34:41PM +0100, Peter Maydell wrote:
> >>>>> So, why does the test code need to care? It's not clear
> >>>>> from the patch... My expectation would be that you'd
> >>>>> just test all the testable target architectures,
> >>>>> regardless of what the host architecture is.
> >>>>
> >>>> I tend to agree.  Maybe the right solution is to get rid of the
> >>>> os.uname().  I think the default should be testing all QEMU
> >>>> binaries that were built, and the host architecture shouldn't
> >>>> matter.
> >>
> >> Yes, looking at os.uname() also seems like an odd thing
> >> for the tests to be doing here. The test framework
> >> should be as far as possible host-architecture agnostic.
> >> (For some of the KVM cases there probably is a need to
> >> care, but those are exceptions, not the rule.)
> >>
> >>> I'm in favor of exercising all built targets, but that seems to me to be
> >>> on another layer, above the test themselves. This change is about the
> >>> behavior of a test when not told about the target arch (and thus binary)
> >>> it should use.
> >>
> >> At that level, I think the right answer is "tell the user
> >> they need to specify the qemu executable they are trying to test".
> >> In particular, there is no guarantee that the user has actually
> >> built the executable for the target that corresponds to the
> >> host, so it doesn't work to try to default to that anyway.
> > 
> > Agreed.  However, I don't see when exactly this message would be
> > triggered.  Cleber, on which use cases do you expect
> > pick_default_qemu_bin() to be called?
> > 
> 
> When a test is run ad-hoc.  You even suggested that tests could/should
> be executable.
> 
> > In an ideal world, any testing runner/tool should be able to
> > automatically test all binaries by default.  Can Avocado help us
> > do that?  (If not, we could just do it inside a
> > ./tests/acceptance/run script).
> > 
> 
> Avocado can do that indeed.  But I'm afraid that's not the main issue.
> Think of the qemu-iotests: do we want a "check" command to run  all
> tests with all binaries?

Good question.  That would be my first expectation, but I'm not
sure.

Pro: testing all binaries by default would cause less confusion
than picking a random QEMU binary.

Con: testing all binaries may be inconvenient for quickly
checking if a test case works.  (I'm not convinced this is a
problem.  If you don't want to test everything, you probably
already have a short target list in your ./configure line?)

Pro: testing a single binary using uname() is already
implemented.

Con: making `avocado run` automatically generate variants of a
test case may take some effort?

-- 
Eduardo

Re: [Qemu-devel] [PATCH] Acceptance tests: host arch to target arch name mapping

[Cleber Rosa]

On 10/17/18 3:48 PM, Eduardo Habkost wrote:
> On Wed, Oct 17, 2018 at 03:25:34PM -0400, Cleber Rosa wrote:
>>
>>
>> On 10/17/18 3:09 PM, Eduardo Habkost wrote:
>>> On Wed, Oct 17, 2018 at 07:40:51PM +0100, Peter Maydell wrote:
>>>> On 17 October 2018 at 18:38, Cleber Rosa <crosa@redhat.com> wrote:
>>>>>
>>>>>
>>>>> On 10/17/18 12:29 PM, Eduardo Habkost wrote:
>>>>>> On Wed, Oct 17, 2018 at 01:34:41PM +0100, Peter Maydell wrote:
>>>>>>> So, why does the test code need to care? It's not clear
>>>>>>> from the patch... My expectation would be that you'd
>>>>>>> just test all the testable target architectures,
>>>>>>> regardless of what the host architecture is.
>>>>>>
>>>>>> I tend to agree.  Maybe the right solution is to get rid of the
>>>>>> os.uname().  I think the default should be testing all QEMU
>>>>>> binaries that were built, and the host architecture shouldn't
>>>>>> matter.
>>>>
>>>> Yes, looking at os.uname() also seems like an odd thing
>>>> for the tests to be doing here. The test framework
>>>> should be as far as possible host-architecture agnostic.
>>>> (For some of the KVM cases there probably is a need to
>>>> care, but those are exceptions, not the rule.)
>>>>
>>>>> I'm in favor of exercising all built targets, but that seems to me to be
>>>>> on another layer, above the test themselves. This change is about the
>>>>> behavior of a test when not told about the target arch (and thus binary)
>>>>> it should use.
>>>>
>>>> At that level, I think the right answer is "tell the user
>>>> they need to specify the qemu executable they are trying to test".
>>>> In particular, there is no guarantee that the user has actually
>>>> built the executable for the target that corresponds to the
>>>> host, so it doesn't work to try to default to that anyway.
>>>
>>> Agreed.  However, I don't see when exactly this message would be
>>> triggered.  Cleber, on which use cases do you expect
>>> pick_default_qemu_bin() to be called?
>>>
>>
>> When a test is run ad-hoc.  You even suggested that tests could/should
>> be executable.
>>
>>> In an ideal world, any testing runner/tool should be able to
>>> automatically test all binaries by default.  Can Avocado help us
>>> do that?  (If not, we could just do it inside a
>>> ./tests/acceptance/run script).
>>>
>>
>> Avocado can do that indeed.  But I'm afraid that's not the main issue.
>> Think of the qemu-iotests: do we want a "check" command to run  all
>> tests with all binaries?
> 
> Good question.  That would be my first expectation, but I'm not
> sure.
> 

If it wasn't clear, I'm trying to define the basic behavior of *one
test*.  I'm aware of a few different behaviors across tests in QEMU:

 1) qemu-iotests: require "check" to run, will attempt to find/run with
a "suitable" QEMU binary.

 2) libqtest based: executables, in theory runnable by themselves, and
will not attempt to find/run a "suitable" QEMU binary.  Those will
print: "Environment variable QTEST_QEMU_BINARY required".

 3) acceptance tests: require the Avocado test runner, will attempt to
find/run with a "suitable" QEMU binary.

So, I'm personally not aware of any test in QEMU which *by themselves*
defaults to running on all (relevant) built targets (machine types?
device types?).  Test selection (defining a test suite) and setting
parameters is always done elsewhere (Makefile, check-block.sh,
qemu-iotests-quick.sh, etc).

> Pro: testing all binaries by default would cause less confusion
> than picking a random QEMU binary.
> 

IMO we have to differentiate between *in test* QEMU binary selection
(some? none?) and other layers (Makefiles, scripts, etc).

> Con: testing all binaries may be inconvenient for quickly
> checking if a test case works.  (I'm not convinced this is a
> problem.  If you don't want to test everything, you probably
> already have a short target list in your ./configure line?)
> 

Convenience is important, but I'm convinced this is a software layering
problem.  I have the feeling we're trying to impose higher level
(environment/build/check) definitions to the lower level (test) entities.

> Pro: testing a single binary using uname() is already
> implemented.
> 

Right.  I'm not unfavorable to changing that behavior, and ERRORing
tests when a binary is not given (similar to libqtest) is a simple
change if we're to do it.  But I do find that usability drops considerably.

And finally I don't think the "if a qemu binary is not explicitly given,
let's try the matching host architecture" is confusing or hard to
follow.  And, it's pretty well documented if you ask me:

---
QEMU binary selection
~~~~~~~~~~~~~~~~~~~~~

The QEMU binary used for the ``self.vm`` QEMUMachine instance will
primarily depend on the value of the ``qemu_bin`` parameter.  If it's
not explicitly set, its default value will be the result of a dynamic
probe in the same source tree.  A suitable binary will be one that
targets the architecture matching (the) host machine.

Based on this description, test writers will usually rely on one of
the following approaches:

1) Set ``qemu_bin``, and use the given binary

2) Do not set ``qemu_bin``, and use a QEMU binary named like
   "${arch}-softmmu/qemu-system-${arch}", either in the current
   working directory, or in the current source tree.

The resulting ``qemu_bin`` value will be preserved in the
``avocado_qemu.Test`` as an attribute with the same name.
---

> Con: making `avocado run` automatically generate variants of a
> test case may take some effort?
> 

Well, it will take some effort, sure.  But my point do we want to
*enforce* that?  I think that should be left to a "run" script or make
rule like you suggested.  IMO, `avocado run a_single_test.py` shouldn't
do more than just that.

Regards,
- Cleber.

Re: [Qemu-devel] [PATCH] Acceptance tests: host arch to target arch name mapping

[Eduardo Habkost]

On Wed, Oct 17, 2018 at 04:54:52PM -0400, Cleber Rosa wrote:
> 
> 
> On 10/17/18 3:48 PM, Eduardo Habkost wrote:
> > On Wed, Oct 17, 2018 at 03:25:34PM -0400, Cleber Rosa wrote:
> >>
> >>
> >> On 10/17/18 3:09 PM, Eduardo Habkost wrote:
> >>> On Wed, Oct 17, 2018 at 07:40:51PM +0100, Peter Maydell wrote:
> >>>> On 17 October 2018 at 18:38, Cleber Rosa <crosa@redhat.com> wrote:
> >>>>>
> >>>>>
> >>>>> On 10/17/18 12:29 PM, Eduardo Habkost wrote:
> >>>>>> On Wed, Oct 17, 2018 at 01:34:41PM +0100, Peter Maydell wrote:
> >>>>>>> So, why does the test code need to care? It's not clear
> >>>>>>> from the patch... My expectation would be that you'd
> >>>>>>> just test all the testable target architectures,
> >>>>>>> regardless of what the host architecture is.
> >>>>>>
> >>>>>> I tend to agree.  Maybe the right solution is to get rid of the
> >>>>>> os.uname().  I think the default should be testing all QEMU
> >>>>>> binaries that were built, and the host architecture shouldn't
> >>>>>> matter.
> >>>>
> >>>> Yes, looking at os.uname() also seems like an odd thing
> >>>> for the tests to be doing here. The test framework
> >>>> should be as far as possible host-architecture agnostic.
> >>>> (For some of the KVM cases there probably is a need to
> >>>> care, but those are exceptions, not the rule.)
> >>>>
> >>>>> I'm in favor of exercising all built targets, but that seems to me to be
> >>>>> on another layer, above the test themselves. This change is about the
> >>>>> behavior of a test when not told about the target arch (and thus binary)
> >>>>> it should use.
> >>>>
> >>>> At that level, I think the right answer is "tell the user
> >>>> they need to specify the qemu executable they are trying to test".
> >>>> In particular, there is no guarantee that the user has actually
> >>>> built the executable for the target that corresponds to the
> >>>> host, so it doesn't work to try to default to that anyway.
> >>>
> >>> Agreed.  However, I don't see when exactly this message would be
> >>> triggered.  Cleber, on which use cases do you expect
> >>> pick_default_qemu_bin() to be called?
> >>>
> >>
> >> When a test is run ad-hoc.  You even suggested that tests could/should
> >> be executable.
> >>
> >>> In an ideal world, any testing runner/tool should be able to
> >>> automatically test all binaries by default.  Can Avocado help us
> >>> do that?  (If not, we could just do it inside a
> >>> ./tests/acceptance/run script).
> >>>
> >>
> >> Avocado can do that indeed.  But I'm afraid that's not the main issue.
> >> Think of the qemu-iotests: do we want a "check" command to run  all
> >> tests with all binaries?
> > 
> > Good question.  That would be my first expectation, but I'm not
> > sure.
> > 
> 
> If it wasn't clear, I'm trying to define the basic behavior of *one
> test*.  I'm aware of a few different behaviors across tests in QEMU:

I think we have some confusion here: I'm not sure what you mean
when you say "one test".  Note that I'm not talking about the
test code architecture, but about the interface we provide for
running tests.

> 
>  1) qemu-iotests: require "check" to run, will attempt to find/run with
> a "suitable" QEMU binary.
> 
>  2) libqtest based: executables, in theory runnable by themselves, and
> will not attempt to find/run a "suitable" QEMU binary.  Those will
> print: "Environment variable QTEST_QEMU_BINARY required".
> 
>  3) acceptance tests: require the Avocado test runner, will attempt to
> find/run with a "suitable" QEMU binary.
> 
> So, I'm personally not aware of any test in QEMU which *by themselves*
> defaults to running on all (relevant) built targets (machine types?
> device types?).  Test selection (defining a test suite) and setting
> parameters is always done elsewhere (Makefile, check-block.sh,
> qemu-iotests-quick.sh, etc).
> 
> > Pro: testing all binaries by default would cause less confusion
> > than picking a random QEMU binary.
> > 
> 
> IMO we have to differentiate between *in test* QEMU binary selection
> (some? none?) and other layers (Makefiles, scripts, etc).
> 
> > Con: testing all binaries may be inconvenient for quickly
> > checking if a test case works.  (I'm not convinced this is a
> > problem.  If you don't want to test everything, you probably
> > already have a short target list in your ./configure line?)
> > 
> 
> Convenience is important, but I'm convinced this is a software layering
> problem.  I have the feeling we're trying to impose higher level
> (environment/build/check) definitions to the lower level (test) entities.

I think we're mixing user interface with code
layering/organization.

The code organization may ensure the QEMU binary selection is in
another layer.  That's fine.

But the user interface we provide to running a single test should
be usable (and do what users expect).  That's where I think the
problem lies.  Maybe this UI problem could be addressed by
avocado, maybe it can be addressed by a wrapper script (see
comments below).


> 
> > Pro: testing a single binary using uname() is already
> > implemented.
> > 
> 
> Right.  I'm not unfavorable to changing that behavior, and ERRORing
> tests when a binary is not given (similar to libqtest) is a simple
> change if we're to do it.  But I do find that usability drops considerably.
> 
> And finally I don't think the "if a qemu binary is not explicitly given,
> let's try the matching host architecture" is confusing or hard to
> follow.  And, it's pretty well documented if you ask me:

I think it may cause confusion, and is inconsistent with all
other methods we recommend for running tests.

> 
> ---
> QEMU binary selection
> ~~~~~~~~~~~~~~~~~~~~~
> 
> The QEMU binary used for the ``self.vm`` QEMUMachine instance will
> primarily depend on the value of the ``qemu_bin`` parameter.  If it's
> not explicitly set, its default value will be the result of a dynamic
> probe in the same source tree.  A suitable binary will be one that
> targets the architecture matching (the) host machine.
> 
> Based on this description, test writers will usually rely on one of
> the following approaches:
> 
> 1) Set ``qemu_bin``, and use the given binary
> 
> 2) Do not set ``qemu_bin``, and use a QEMU binary named like
>    "${arch}-softmmu/qemu-system-${arch}", either in the current
>    working directory, or in the current source tree.
> 
> The resulting ``qemu_bin`` value will be preserved in the
> ``avocado_qemu.Test`` as an attribute with the same name.

If we provide a good user interface for running single tests
against all binaries, users won't even care about `qemu_bin`, and
this would be just a low level details inside the avocado_qemu
code.

"This is documented" is good.  "This doesn't need to be
documented" would be awesome.


> ---
> 
> > Con: making `avocado run` automatically generate variants of a
> > test case may take some effort?
> > 
> 
> Well, it will take some effort, sure.  But my point do we want to
> *enforce* that?  I think that should be left to a "run" script or make
> rule like you suggested.  IMO, `avocado run a_single_test.py` shouldn't
> do more than just that.

What do you mean by "do just that"?

I would love if avocado could be smarter and
"avocado run [<test>]" automatically got test variant information
somewhere and run multiple variants.  But if this is not possible
today, a wrapper script would be good enough to me.

-- 
Eduardo

Re: [Qemu-devel] [PATCH] Acceptance tests: host arch to target arch name mapping

[Cleber Rosa]

On 10/17/18 6:12 PM, Eduardo Habkost wrote:
> On Wed, Oct 17, 2018 at 04:54:52PM -0400, Cleber Rosa wrote:
>>
>>
>> On 10/17/18 3:48 PM, Eduardo Habkost wrote:
>>> On Wed, Oct 17, 2018 at 03:25:34PM -0400, Cleber Rosa wrote:
>>>>
>>>>
>>>> On 10/17/18 3:09 PM, Eduardo Habkost wrote:
>>>>> On Wed, Oct 17, 2018 at 07:40:51PM +0100, Peter Maydell wrote:
>>>>>> On 17 October 2018 at 18:38, Cleber Rosa <crosa@redhat.com> wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 10/17/18 12:29 PM, Eduardo Habkost wrote:
>>>>>>>> On Wed, Oct 17, 2018 at 01:34:41PM +0100, Peter Maydell wrote:
>>>>>>>>> So, why does the test code need to care? It's not clear
>>>>>>>>> from the patch... My expectation would be that you'd
>>>>>>>>> just test all the testable target architectures,
>>>>>>>>> regardless of what the host architecture is.
>>>>>>>>
>>>>>>>> I tend to agree.  Maybe the right solution is to get rid of the
>>>>>>>> os.uname().  I think the default should be testing all QEMU
>>>>>>>> binaries that were built, and the host architecture shouldn't
>>>>>>>> matter.
>>>>>>
>>>>>> Yes, looking at os.uname() also seems like an odd thing
>>>>>> for the tests to be doing here. The test framework
>>>>>> should be as far as possible host-architecture agnostic.
>>>>>> (For some of the KVM cases there probably is a need to
>>>>>> care, but those are exceptions, not the rule.)
>>>>>>
>>>>>>> I'm in favor of exercising all built targets, but that seems to me to be
>>>>>>> on another layer, above the test themselves. This change is about the
>>>>>>> behavior of a test when not told about the target arch (and thus binary)
>>>>>>> it should use.
>>>>>>
>>>>>> At that level, I think the right answer is "tell the user
>>>>>> they need to specify the qemu executable they are trying to test".
>>>>>> In particular, there is no guarantee that the user has actually
>>>>>> built the executable for the target that corresponds to the
>>>>>> host, so it doesn't work to try to default to that anyway.
>>>>>
>>>>> Agreed.  However, I don't see when exactly this message would be
>>>>> triggered.  Cleber, on which use cases do you expect
>>>>> pick_default_qemu_bin() to be called?
>>>>>
>>>>
>>>> When a test is run ad-hoc.  You even suggested that tests could/should
>>>> be executable.
>>>>
>>>>> In an ideal world, any testing runner/tool should be able to
>>>>> automatically test all binaries by default.  Can Avocado help us
>>>>> do that?  (If not, we could just do it inside a
>>>>> ./tests/acceptance/run script).
>>>>>
>>>>
>>>> Avocado can do that indeed.  But I'm afraid that's not the main issue.
>>>> Think of the qemu-iotests: do we want a "check" command to run  all
>>>> tests with all binaries?
>>>
>>> Good question.  That would be my first expectation, but I'm not
>>> sure.
>>>
>>
>> If it wasn't clear, I'm trying to define the basic behavior of *one
>> test*.  I'm aware of a few different behaviors across tests in QEMU:
> 
> I think we have some confusion here: I'm not sure what you mean
> when you say "one test".  Note that I'm not talking about the
> test code architecture, but about the interface we provide for
> running tests.
> 
>>
>>  1) qemu-iotests: require "check" to run, will attempt to find/run with
>> a "suitable" QEMU binary.
>>
>>  2) libqtest based: executables, in theory runnable by themselves, and
>> will not attempt to find/run a "suitable" QEMU binary.  Those will
>> print: "Environment variable QTEST_QEMU_BINARY required".
>>
>>  3) acceptance tests: require the Avocado test runner, will attempt to
>> find/run with a "suitable" QEMU binary.
>>
>> So, I'm personally not aware of any test in QEMU which *by themselves*
>> defaults to running on all (relevant) built targets (machine types?
>> device types?).  Test selection (defining a test suite) and setting
>> parameters is always done elsewhere (Makefile, check-block.sh,
>> qemu-iotests-quick.sh, etc).
>>
>>> Pro: testing all binaries by default would cause less confusion
>>> than picking a random QEMU binary.
>>>
>>
>> IMO we have to differentiate between *in test* QEMU binary selection
>> (some? none?) and other layers (Makefiles, scripts, etc).
>>
>>> Con: testing all binaries may be inconvenient for quickly
>>> checking if a test case works.  (I'm not convinced this is a
>>> problem.  If you don't want to test everything, you probably
>>> already have a short target list in your ./configure line?)
>>>
>>
>> Convenience is important, but I'm convinced this is a software layering
>> problem.  I have the feeling we're trying to impose higher level
>> (environment/build/check) definitions to the lower level (test) entities.
> 
> I think we're mixing user interface with code
> layering/organization.
> 
> The code organization may ensure the QEMU binary selection is in
> another layer.  That's fine.
> 

OK.

> But the user interface we provide to running a single test should
> be usable (and do what users expect).  That's where I think the
> problem lies.  Maybe this UI problem could be addressed by
> avocado, maybe it can be addressed by a wrapper script (see
> comments below).
> 
> 

I absolutely agree with "running a single test should be usable (and do
what users expect)".

>>
>>> Pro: testing a single binary using uname() is already
>>> implemented.
>>>
>>
>> Right.  I'm not unfavorable to changing that behavior, and ERRORing
>> tests when a binary is not given (similar to libqtest) is a simple
>> change if we're to do it.  But I do find that usability drops considerably.
>>
>> And finally I don't think the "if a qemu binary is not explicitly given,
>> let's try the matching host architecture" is confusing or hard to
>> follow.  And, it's pretty well documented if you ask me:
> 
> I think it may cause confusion, and is inconsistent with all
> other methods we recommend for running tests.
> 

It's hard to argue against "it may cause confusion" when something is
decided during runtime, so you clearly have a point.  But, given the
behavior of the iotests which is exactly that, I don't consider it
inconsistent with all other tests in QEMU.  As Peter said, it may not be
a model to follow, though.

>>
>> ---
>> QEMU binary selection
>> ~~~~~~~~~~~~~~~~~~~~~
>>
>> The QEMU binary used for the ``self.vm`` QEMUMachine instance will
>> primarily depend on the value of the ``qemu_bin`` parameter.  If it's
>> not explicitly set, its default value will be the result of a dynamic
>> probe in the same source tree.  A suitable binary will be one that
>> targets the architecture matching (the) host machine.
>>
>> Based on this description, test writers will usually rely on one of
>> the following approaches:
>>
>> 1) Set ``qemu_bin``, and use the given binary
>>
>> 2) Do not set ``qemu_bin``, and use a QEMU binary named like
>>    "${arch}-softmmu/qemu-system-${arch}", either in the current
>>    working directory, or in the current source tree.
>>
>> The resulting ``qemu_bin`` value will be preserved in the
>> ``avocado_qemu.Test`` as an attribute with the same name.
> 
> If we provide a good user interface for running single tests
> against all binaries, users won't even care about `qemu_bin`, and
> this would be just a low level details inside the avocado_qemu
> code.
> 
> "This is documented" is good.  "This doesn't need to be
> documented" would be awesome.
> 
> 

Agreed.

>> ---
>>
>>> Con: making `avocado run` automatically generate variants of a
>>> test case may take some effort?
>>>
>>
>> Well, it will take some effort, sure.  But my point do we want to
>> *enforce* that?  I think that should be left to a "run" script or make
>> rule like you suggested.  IMO, `avocado run a_single_test.py` shouldn't
>> do more than just that.
> 
> What do you mean by "do just that"?

I mean "running a single test" (one test, one variant).  The test
results would show a "RESULTS: PASS 1 | FAIL 0 | ... | CANCEL 0".

> 
> I would love if avocado could be smarter and
> "avocado run [<test>]" automatically got test variant information
> somewhere and run multiple variants.  But if this is not possible
> today, a wrapper script would be good enough to me.
> 

We're definitely on the same page, and, I believe this could be
implemented with a new varianter plugin.  Then, doing:

 $ avocado run --varianter-qemu-targets-built <test>

Would give us N test executions, one per variant.  I kind of
demonstrated that with the variants/arch.json file.

Now, nothing prevents us from having an Avocado configuration file that
will make "--varianter-qemu-targets-built" a default option when running
inside a QEMU build tree.  Then, I'd expect:

 $ avocado config
 Config files read (in order):
      /etc/avocado/avocado.conf
      ...
      /tmp/qemu-build/.avocado.conf

And "avocado run a_single_test.py" to produce to produce something like:

JOB ID     : 0fb835fb82d26627456c9f6af045858b20e1e5af
JOB LOG    : job-results/job-2018-10-17T19.04-0fb835f/job.log
VARIANTS   : QEMUBuiltTargets [x86_64-softmmu, ppc64-softmmu]
 (1/2) : a_single_test.py-x86_64-softmmu PASS (0.03 s)
 (2/2) : a_single_test.py-ppc64-softmmu PASS (0.03 s)
RESULTS    : PASS 2 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 |
CANCEL 0
JOB TIME   : 0.19 s

Does this fits into your understanding of what we need?

- Cleber.

Re: [Qemu-devel] [PATCH] Acceptance tests: host arch to target arch name mapping

[Eduardo Habkost]

On Wed, Oct 17, 2018 at 07:17:25PM -0400, Cleber Rosa wrote:
> 
> 
> On 10/17/18 6:12 PM, Eduardo Habkost wrote:
> > On Wed, Oct 17, 2018 at 04:54:52PM -0400, Cleber Rosa wrote:
> >>
> >>
> >> On 10/17/18 3:48 PM, Eduardo Habkost wrote:
> >>> On Wed, Oct 17, 2018 at 03:25:34PM -0400, Cleber Rosa wrote:
> >>>>
> >>>>
> >>>> On 10/17/18 3:09 PM, Eduardo Habkost wrote:
> >>>>> On Wed, Oct 17, 2018 at 07:40:51PM +0100, Peter Maydell wrote:
> >>>>>> On 17 October 2018 at 18:38, Cleber Rosa <crosa@redhat.com> wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>> On 10/17/18 12:29 PM, Eduardo Habkost wrote:
> >>>>>>>> On Wed, Oct 17, 2018 at 01:34:41PM +0100, Peter Maydell wrote:
> >>>>>>>>> So, why does the test code need to care? It's not clear
> >>>>>>>>> from the patch... My expectation would be that you'd
> >>>>>>>>> just test all the testable target architectures,
> >>>>>>>>> regardless of what the host architecture is.
> >>>>>>>>
> >>>>>>>> I tend to agree.  Maybe the right solution is to get rid of the
> >>>>>>>> os.uname().  I think the default should be testing all QEMU
> >>>>>>>> binaries that were built, and the host architecture shouldn't
> >>>>>>>> matter.
> >>>>>>
> >>>>>> Yes, looking at os.uname() also seems like an odd thing
> >>>>>> for the tests to be doing here. The test framework
> >>>>>> should be as far as possible host-architecture agnostic.
> >>>>>> (For some of the KVM cases there probably is a need to
> >>>>>> care, but those are exceptions, not the rule.)
> >>>>>>
> >>>>>>> I'm in favor of exercising all built targets, but that seems to me to be
> >>>>>>> on another layer, above the test themselves. This change is about the
> >>>>>>> behavior of a test when not told about the target arch (and thus binary)
> >>>>>>> it should use.
> >>>>>>
> >>>>>> At that level, I think the right answer is "tell the user
> >>>>>> they need to specify the qemu executable they are trying to test".
> >>>>>> In particular, there is no guarantee that the user has actually
> >>>>>> built the executable for the target that corresponds to the
> >>>>>> host, so it doesn't work to try to default to that anyway.
> >>>>>
> >>>>> Agreed.  However, I don't see when exactly this message would be
> >>>>> triggered.  Cleber, on which use cases do you expect
> >>>>> pick_default_qemu_bin() to be called?
> >>>>>
> >>>>
> >>>> When a test is run ad-hoc.  You even suggested that tests could/should
> >>>> be executable.
> >>>>
> >>>>> In an ideal world, any testing runner/tool should be able to
> >>>>> automatically test all binaries by default.  Can Avocado help us
> >>>>> do that?  (If not, we could just do it inside a
> >>>>> ./tests/acceptance/run script).
> >>>>>
> >>>>
> >>>> Avocado can do that indeed.  But I'm afraid that's not the main issue.
> >>>> Think of the qemu-iotests: do we want a "check" command to run  all
> >>>> tests with all binaries?
> >>>
> >>> Good question.  That would be my first expectation, but I'm not
> >>> sure.
> >>>
> >>
> >> If it wasn't clear, I'm trying to define the basic behavior of *one
> >> test*.  I'm aware of a few different behaviors across tests in QEMU:
> > 
> > I think we have some confusion here: I'm not sure what you mean
> > when you say "one test".  Note that I'm not talking about the
> > test code architecture, but about the interface we provide for
> > running tests.
> > 
> >>
> >>  1) qemu-iotests: require "check" to run, will attempt to find/run with
> >> a "suitable" QEMU binary.
> >>
> >>  2) libqtest based: executables, in theory runnable by themselves, and
> >> will not attempt to find/run a "suitable" QEMU binary.  Those will
> >> print: "Environment variable QTEST_QEMU_BINARY required".
> >>
> >>  3) acceptance tests: require the Avocado test runner, will attempt to
> >> find/run with a "suitable" QEMU binary.
> >>
> >> So, I'm personally not aware of any test in QEMU which *by themselves*
> >> defaults to running on all (relevant) built targets (machine types?
> >> device types?).  Test selection (defining a test suite) and setting
> >> parameters is always done elsewhere (Makefile, check-block.sh,
> >> qemu-iotests-quick.sh, etc).
> >>
> >>> Pro: testing all binaries by default would cause less confusion
> >>> than picking a random QEMU binary.
> >>>
> >>
> >> IMO we have to differentiate between *in test* QEMU binary selection
> >> (some? none?) and other layers (Makefiles, scripts, etc).
> >>
> >>> Con: testing all binaries may be inconvenient for quickly
> >>> checking if a test case works.  (I'm not convinced this is a
> >>> problem.  If you don't want to test everything, you probably
> >>> already have a short target list in your ./configure line?)
> >>>
> >>
> >> Convenience is important, but I'm convinced this is a software layering
> >> problem.  I have the feeling we're trying to impose higher level
> >> (environment/build/check) definitions to the lower level (test) entities.
> > 
> > I think we're mixing user interface with code
> > layering/organization.
> > 
> > The code organization may ensure the QEMU binary selection is in
> > another layer.  That's fine.
> > 
> 
> OK.
> 
> > But the user interface we provide to running a single test should
> > be usable (and do what users expect).  That's where I think the
> > problem lies.  Maybe this UI problem could be addressed by
> > avocado, maybe it can be addressed by a wrapper script (see
> > comments below).
> > 
> > 
> 
> I absolutely agree with "running a single test should be usable (and do
> what users expect)".
> 
> >>
> >>> Pro: testing a single binary using uname() is already
> >>> implemented.
> >>>
> >>
> >> Right.  I'm not unfavorable to changing that behavior, and ERRORing
> >> tests when a binary is not given (similar to libqtest) is a simple
> >> change if we're to do it.  But I do find that usability drops considerably.
> >>
> >> And finally I don't think the "if a qemu binary is not explicitly given,
> >> let's try the matching host architecture" is confusing or hard to
> >> follow.  And, it's pretty well documented if you ask me:
> > 
> > I think it may cause confusion, and is inconsistent with all
> > other methods we recommend for running tests.
> > 
> 
> It's hard to argue against "it may cause confusion" when something is
> decided during runtime, so you clearly have a point.  But, given the
> behavior of the iotests which is exactly that, I don't consider it
> inconsistent with all other tests in QEMU.  As Peter said, it may not be
> a model to follow, though.

Yeah, we don't seem to have a consistent rule, here.

Anyway, maybe "run against all binaries" won't be an absolute
rule: maybe we'll have exceptions on some cases to keep the size
of test jobs under control.

iotests itself looks like a justifiable exception: if the full
test suite takes very long to run and it's testing mostly
arch-independent code, it could make sense to choose only one
QEMU binary for running those tests by default.


> 
> >>
> >> ---
> >> QEMU binary selection
> >> ~~~~~~~~~~~~~~~~~~~~~
> >>
> >> The QEMU binary used for the ``self.vm`` QEMUMachine instance will
> >> primarily depend on the value of the ``qemu_bin`` parameter.  If it's
> >> not explicitly set, its default value will be the result of a dynamic
> >> probe in the same source tree.  A suitable binary will be one that
> >> targets the architecture matching (the) host machine.
> >>
> >> Based on this description, test writers will usually rely on one of
> >> the following approaches:
> >>
> >> 1) Set ``qemu_bin``, and use the given binary
> >>
> >> 2) Do not set ``qemu_bin``, and use a QEMU binary named like
> >>    "${arch}-softmmu/qemu-system-${arch}", either in the current
> >>    working directory, or in the current source tree.
> >>
> >> The resulting ``qemu_bin`` value will be preserved in the
> >> ``avocado_qemu.Test`` as an attribute with the same name.
> > 
> > If we provide a good user interface for running single tests
> > against all binaries, users won't even care about `qemu_bin`, and
> > this would be just a low level details inside the avocado_qemu
> > code.
> > 
> > "This is documented" is good.  "This doesn't need to be
> > documented" would be awesome.
> > 
> > 
> 
> Agreed.
> 
> >> ---
> >>
> >>> Con: making `avocado run` automatically generate variants of a
> >>> test case may take some effort?
> >>>
> >>
> >> Well, it will take some effort, sure.  But my point do we want to
> >> *enforce* that?  I think that should be left to a "run" script or make
> >> rule like you suggested.  IMO, `avocado run a_single_test.py` shouldn't
> >> do more than just that.
> > 
> > What do you mean by "do just that"?
> 
> I mean "running a single test" (one test, one variant).  The test
> results would show a "RESULTS: PASS 1 | FAIL 0 | ... | CANCEL 0".

Got it.


> 
> > 
> > I would love if avocado could be smarter and
> > "avocado run [<test>]" automatically got test variant information
> > somewhere and run multiple variants.  But if this is not possible
> > today, a wrapper script would be good enough to me.
> > 
> 
> We're definitely on the same page, and, I believe this could be
> implemented with a new varianter plugin.  Then, doing:
> 
>  $ avocado run --varianter-qemu-targets-built <test>
> 
> Would give us N test executions, one per variant.  I kind of
> demonstrated that with the variants/arch.json file.
> 
> Now, nothing prevents us from having an Avocado configuration file that
> will make "--varianter-qemu-targets-built" a default option when running
> inside a QEMU build tree.  Then, I'd expect:
> 
>  $ avocado config
>  Config files read (in order):
>       /etc/avocado/avocado.conf
>       ...
>       /tmp/qemu-build/.avocado.conf
> 
> And "avocado run a_single_test.py" to produce to produce something like:
> 
> JOB ID     : 0fb835fb82d26627456c9f6af045858b20e1e5af
> JOB LOG    : job-results/job-2018-10-17T19.04-0fb835f/job.log
> VARIANTS   : QEMUBuiltTargets [x86_64-softmmu, ppc64-softmmu]
>  (1/2) : a_single_test.py-x86_64-softmmu PASS (0.03 s)
>  (2/2) : a_single_test.py-ppc64-softmmu PASS (0.03 s)
> RESULTS    : PASS 2 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 |
> CANCEL 0
> JOB TIME   : 0.19 s
> 
> Does this fits into your understanding of what we need?

Absolutely.  Thanks!

-- 
Eduardo

Re: [Qemu-devel] [PATCH] Acceptance tests: host arch to target arch name mapping

[Murilo Opsfelder Araujo]

On Wed, Oct 17, 2018 at 04:09:51PM -0300, Eduardo Habkost wrote:
> On Wed, Oct 17, 2018 at 07:40:51PM +0100, Peter Maydell wrote:
> > On 17 October 2018 at 18:38, Cleber Rosa <crosa@redhat.com> wrote:
> > >
> > >
> > > On 10/17/18 12:29 PM, Eduardo Habkost wrote:
> > >> On Wed, Oct 17, 2018 at 01:34:41PM +0100, Peter Maydell wrote:
> > >>> So, why does the test code need to care? It's not clear
> > >>> from the patch... My expectation would be that you'd
> > >>> just test all the testable target architectures,
> > >>> regardless of what the host architecture is.
> > >>
> > >> I tend to agree.  Maybe the right solution is to get rid of the
> > >> os.uname().  I think the default should be testing all QEMU
> > >> binaries that were built, and the host architecture shouldn't
> > >> matter.
> >
> > Yes, looking at os.uname() also seems like an odd thing
> > for the tests to be doing here. The test framework
> > should be as far as possible host-architecture agnostic.
> > (For some of the KVM cases there probably is a need to
> > care, but those are exceptions, not the rule.)
> >
> > > I'm in favor of exercising all built targets, but that seems to me to be
> > > on another layer, above the test themselves. This change is about the
> > > behavior of a test when not told about the target arch (and thus binary)
> > > it should use.
> >
> > At that level, I think the right answer is "tell the user
> > they need to specify the qemu executable they are trying to test".
> > In particular, there is no guarantee that the user has actually
> > built the executable for the target that corresponds to the
> > host, so it doesn't work to try to default to that anyway.
>
> Agreed.  However, I don't see when exactly this message would be
> triggered.  Cleber, on which use cases do you expect
> pick_default_qemu_bin() to be called?
>
> In an ideal world, any testing runner/tool should be able to
> automatically test all binaries by default.  Can Avocado help us
> do that?  (If not, we could just do it inside a
> ./tests/acceptance/run script).
>
> --
> Eduardo
>

Why don't we add a variants file to QEMU's tree listing all known possible paths
for QEMU binary that can result from a build?  For example:

    $ cat tests/acceptance/variants
    qemu_bin:
        - ppc-softmmu/qemu-system-ppc
        - ppc64-softmmu/qemu-system-ppc64
        - x86_64-softmmu/qemu-system-x86_64

Then avocado could multiplex these variants file and call ./tests/acceptance/run
for each value of qemu_bin.  `run` script could skip and return if $qemu_bin
doesn't exist.  This approach also allows user forcing a value of qemu_bin when
calling `run` manually, for example:

    ./tests/acceptance/run --qemu_bin=/path/to/your/qemu-system-blah ...

This ./tests/acceptance/run can serve as an entry point to run all the tests.
If more parameters are considered mandatory in the future, the logic can be
placed there.

--
Murilo

Re: [Qemu-devel] [PATCH] Acceptance tests: host arch to target arch name mapping

[Cleber Rosa]

On 10/17/18 4:46 PM, Murilo Opsfelder Araujo wrote:
> On Wed, Oct 17, 2018 at 04:09:51PM -0300, Eduardo Habkost wrote:
>> On Wed, Oct 17, 2018 at 07:40:51PM +0100, Peter Maydell wrote:
>>> On 17 October 2018 at 18:38, Cleber Rosa <crosa@redhat.com> wrote:
>>>>
>>>>
>>>> On 10/17/18 12:29 PM, Eduardo Habkost wrote:
>>>>> On Wed, Oct 17, 2018 at 01:34:41PM +0100, Peter Maydell wrote:
>>>>>> So, why does the test code need to care? It's not clear
>>>>>> from the patch... My expectation would be that you'd
>>>>>> just test all the testable target architectures,
>>>>>> regardless of what the host architecture is.
>>>>>
>>>>> I tend to agree.  Maybe the right solution is to get rid of the
>>>>> os.uname().  I think the default should be testing all QEMU
>>>>> binaries that were built, and the host architecture shouldn't
>>>>> matter.
>>>
>>> Yes, looking at os.uname() also seems like an odd thing
>>> for the tests to be doing here. The test framework
>>> should be as far as possible host-architecture agnostic.
>>> (For some of the KVM cases there probably is a need to
>>> care, but those are exceptions, not the rule.)
>>>
>>>> I'm in favor of exercising all built targets, but that seems to me to be
>>>> on another layer, above the test themselves. This change is about the
>>>> behavior of a test when not told about the target arch (and thus binary)
>>>> it should use.
>>>
>>> At that level, I think the right answer is "tell the user
>>> they need to specify the qemu executable they are trying to test".
>>> In particular, there is no guarantee that the user has actually
>>> built the executable for the target that corresponds to the
>>> host, so it doesn't work to try to default to that anyway.
>>
>> Agreed.  However, I don't see when exactly this message would be
>> triggered.  Cleber, on which use cases do you expect
>> pick_default_qemu_bin() to be called?
>>
>> In an ideal world, any testing runner/tool should be able to
>> automatically test all binaries by default.  Can Avocado help us
>> do that?  (If not, we could just do it inside a
>> ./tests/acceptance/run script).
>>
>> --
>> Eduardo
>>
> 
> Why don't we add a variants file to QEMU's tree listing all known possible paths
> for QEMU binary that can result from a build?  For example:
> 
>     $ cat tests/acceptance/variants
>     qemu_bin:
>         - ppc-softmmu/qemu-system-ppc
>         - ppc64-softmmu/qemu-system-ppc64
>         - x86_64-softmmu/qemu-system-x86_64
> 

That's possible, and even proposed on the "basic arch support" series.
But this is a higher level question IMO (it exceeds the test boundaries).

> Then avocado could multiplex these variants file and call ./tests/acceptance/run
> for each value of qemu_bin.  `run` script could skip and return if $qemu_bin
> doesn't exist.  This approach also allows user forcing a value of qemu_bin when
> calling `run` manually, for example:
> 
>     ./tests/acceptance/run --qemu_bin=/path/to/your/qemu-system-blah ...
> 
> This ./tests/acceptance/run can serve as an entry point to run all the tests.
> If more parameters are considered mandatory in the future, the logic can be
> placed there.
> 

I'm completely favorable to having extra scripts or make rules that
define a standard "job" behavior.  In fact, I think we'll end up having
many of those.  "make check-acceptance" is just the first one.

But being able to running individual tests should still be possible, and
easy IMO.

Regards,
- Cleber.

> --
> Murilo
> 

Re: [Qemu-devel] [PATCH] Acceptance tests: host arch to target arch name mapping

[Eduardo Habkost]

On Wed, Oct 17, 2018 at 04:59:25PM -0400, Cleber Rosa wrote:
> On 10/17/18 4:46 PM, Murilo Opsfelder Araujo wrote:
[...]
> > Then avocado could multiplex these variants file and call ./tests/acceptance/run
> > for each value of qemu_bin.  `run` script could skip and return if $qemu_bin
> > doesn't exist.  This approach also allows user forcing a value of qemu_bin when
> > calling `run` manually, for example:
> > 
> >     ./tests/acceptance/run --qemu_bin=/path/to/your/qemu-system-blah ...
> > 
> > This ./tests/acceptance/run can serve as an entry point to run all the tests.
> > If more parameters are considered mandatory in the future, the logic can be
> > placed there.
> > 
> 
> I'm completely favorable to having extra scripts or make rules that
> define a standard "job" behavior.  In fact, I think we'll end up having
> many of those.  "make check-acceptance" is just the first one.
> 
> But being able to running individual tests should still be possible, and
> easy IMO.

Agreed.  But is there anything that requires "running an
individual test" to be synonymous to "running a test against only
one QEMU binary"?

We seem to have a terminology problem here: "I'm running one
individual test" may mean something to an user, but mean
something completely different to somebody thinking about Avocado
test cases.  Is this the source of our disagreement?

-- 
Eduardo

Re: [Qemu-devel] [PATCH] Acceptance tests: host arch to target arch name mapping

[Cleber Rosa]

On 10/17/18 6:15 PM, Eduardo Habkost wrote:
> On Wed, Oct 17, 2018 at 04:59:25PM -0400, Cleber Rosa wrote:
>> On 10/17/18 4:46 PM, Murilo Opsfelder Araujo wrote:
> [...]
>>> Then avocado could multiplex these variants file and call ./tests/acceptance/run
>>> for each value of qemu_bin.  `run` script could skip and return if $qemu_bin
>>> doesn't exist.  This approach also allows user forcing a value of qemu_bin when
>>> calling `run` manually, for example:
>>>
>>>     ./tests/acceptance/run --qemu_bin=/path/to/your/qemu-system-blah ...
>>>
>>> This ./tests/acceptance/run can serve as an entry point to run all the tests.
>>> If more parameters are considered mandatory in the future, the logic can be
>>> placed there.
>>>
>>
>> I'm completely favorable to having extra scripts or make rules that
>> define a standard "job" behavior.  In fact, I think we'll end up having
>> many of those.  "make check-acceptance" is just the first one.
>>
>> But being able to running individual tests should still be possible, and
>> easy IMO.
> 
> Agreed.  But is there anything that requires "running an
> individual test" to be synonymous to "running a test against only
> one QEMU binary"?
> 

I consider, generally speaking:

 * the test: the (parameterizable) code
 * running an individual test: the execution of that code, once, with
one parameter

There may be tests in which the logic of *one test* may require
executing different QEMU binaries, but that is the exception, not the
rule IMO.

Given that the acceptance tests are Avocado tests, I think it'd be a
mistake (or very hard) to try to make "an individual (Avocado) test" use
all the built QEMU binaries.  Variants (one for each unique set of
parameters) is the natural thing to use here.

> We seem to have a terminology problem here: "I'm running one
> individual test" may mean something to an user, but mean
> something completely different to somebody thinking about Avocado
> test cases.  Is this the source of our disagreement?
> 

It could be, I'm certainly biased by the Avocado definition of tests and
my limited understanding what others here may mean when they say "a
test".  For instance, it may be that, to some, "make check" is
considered "a test", and consequently, needs to run with all built QEMU
binaries.

Regards,
- Cleber.

Re: [Qemu-devel] [PATCH] Acceptance tests: host arch to target arch name mapping

[Eduardo Habkost]

On Wed, Oct 17, 2018 at 06:47:41PM -0400, Cleber Rosa wrote:
> 
> 
> On 10/17/18 6:15 PM, Eduardo Habkost wrote:
> > On Wed, Oct 17, 2018 at 04:59:25PM -0400, Cleber Rosa wrote:
> >> On 10/17/18 4:46 PM, Murilo Opsfelder Araujo wrote:
> > [...]
> >>> Then avocado could multiplex these variants file and call ./tests/acceptance/run
> >>> for each value of qemu_bin.  `run` script could skip and return if $qemu_bin
> >>> doesn't exist.  This approach also allows user forcing a value of qemu_bin when
> >>> calling `run` manually, for example:
> >>>
> >>>     ./tests/acceptance/run --qemu_bin=/path/to/your/qemu-system-blah ...
> >>>
> >>> This ./tests/acceptance/run can serve as an entry point to run all the tests.
> >>> If more parameters are considered mandatory in the future, the logic can be
> >>> placed there.
> >>>
> >>
> >> I'm completely favorable to having extra scripts or make rules that
> >> define a standard "job" behavior.  In fact, I think we'll end up having
> >> many of those.  "make check-acceptance" is just the first one.
> >>
> >> But being able to running individual tests should still be possible, and
> >> easy IMO.
> > 
> > Agreed.  But is there anything that requires "running an
> > individual test" to be synonymous to "running a test against only
> > one QEMU binary"?
> > 
> 
> I consider, generally speaking:
> 
>  * the test: the (parameterizable) code
>  * running an individual test: the execution of that code, once, with
> one parameter

This is where we're talking about completely different things.
When developers say "I want to run the test I have written", they
are probably expecting that code to run multiple times, each time
with different parameters.

If we implement this as multiple Avocado test cases, or in a
separate layer, it doesn't matter as long as it's easy and
obvious to run them.

> 
> There may be tests in which the logic of *one test* may require
> executing different QEMU binaries, but that is the exception, not the
> rule IMO.
> 
> Given that the acceptance tests are Avocado tests, I think it'd be a
> mistake (or very hard) to try to make "an individual (Avocado) test" use
> all the built QEMU binaries.  Variants (one for each unique set of
> parameters) is the natural thing to use here.

I trust your advice regarding the Avocado API.  If it's not good
practice, we don't need to make an individual Avocado test case
use built QEMU binaries.  We just need to provide an easy way for
developers to run the test they have written against all the
binaries.

If we can make that work with "avocado run mytest.py" it would
be nice, but not a must.

If we can make that work with "./tests/acceptance/mytest.py", it
would be great, but not a must.

If we need to make that work with "./tests/acceptance/run
mytest.py", it's good enough.


> 
> > We seem to have a terminology problem here: "I'm running one
> > individual test" may mean something to an user, but mean
> > something completely different to somebody thinking about Avocado
> > test cases.  Is this the source of our disagreement?
> > 
> 
> It could be, I'm certainly biased by the Avocado definition of tests and
> my limited understanding what others here may mean when they say "a
> test".  For instance, it may be that, to some, "make check" is
> considered "a test", and consequently, needs to run with all built QEMU
> binaries.

Probably "make check" wouldn't be called "a test", but:
* "tests/acceptance/mytest.py" is probably going to be called
  "a test".
* "I want to run mytest.py" would probably mean
  "I want to run mytst.py against all built QEMU binaries",
  not "I want to run one single Avocado test case with mytest.py".

-- 
Eduardo

Re: [Qemu-devel] [PATCH] Acceptance tests: host arch to target arch name mapping

[Murilo Opsfelder Araujo]

On Wed, Oct 17, 2018 at 07:40:51PM +0100, Peter Maydell wrote:
> On 17 October 2018 at 18:38, Cleber Rosa <crosa@redhat.com> wrote:
> >
> >
> > On 10/17/18 12:29 PM, Eduardo Habkost wrote:
> >> On Wed, Oct 17, 2018 at 01:34:41PM +0100, Peter Maydell wrote:
> >>> So, why does the test code need to care? It's not clear
> >>> from the patch... My expectation would be that you'd
> >>> just test all the testable target architectures,
> >>> regardless of what the host architecture is.
> >>
> >> I tend to agree.  Maybe the right solution is to get rid of the
> >> os.uname().  I think the default should be testing all QEMU
> >> binaries that were built, and the host architecture shouldn't
> >> matter.
>
> Yes, looking at os.uname() also seems like an odd thing
> for the tests to be doing here. The test framework
> should be as far as possible host-architecture agnostic.
> (For some of the KVM cases there probably is a need to
> care, but those are exceptions, not the rule.)
>
> > I'm in favor of exercising all built targets, but that seems to me to be
> > on another layer, above the test themselves. This change is about the
> > behavior of a test when not told about the target arch (and thus binary)
> > it should use.
>
> At that level, I think the right answer is "tell the user
> they need to specify the qemu executable they are trying to test".
> In particular, there is no guarantee that the user has actually
> built the executable for the target that corresponds to the
> host, so it doesn't work to try to default to that anyway.
>
> thanks
> -- PMM
>

I agree with Peter.  We can make qemu_bin parameter mandatory.  If it is not
given, error out.  Trying to guess it based on host architecture turns out to be
troublesome.

If we decide to follow this approach of not guessing QEMU binary, we should
update docs/devel/testing.rst to make it crystal clear qemu_bin parameter is
mandatory.

--
Murilo

Re: [Qemu-devel] [PATCH] Acceptance tests: host arch to target arch name mapping

[Eduardo Habkost]

On Wed, Oct 17, 2018 at 04:43:15PM -0300, Murilo Opsfelder Araujo wrote:
> On Wed, Oct 17, 2018 at 07:40:51PM +0100, Peter Maydell wrote:
> > On 17 October 2018 at 18:38, Cleber Rosa <crosa@redhat.com> wrote:
> > >
> > >
> > > On 10/17/18 12:29 PM, Eduardo Habkost wrote:
> > >> On Wed, Oct 17, 2018 at 01:34:41PM +0100, Peter Maydell wrote:
> > >>> So, why does the test code need to care? It's not clear
> > >>> from the patch... My expectation would be that you'd
> > >>> just test all the testable target architectures,
> > >>> regardless of what the host architecture is.
> > >>
> > >> I tend to agree.  Maybe the right solution is to get rid of the
> > >> os.uname().  I think the default should be testing all QEMU
> > >> binaries that were built, and the host architecture shouldn't
> > >> matter.
> >
> > Yes, looking at os.uname() also seems like an odd thing
> > for the tests to be doing here. The test framework
> > should be as far as possible host-architecture agnostic.
> > (For some of the KVM cases there probably is a need to
> > care, but those are exceptions, not the rule.)
> >
> > > I'm in favor of exercising all built targets, but that seems to me to be
> > > on another layer, above the test themselves. This change is about the
> > > behavior of a test when not told about the target arch (and thus binary)
> > > it should use.
> >
> > At that level, I think the right answer is "tell the user
> > they need to specify the qemu executable they are trying to test".
> > In particular, there is no guarantee that the user has actually
> > built the executable for the target that corresponds to the
> > host, so it doesn't work to try to default to that anyway.
> >
> > thanks
> > -- PMM
> >
> 
> I agree with Peter.  We can make qemu_bin parameter mandatory.  If it is not
> given, error out.  Trying to guess it based on host architecture turns out to be
> troublesome.
> 
> If we decide to follow this approach of not guessing QEMU binary, we should
> update docs/devel/testing.rst to make it crystal clear qemu_bin parameter is
> mandatory.

That's not a perfect solution to me, but it sounds better than
using uname() and silently making a decision for the user.

-- 
Eduardo

Re: [Qemu-devel] [PATCH] Acceptance tests: host arch to target arch name mapping

[Wainer dos Santos Moschetta]

On 10/17/2018 05:05 PM, Eduardo Habkost wrote:
> On Wed, Oct 17, 2018 at 04:43:15PM -0300, Murilo Opsfelder Araujo wrote:
>> On Wed, Oct 17, 2018 at 07:40:51PM +0100, Peter Maydell wrote:
>>> On 17 October 2018 at 18:38, Cleber Rosa <crosa@redhat.com> wrote:
>>>>
>>>> On 10/17/18 12:29 PM, Eduardo Habkost wrote:
>>>>> On Wed, Oct 17, 2018 at 01:34:41PM +0100, Peter Maydell wrote:
>>>>>> So, why does the test code need to care? It's not clear
>>>>>> from the patch... My expectation would be that you'd
>>>>>> just test all the testable target architectures,
>>>>>> regardless of what the host architecture is.
>>>>> I tend to agree.  Maybe the right solution is to get rid of the
>>>>> os.uname().  I think the default should be testing all QEMU
>>>>> binaries that were built, and the host architecture shouldn't
>>>>> matter.
>>> Yes, looking at os.uname() also seems like an odd thing
>>> for the tests to be doing here. The test framework
>>> should be as far as possible host-architecture agnostic.
>>> (For some of the KVM cases there probably is a need to
>>> care, but those are exceptions, not the rule.)
>>>
>>>> I'm in favor of exercising all built targets, but that seems to me to be
>>>> on another layer, above the test themselves. This change is about the
>>>> behavior of a test when not told about the target arch (and thus binary)
>>>> it should use.
>>> At that level, I think the right answer is "tell the user
>>> they need to specify the qemu executable they are trying to test".
>>> In particular, there is no guarantee that the user has actually
>>> built the executable for the target that corresponds to the
>>> host, so it doesn't work to try to default to that anyway.
>>>
>>> thanks
>>> -- PMM
>>>
>> I agree with Peter.  We can make qemu_bin parameter mandatory.  If it is not
>> given, error out.  Trying to guess it based on host architecture turns out to be
>> troublesome.
>>
>> If we decide to follow this approach of not guessing QEMU binary, we should
>> update docs/devel/testing.rst to make it crystal clear qemu_bin parameter is
>> mandatory.
> That's not a perfect solution to me, but it sounds better than
> using uname() and silently making a decision for the user.
>

As someone that have been writing acceptance tests recently, I find very 
convenient it picking the qemu binary from current build automatically.

On the other hand, as an CI system or tester (I presume), I would want 
to run tests on all target built. Or at least have control over it.

I don't know if the architectures in question are so broadly used on 
workstations that would justify this patch. Maybe we can stick with 
current uname() approach (and if not able to find the binary, well, you 
should point do it), and rather introduce a mechanism for running tests 
against several targets (satisfying CI needs)?

I hope it helps,
Wainer.

Re: [Qemu-devel] [PATCH] Acceptance tests: host arch to target arch name mapping

[Cleber Rosa]

On 10/17/18 4:33 PM, Wainer dos Santos Moschetta wrote:
> 
> On 10/17/2018 05:05 PM, Eduardo Habkost wrote:
>> On Wed, Oct 17, 2018 at 04:43:15PM -0300, Murilo Opsfelder Araujo wrote:
>>> On Wed, Oct 17, 2018 at 07:40:51PM +0100, Peter Maydell wrote:
>>>> On 17 October 2018 at 18:38, Cleber Rosa <crosa@redhat.com> wrote:
>>>>>
>>>>> On 10/17/18 12:29 PM, Eduardo Habkost wrote:
>>>>>> On Wed, Oct 17, 2018 at 01:34:41PM +0100, Peter Maydell wrote:
>>>>>>> So, why does the test code need to care? It's not clear
>>>>>>> from the patch... My expectation would be that you'd
>>>>>>> just test all the testable target architectures,
>>>>>>> regardless of what the host architecture is.
>>>>>> I tend to agree.  Maybe the right solution is to get rid of the
>>>>>> os.uname().  I think the default should be testing all QEMU
>>>>>> binaries that were built, and the host architecture shouldn't
>>>>>> matter.
>>>> Yes, looking at os.uname() also seems like an odd thing
>>>> for the tests to be doing here. The test framework
>>>> should be as far as possible host-architecture agnostic.
>>>> (For some of the KVM cases there probably is a need to
>>>> care, but those are exceptions, not the rule.)
>>>>
>>>>> I'm in favor of exercising all built targets, but that seems to me
>>>>> to be
>>>>> on another layer, above the test themselves. This change is about the
>>>>> behavior of a test when not told about the target arch (and thus
>>>>> binary)
>>>>> it should use.
>>>> At that level, I think the right answer is "tell the user
>>>> they need to specify the qemu executable they are trying to test".
>>>> In particular, there is no guarantee that the user has actually
>>>> built the executable for the target that corresponds to the
>>>> host, so it doesn't work to try to default to that anyway.
>>>>
>>>> thanks
>>>> -- PMM
>>>>
>>> I agree with Peter.  We can make qemu_bin parameter mandatory.  If it
>>> is not
>>> given, error out.  Trying to guess it based on host architecture
>>> turns out to be
>>> troublesome.
>>>
>>> If we decide to follow this approach of not guessing QEMU binary, we
>>> should
>>> update docs/devel/testing.rst to make it crystal clear qemu_bin
>>> parameter is
>>> mandatory.
>> That's not a perfect solution to me, but it sounds better than
>> using uname() and silently making a decision for the user.
>>
> 
> As someone that have been writing acceptance tests recently, I find very
> convenient it picking the qemu binary from current build automatically.
> 

I can relate to that.  It may not sound like much, but every single
barrier or extra requirement we put on the initial test writing, is
another possible lost test case written.

> On the other hand, as an CI system or tester (I presume), I would want
> to run tests on all target built. Or at least have control over it.
> 

Absolutely.  The control already exists though.  I think the key
decision we need now is:

 * Should an individual test refuse to run and error out unless an
explicit QEMU binary parameter is given? OR

 * Should an individual test default to a host/target binary is one
exists in the system?

> I don't know if the architectures in question are so broadly used on
> workstations that would justify this patch. Maybe we can stick with
> current uname() approach (and if not able to find the binary, well, you
> should point do it), and rather introduce a mechanism for running tests
> against several targets (satisfying CI needs)?
> 

Well, I added all for the sake of comprehensiveness (well, actually got
two wrong as you noticed :).  The tooling to exercise many targets,
machine types, devices, is certainly something we need, but, it's
another layer altogether.

> I hope it helps,
> Wainer.

It sure does!
- Cleber.

Re: [Qemu-devel] [PATCH] Acceptance tests: host arch to target arch name mapping

[Eduardo Habkost]

On Wed, Oct 17, 2018 at 05:10:18PM -0400, Cleber Rosa wrote:
> 
> 
> On 10/17/18 4:33 PM, Wainer dos Santos Moschetta wrote:
> > 
> > On 10/17/2018 05:05 PM, Eduardo Habkost wrote:
> >> On Wed, Oct 17, 2018 at 04:43:15PM -0300, Murilo Opsfelder Araujo wrote:
> >>> On Wed, Oct 17, 2018 at 07:40:51PM +0100, Peter Maydell wrote:
> >>>> On 17 October 2018 at 18:38, Cleber Rosa <crosa@redhat.com> wrote:
> >>>>>
> >>>>> On 10/17/18 12:29 PM, Eduardo Habkost wrote:
> >>>>>> On Wed, Oct 17, 2018 at 01:34:41PM +0100, Peter Maydell wrote:
> >>>>>>> So, why does the test code need to care? It's not clear
> >>>>>>> from the patch... My expectation would be that you'd
> >>>>>>> just test all the testable target architectures,
> >>>>>>> regardless of what the host architecture is.
> >>>>>> I tend to agree.  Maybe the right solution is to get rid of the
> >>>>>> os.uname().  I think the default should be testing all QEMU
> >>>>>> binaries that were built, and the host architecture shouldn't
> >>>>>> matter.
> >>>> Yes, looking at os.uname() also seems like an odd thing
> >>>> for the tests to be doing here. The test framework
> >>>> should be as far as possible host-architecture agnostic.
> >>>> (For some of the KVM cases there probably is a need to
> >>>> care, but those are exceptions, not the rule.)
> >>>>
> >>>>> I'm in favor of exercising all built targets, but that seems to me
> >>>>> to be
> >>>>> on another layer, above the test themselves. This change is about the
> >>>>> behavior of a test when not told about the target arch (and thus
> >>>>> binary)
> >>>>> it should use.
> >>>> At that level, I think the right answer is "tell the user
> >>>> they need to specify the qemu executable they are trying to test".
> >>>> In particular, there is no guarantee that the user has actually
> >>>> built the executable for the target that corresponds to the
> >>>> host, so it doesn't work to try to default to that anyway.
> >>>>
> >>>> thanks
> >>>> -- PMM
> >>>>
> >>> I agree with Peter.  We can make qemu_bin parameter mandatory.  If it
> >>> is not
> >>> given, error out.  Trying to guess it based on host architecture
> >>> turns out to be
> >>> troublesome.
> >>>
> >>> If we decide to follow this approach of not guessing QEMU binary, we
> >>> should
> >>> update docs/devel/testing.rst to make it crystal clear qemu_bin
> >>> parameter is
> >>> mandatory.
> >> That's not a perfect solution to me, but it sounds better than
> >> using uname() and silently making a decision for the user.
> >>
> > 
> > As someone that have been writing acceptance tests recently, I find very
> > convenient it picking the qemu binary from current build automatically.
> > 
> 
> I can relate to that.  It may not sound like much, but every single
> barrier or extra requirement we put on the initial test writing, is
> another possible lost test case written.

Agreed.


> 
> > On the other hand, as an CI system or tester (I presume), I would want
> > to run tests on all target built. Or at least have control over it.
> > 
> 
> Absolutely.  The control already exists though.  I think the key
> decision we need now is:
> 
>  * Should an individual test refuse to run and error out unless an
> explicit QEMU binary parameter is given? OR
> 
>  * Should an individual test default to a host/target binary is one
> exists in the system?

We have a third option: an individual test[1] not erroring out
and testing all QEMU binaries in the current build.  What are the
obstacles for doing that?

[1] By "an individual test" I understand "a command where the
    user indicates that they want to run a single test".

> 
> > I don't know if the architectures in question are so broadly used on
> > workstations that would justify this patch. Maybe we can stick with
> > current uname() approach (and if not able to find the binary, well, you
> > should point do it), and rather introduce a mechanism for running tests
> > against several targets (satisfying CI needs)?
> > 
> 
> Well, I added all for the sake of comprehensiveness (well, actually got
> two wrong as you noticed :).  The tooling to exercise many targets,
> machine types, devices, is certainly something we need, but, it's
> another layer altogether.

Maybe the key difference here is that people probably don't
expect every machine type and device to be always tested, but
they are likely to expect test commands to test all QEMU binaries
in the current build dir.  That's the behavior of all
"make check-*" rules currently.

If this should be considered another layer in the system, that's
fine.  But then we should ensure the UIs/instructions we provide
for running tests talk to that layer.

-- 
Eduardo

Re: [Qemu-devel] [PATCH] Acceptance tests: host arch to target arch name mapping

[Eduardo Habkost]

On Wed, Oct 17, 2018 at 05:33:30PM -0300, Wainer dos Santos Moschetta wrote:
> 
> On 10/17/2018 05:05 PM, Eduardo Habkost wrote:
> > On Wed, Oct 17, 2018 at 04:43:15PM -0300, Murilo Opsfelder Araujo wrote:
> > > On Wed, Oct 17, 2018 at 07:40:51PM +0100, Peter Maydell wrote:
> > > > On 17 October 2018 at 18:38, Cleber Rosa <crosa@redhat.com> wrote:
> > > > > 
> > > > > On 10/17/18 12:29 PM, Eduardo Habkost wrote:
> > > > > > On Wed, Oct 17, 2018 at 01:34:41PM +0100, Peter Maydell wrote:
> > > > > > > So, why does the test code need to care? It's not clear
> > > > > > > from the patch... My expectation would be that you'd
> > > > > > > just test all the testable target architectures,
> > > > > > > regardless of what the host architecture is.
> > > > > > I tend to agree.  Maybe the right solution is to get rid of the
> > > > > > os.uname().  I think the default should be testing all QEMU
> > > > > > binaries that were built, and the host architecture shouldn't
> > > > > > matter.
> > > > Yes, looking at os.uname() also seems like an odd thing
> > > > for the tests to be doing here. The test framework
> > > > should be as far as possible host-architecture agnostic.
> > > > (For some of the KVM cases there probably is a need to
> > > > care, but those are exceptions, not the rule.)
> > > > 
> > > > > I'm in favor of exercising all built targets, but that seems to me to be
> > > > > on another layer, above the test themselves. This change is about the
> > > > > behavior of a test when not told about the target arch (and thus binary)
> > > > > it should use.
> > > > At that level, I think the right answer is "tell the user
> > > > they need to specify the qemu executable they are trying to test".
> > > > In particular, there is no guarantee that the user has actually
> > > > built the executable for the target that corresponds to the
> > > > host, so it doesn't work to try to default to that anyway.
> > > > 
> > > > thanks
> > > > -- PMM
> > > > 
> > > I agree with Peter.  We can make qemu_bin parameter mandatory.  If it is not
> > > given, error out.  Trying to guess it based on host architecture turns out to be
> > > troublesome.
> > > 
> > > If we decide to follow this approach of not guessing QEMU binary, we should
> > > update docs/devel/testing.rst to make it crystal clear qemu_bin parameter is
> > > mandatory.
> > That's not a perfect solution to me, but it sounds better than
> > using uname() and silently making a decision for the user.
> > 
> 
> As someone that have been writing acceptance tests recently, I find very
> convenient it picking the qemu binary from current build automatically.

Picking the QEMU binary(ies) from the current build is a good
idea.

Picking one one arbitrary QEMU binary from the build (e.g. using
uname) and ignoring all the others silently may cause confusion.

At least iotests doesn't do that silently: it prints a message
indicating what exactly is being tested, so people know only
one specific QEMU binary is being tested.

> 
> On the other hand, as an CI system or tester (I presume), I would want to
> run tests on all target built. Or at least have control over it.

I'm not thinking of CI systems, but of the confusion that may
arise from a developer doing this on his machine:

1) Developer runs "./configure --target-list=x86_64-softmmu,ppc64-softmmu"
   on his x86_64 workstation.
2) Developer implements a change that breaks qemu-system-ppc64 crash.
3) Developer runs "make".
4) Developer runs "avocado run tests/acceptance" or
   "./tests/acceptance/mytest.py".
5) Avocado runs tests for qemu-system-x86_64 only.
6) Developer incorrectly believes everything is fine.


> 
> I don't know if the architectures in question are so broadly used on
> workstations that would justify this patch. Maybe we can stick with current
> uname() approach (and if not able to find the binary, well, you should point
> do it), and rather introduce a mechanism for running tests against several
> targets (satisfying CI needs)?
> 
> I hope it helps,
> Wainer.

-- 
Eduardo

Re: [Qemu-devel] [PATCH] Acceptance tests: host arch to target arch name mapping

[Cleber Rosa]

On 10/17/18 5:16 PM, Eduardo Habkost wrote:
> On Wed, Oct 17, 2018 at 05:33:30PM -0300, Wainer dos Santos Moschetta wrote:
>>
>> On 10/17/2018 05:05 PM, Eduardo Habkost wrote:
>>> On Wed, Oct 17, 2018 at 04:43:15PM -0300, Murilo Opsfelder Araujo wrote:
>>>> On Wed, Oct 17, 2018 at 07:40:51PM +0100, Peter Maydell wrote:
>>>>> On 17 October 2018 at 18:38, Cleber Rosa <crosa@redhat.com> wrote:
>>>>>>
>>>>>> On 10/17/18 12:29 PM, Eduardo Habkost wrote:
>>>>>>> On Wed, Oct 17, 2018 at 01:34:41PM +0100, Peter Maydell wrote:
>>>>>>>> So, why does the test code need to care? It's not clear
>>>>>>>> from the patch... My expectation would be that you'd
>>>>>>>> just test all the testable target architectures,
>>>>>>>> regardless of what the host architecture is.
>>>>>>> I tend to agree.  Maybe the right solution is to get rid of the
>>>>>>> os.uname().  I think the default should be testing all QEMU
>>>>>>> binaries that were built, and the host architecture shouldn't
>>>>>>> matter.
>>>>> Yes, looking at os.uname() also seems like an odd thing
>>>>> for the tests to be doing here. The test framework
>>>>> should be as far as possible host-architecture agnostic.
>>>>> (For some of the KVM cases there probably is a need to
>>>>> care, but those are exceptions, not the rule.)
>>>>>
>>>>>> I'm in favor of exercising all built targets, but that seems to me to be
>>>>>> on another layer, above the test themselves. This change is about the
>>>>>> behavior of a test when not told about the target arch (and thus binary)
>>>>>> it should use.
>>>>> At that level, I think the right answer is "tell the user
>>>>> they need to specify the qemu executable they are trying to test".
>>>>> In particular, there is no guarantee that the user has actually
>>>>> built the executable for the target that corresponds to the
>>>>> host, so it doesn't work to try to default to that anyway.
>>>>>
>>>>> thanks
>>>>> -- PMM
>>>>>
>>>> I agree with Peter.  We can make qemu_bin parameter mandatory.  If it is not
>>>> given, error out.  Trying to guess it based on host architecture turns out to be
>>>> troublesome.
>>>>
>>>> If we decide to follow this approach of not guessing QEMU binary, we should
>>>> update docs/devel/testing.rst to make it crystal clear qemu_bin parameter is
>>>> mandatory.
>>> That's not a perfect solution to me, but it sounds better than
>>> using uname() and silently making a decision for the user.
>>>
>>
>> As someone that have been writing acceptance tests recently, I find very
>> convenient it picking the qemu binary from current build automatically.
> 
> Picking the QEMU binary(ies) from the current build is a good
> idea.
> 
> Picking one one arbitrary QEMU binary from the build (e.g. using
> uname) and ignoring all the others silently may cause confusion.
> 
> At least iotests doesn't do that silently: it prints a message
> indicating what exactly is being tested, so people know only
> one specific QEMU binary is being tested.
> 

This is also done in the acceptance tests, the difference is the amount
of logging that by default goes to the UI (and/or the format).  But it's
always in the log:

$ avocado --show=test run tests/acceptance/version.py | grep qemu_bin
PARAMS (key=qemu_bin, path=*, default=x86_64-softmmu/qemu-system-x86_64)
=> 'x86_64-softmmu/qemu-system-x86_64'

>>
>> On the other hand, as an CI system or tester (I presume), I would want to
>> run tests on all target built. Or at least have control over it.
> 
> I'm not thinking of CI systems, but of the confusion that may
> arise from a developer doing this on his machine:
> 
> 1) Developer runs "./configure --target-list=x86_64-softmmu,ppc64-softmmu"
>    on his x86_64 workstation.
> 2) Developer implements a change that breaks qemu-system-ppc64 crash.
> 3) Developer runs "make".
> 4) Developer runs "avocado run tests/acceptance" or
>    "./tests/acceptance/mytest.py".

IMO, this will be done by someone writing mytest.py, and not by someone
hacking QEMU.  Someone hacking QEMU will run "make check-acceptance"...

> 5) Avocado runs tests for qemu-system-x86_64 only.

Which as I said before, is higher level and can be changed to run on all
built targets.  So with that, *this* doesn't happen.

> 6) Developer incorrectly believes everything is fine.
> 
> 

And neither does that.  The error will be flagged.

Again, I just think it's a layer issue and using one simple IMO default
setting at the test level.

- Cleber.

>>
>> I don't know if the architectures in question are so broadly used on
>> workstations that would justify this patch. Maybe we can stick with current
>> uname() approach (and if not able to find the binary, well, you should point
>> do it), and rather introduce a mechanism for running tests against several
>> targets (satisfying CI needs)?
>>
>> I hope it helps,
>> Wainer.
> 

Re: [Qemu-devel] [PATCH] Acceptance tests: host arch to target arch name mapping

[Cleber Rosa]

On 10/17/18 8:34 AM, Peter Maydell wrote:
> On 17 October 2018 at 00:22, Cleber Rosa <crosa@redhat.com> wrote:
>> The host arch name is not always the target arch name, so it's
>> necessary to have a mapping.
>>
>> The configure scripts contains what is the authoritative and failproof
>> mapping, but, reusing it is not straightforward, so it's replicated in
>> the acceptance tests supporting code.
> 
> So, why does the test code need to care? It's not clear
> from the patch... My expectation would be that you'd
> just test all the testable target architectures,
> regardless of what the host architecture is.
> 

That's a good question, and now I realize that I could have given more
information on the commit message (fixing it).

The core issue is that when running tests and a QEMU binary is not
explicitly chosen, we pick one first from the build tree (then from
system) that matches the host architecture.

Without such a mapping, when running tests say on a host arch ppc64le, a
QEMU binary will not be found.  This was first mentioned here:

https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg00865.html

Thanks!
- Cleber.

> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH] Acceptance tests: host arch to target arch name mapping

[Daniel P. Berrangé]

On Wed, Oct 17, 2018 at 12:31:56PM -0400, Cleber Rosa wrote:
> 
> 
> On 10/17/18 8:34 AM, Peter Maydell wrote:
> > On 17 October 2018 at 00:22, Cleber Rosa <crosa@redhat.com> wrote:
> >> The host arch name is not always the target arch name, so it's
> >> necessary to have a mapping.
> >>
> >> The configure scripts contains what is the authoritative and failproof
> >> mapping, but, reusing it is not straightforward, so it's replicated in
> >> the acceptance tests supporting code.
> > 
> > So, why does the test code need to care? It's not clear
> > from the patch... My expectation would be that you'd
> > just test all the testable target architectures,
> > regardless of what the host architecture is.
> > 
> 
> That's a good question, and now I realize that I could have given more
> information on the commit message (fixing it).
> 
> The core issue is that when running tests and a QEMU binary is not
> explicitly chosen, we pick one first from the build tree (then from
> system) that matches the host architecture.
> 
> Without such a mapping, when running tests say on a host arch ppc64le, a
> QEMU binary will not be found.  This was first mentioned here:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg00865.html

The configure script knows what the matching target for this host
arch is. Can you just make it set a makefile variable you can then
pick up ?

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: [Qemu-devel] [PATCH] Acceptance tests: host arch to target arch name mapping

[Cleber Rosa]

On 10/17/18 12:51 PM, Daniel P. Berrangé wrote:
> On Wed, Oct 17, 2018 at 12:31:56PM -0400, Cleber Rosa wrote:
>>
>>
>> On 10/17/18 8:34 AM, Peter Maydell wrote:
>>> On 17 October 2018 at 00:22, Cleber Rosa <crosa@redhat.com> wrote:
>>>> The host arch name is not always the target arch name, so it's
>>>> necessary to have a mapping.
>>>>
>>>> The configure scripts contains what is the authoritative and failproof
>>>> mapping, but, reusing it is not straightforward, so it's replicated in
>>>> the acceptance tests supporting code.
>>>
>>> So, why does the test code need to care? It's not clear
>>> from the patch... My expectation would be that you'd
>>> just test all the testable target architectures,
>>> regardless of what the host architecture is.
>>>
>>
>> That's a good question, and now I realize that I could have given more
>> information on the commit message (fixing it).
>>
>> The core issue is that when running tests and a QEMU binary is not
>> explicitly chosen, we pick one first from the build tree (then from
>> system) that matches the host architecture.
>>
>> Without such a mapping, when running tests say on a host arch ppc64le, a
>> QEMU binary will not be found.  This was first mentioned here:
>>
>> https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg00865.html
> 
> The configure script knows what the matching target for this host
> arch is. Can you just make it set a makefile variable you can then
> pick up ?
> 

That looks like the right approach when doing something like "make
check-acceptance".

But, tying the (Python) test code to information from the generated
Makefiles is something we tried before and was mostly rejected[1].

One strong (new) reason for not having that dependency are the packaged
tests (say a qemu-tests RPM) that should be able to run on environments
such as Fedora CI.

Thanks for the input!
- Cleber.

[1] - http://lists.gnu.org/archive/html/qemu-devel/2017-07/msg06757.html

> Regards,
> Daniel
> 

Re: [Qemu-devel] [PATCH] Acceptance tests: host arch to target arch name mapping

[Wainer dos Santos Moschetta]

Hello Cleber!

On 10/16/2018 08:22 PM, Cleber Rosa wrote:
> The host arch name is not always the target arch name, so it's
> necessary to have a mapping.
>
> The configure scripts contains what is the authoritative and failproof
> mapping, but, reusing it is not straightforward, so it's replicated in
> the acceptance tests supporting code.
>
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
>   configure                                 |  2 ++
>   tests/acceptance/avocado_qemu/__init__.py | 23 +++++++++++++++++++++++
>   2 files changed, 25 insertions(+)
>
> diff --git a/configure b/configure
> index 8af2be959f..e029b756d4 100755
> --- a/configure
> +++ b/configure
> @@ -6992,6 +6992,8 @@ TARGET_ARCH="$target_name"
>   TARGET_BASE_ARCH=""
>   TARGET_ABI_DIR=""
>   
> +# When updating target_name => TARGET_ARCH, please also update the
> +# HOST_TARGET_ARCH mapping in tests/acceptance/avocado_qemu/__init__.py
>   case "$target_name" in
>     i386)
>       mttcg="yes"
> diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
> index 1e54fd5932..d9bc4736ec 100644
> --- a/tests/acceptance/avocado_qemu/__init__.py
> +++ b/tests/acceptance/avocado_qemu/__init__.py
> @@ -19,6 +19,28 @@ sys.path.append(os.path.join(SRC_ROOT_DIR, 'scripts'))
>   
>   from qemu import QEMUMachine
>   
> +
> +#: Mapping of host arch names to target arch names.  It's expected that the
> +#: arch identification on the host, using os.uname()[4], would return the
> +#: key (LHS).  The QEMU target name, and consequently the target binary, would
> +#: be based on the name on the value (RHS).
> +HOST_TARGET_ARCH = {
> +    'armeb': 'arm',
> +    'aarch64_be': 'aarch64',
> +    'microblazeel': 'microblaze',
> +    'mipsel': 'mips',
> +    'mipsn32el' : 'mips64',

Missing mipsn32 mapping to mips64:

   mipsn32|mipsn32el)
     TARGET_ARCH=mips64
     TARGET_BASE_ARCH=mips
     target_compiler=$cross_cc_mipsn32
     echo "TARGET_ABI_MIPSN32=y" >> $config_target_mak
     echo "TARGET_ABI32=y" >> $config_target_mak
   ;;

> +    'mips64el': 'mips64',
> +    'or1k': 'openrisc',
> +    'ppc64le': 'ppc64',
> +    'ppc64abi32': 'ppc64',
> +    'riscv64': 'riscv',

riscv64 is not mapped to riscv:

   riscv64)
     TARGET_BASE_ARCH=riscv
     TARGET_ABI_DIR=riscv
     mttcg=yes
     target_compiler=$cross_cc_riscv64
   ;;

Thanks,
Wainer.

> +    'sh4eb': 'sh4',
> +    'sparc32plus': 'sparc64',
> +    'xtensaeb': 'xtensa'
> +    }
> +
> +
>   def is_readable_executable_file(path):
>       return os.path.isfile(path) and os.access(path, os.R_OK | os.X_OK)
>   
> @@ -29,6 +51,7 @@ def pick_default_qemu_bin():
>       directory or in the source tree root directory.
>       """
>       arch = os.uname()[4]
> +    arch = HOST_TARGET_ARCH.get(arch, arch)
>       qemu_bin_relative_path = os.path.join("%s-softmmu" % arch,
>                                             "qemu-system-%s" % arch)
>       if is_readable_executable_file(qemu_bin_relative_path):

Re: [Qemu-devel] [PATCH] Acceptance tests: host arch to target arch name mapping

[Cleber Rosa]

On 10/17/18 10:54 AM, Wainer dos Santos Moschetta wrote:
> Hello Cleber!
> 
> On 10/16/2018 08:22 PM, Cleber Rosa wrote:
>> The host arch name is not always the target arch name, so it's
>> necessary to have a mapping.
>>
>> The configure scripts contains what is the authoritative and failproof
>> mapping, but, reusing it is not straightforward, so it's replicated in
>> the acceptance tests supporting code.
>>
>> Signed-off-by: Cleber Rosa <crosa@redhat.com>
>> ---
>>   configure                                 |  2 ++
>>   tests/acceptance/avocado_qemu/__init__.py | 23 +++++++++++++++++++++++
>>   2 files changed, 25 insertions(+)
>>
>> diff --git a/configure b/configure
>> index 8af2be959f..e029b756d4 100755
>> --- a/configure
>> +++ b/configure
>> @@ -6992,6 +6992,8 @@ TARGET_ARCH="$target_name"
>>   TARGET_BASE_ARCH=""
>>   TARGET_ABI_DIR=""
>>   +# When updating target_name => TARGET_ARCH, please also update the
>> +# HOST_TARGET_ARCH mapping in tests/acceptance/avocado_qemu/__init__.py
>>   case "$target_name" in
>>     i386)
>>       mttcg="yes"
>> diff --git a/tests/acceptance/avocado_qemu/__init__.py
>> b/tests/acceptance/avocado_qemu/__init__.py
>> index 1e54fd5932..d9bc4736ec 100644
>> --- a/tests/acceptance/avocado_qemu/__init__.py
>> +++ b/tests/acceptance/avocado_qemu/__init__.py
>> @@ -19,6 +19,28 @@ sys.path.append(os.path.join(SRC_ROOT_DIR, 'scripts'))
>>     from qemu import QEMUMachine
>>   +
>> +#: Mapping of host arch names to target arch names.  It's expected
>> that the
>> +#: arch identification on the host, using os.uname()[4], would return
>> the
>> +#: key (LHS).  The QEMU target name, and consequently the target
>> binary, would
>> +#: be based on the name on the value (RHS).
>> +HOST_TARGET_ARCH = {
>> +    'armeb': 'arm',
>> +    'aarch64_be': 'aarch64',
>> +    'microblazeel': 'microblaze',
>> +    'mipsel': 'mips',
>> +    'mipsn32el' : 'mips64',
> 
> Missing mipsn32 mapping to mips64:
> 
>   mipsn32|mipsn32el)
>     TARGET_ARCH=mips64
>     TARGET_BASE_ARCH=mips
>     target_compiler=$cross_cc_mipsn32
>     echo "TARGET_ABI_MIPSN32=y" >> $config_target_mak
>     echo "TARGET_ABI32=y" >> $config_target_mak
>   ;;
> 

Right!

>> +    'mips64el': 'mips64',
>> +    'or1k': 'openrisc',
>> +    'ppc64le': 'ppc64',
>> +    'ppc64abi32': 'ppc64',
>> +    'riscv64': 'riscv',
> 
> riscv64 is not mapped to riscv:
> 
>   riscv64)
>     TARGET_BASE_ARCH=riscv
>     TARGET_ABI_DIR=riscv
>     mttcg=yes
>     target_compiler=$cross_cc_riscv64
>   ;;
> 

Right again!

Thanks catching those mistakes!
- Cleber.

> Thanks,
> Wainer.
> 
>> +    'sh4eb': 'sh4',
>> +    'sparc32plus': 'sparc64',
>> +    'xtensaeb': 'xtensa'
>> +    }
>> +
>> +
>>   def is_readable_executable_file(path):
>>       return os.path.isfile(path) and os.access(path, os.R_OK | os.X_OK)
>>   @@ -29,6 +51,7 @@ def pick_default_qemu_bin():
>>       directory or in the source tree root directory.
>>       """
>>       arch = os.uname()[4]
>> +    arch = HOST_TARGET_ARCH.get(arch, arch)
>>       qemu_bin_relative_path = os.path.join("%s-softmmu" % arch,
>>                                             "qemu-system-%s" % arch)
>>       if is_readable_executable_file(qemu_bin_relative_path):
> 

-- 
Cleber Rosa
[ Sr Software Engineer - Virtualization Team - Red Hat ]
[ Avocado Test Framework - avocado-framework.github.io ]
[  7ABB 96EB 8B46 B94D 5E0F  E9BB 657E 8D33 A5F2 09F3  ]