Acceptance tests: host arch to target arch name mapping

Hi Cleber,

Quoted 21 lines · 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 <[email protected]>

---
 configure                                 |  2 ++
 tests/acceptance/avocado_qemu/__init__.py | 23 +++++++++++++++++++++++
 2 files changed, 25 insertions(+)

configure +0 -0

index 8af2be959f..e029b756d4 100755
@@ -6992,6 +6992,8 @@ TARGET_ARCH="$target_name"
 TARGET_BASE_ARCH=""
 TARGET_ABI_DIR=""
 

tests/acceptance/avocado_qemu/__init__.py

Quoted 9 lines

configure +2 -0

+# 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"

tests/acceptance/avocado_qemu/__init__.py +0 -0

index 1e54fd5932..d9bc4736ec 100644

I'd put this in scripts/qemu.py

tests/acceptance/avocado_qemu/__init__.py sys.path.append(os.path.join(SRC_ROOT_DIR, 'scripts')) · line 29

Quoted 12 lines

tests/acceptance/avocado_qemu/__init__.py +8 -0

@@ -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'},

tests/acceptance/avocado_qemu/__init__.py sys.path.append(os.path.join(SRC_ROOT_DIR, 'scripts')) · line 41

Quoted 12 lines

tests/acceptance/avocado_qemu/__init__.py +12 -0

+    '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]

tests/acceptance/avocado_qemu/__init__.py def pick_default_qemu_bin(): · line 54

Quoted 10 lines

tests/acceptance/avocado_qemu/__init__.py +3 -0

+
+
 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])

tests/acceptance/avocado_qemu/__init__.py def pick_default_qemu_bin(): · line 57

Quoted 4 lines

tests/acceptance/avocado_qemu/__init__.py +0 -0

     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.

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.

Quoted 3 lines · Cleber Rosa

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

Quoted 3 lines

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.

Quoted 4 lines · Peter Maydell

Quoted 3 lines · Cleber Rosa

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

Quoted 8 lines

Quoted 3 lines

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.

Quoted 6 lines · Eduardo Habkost

Quoted 4 lines · Peter Maydell

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.

Quoted 4 lines

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.

Quoted 9 lines · Cleber Rosa

Quoted 6 lines · Eduardo Habkost

Quoted 4 lines · Peter Maydell

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.

Quoted 4 lines

Quoted 4 lines

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.)

Quoted 4 lines

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

Quoted 6 lines · Peter Maydell

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

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.

Quoted 9 lines · Cleber Rosa

Quoted 6 lines · Eduardo Habkost

Quoted 4 lines · Peter Maydell

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.

Quoted 4 lines

Quoted 4 lines

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.)

Quoted 4 lines

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.

Quoted 26 lines · Peter Maydell

Quoted 9 lines · Cleber Rosa

Quoted 6 lines · Eduardo Habkost

Quoted 4 lines · Peter Maydell

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.

Quoted 4 lines

Quoted 4 lines

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.)

Quoted 4 lines

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.

Quoted 5 lines

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).

Quoted 33 lines · Eduardo Habkost

Quoted 26 lines · Peter Maydell

Quoted 9 lines · Cleber Rosa

Quoted 6 lines · Eduardo Habkost

Quoted 4 lines · Peter Maydell

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.

Quoted 4 lines

Quoted 4 lines

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.)

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 4 lines

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.

Quoted 5 lines

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?

Quoted 50 lines · Cleber Rosa

Quoted 33 lines · Eduardo Habkost

Quoted 26 lines · Peter Maydell

Quoted 9 lines · Cleber Rosa

Quoted 6 lines · Eduardo Habkost

Quoted 4 lines · Peter Maydell

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.

Quoted 4 lines

Quoted 4 lines

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.)

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 4 lines

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.

Quoted 5 lines

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).

Quoted 3 lines

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?

Quoted 55 lines · Eduardo Habkost

Quoted 53 lines · Cleber Rosa

Quoted 33 lines · Eduardo Habkost

Quoted 26 lines · Peter Maydell

Quoted 9 lines · Cleber Rosa

Quoted 6 lines · Eduardo Habkost

Quoted 4 lines · Peter Maydell

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.

Quoted 4 lines

Quoted 4 lines

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.)

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 4 lines

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.

Quoted 5 lines

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?

Quoted 3 lines

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).

Quoted 3 lines

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).

Quoted 5 lines

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.

Quoted 4 lines

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.

---

Quoted 3 lines

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.

Quoted 58 lines · Cleber Rosa

Quoted 55 lines · Eduardo Habkost

Quoted 53 lines · Cleber Rosa

Quoted 33 lines · Eduardo Habkost

Quoted 26 lines · Peter Maydell

Quoted 9 lines · Cleber Rosa

Quoted 6 lines · Eduardo Habkost

Quoted 4 lines · Peter Maydell

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.

Quoted 4 lines

Quoted 4 lines

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.)

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 4 lines

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.

Quoted 5 lines

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?

Quoted 6 lines

Quoted 3 lines

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.

Quoted 30 lines

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).

Quoted 3 lines

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).

Quoted 5 lines

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?)

Quoted 3 lines

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).

Quoted 9 lines

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 14 lines

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

Quoted 8 lines

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.

Quoted 6 lines

---

Quoted 3 lines

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

Quoted 4 lines

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.

Quoted 105 lines · Eduardo Habkost

Quoted 58 lines · Cleber Rosa

Quoted 55 lines · Eduardo Habkost

Quoted 53 lines · Cleber Rosa

Quoted 33 lines · Eduardo Habkost

Quoted 26 lines · Peter Maydell

Quoted 9 lines · Cleber Rosa

Quoted 6 lines · Eduardo Habkost

Quoted 4 lines · Peter Maydell

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.

Quoted 4 lines

Quoted 4 lines

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.)

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 4 lines

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.

Quoted 5 lines

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?

Quoted 6 lines

Quoted 3 lines

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.

Quoted 30 lines

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).

Quoted 3 lines

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).

Quoted 5 lines

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?)

Quoted 3 lines

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.

Quoted 6 lines

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.

Quoted 7 lines

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)".

Quoted 9 lines

Quoted 9 lines

Quoted 4 lines

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.

Quoted 7 lines

Quoted 3 lines

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.

Quoted 24 lines

Quoted 15 lines

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

Quoted 8 lines

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.

Quoted 9 lines

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.

Quoted 11 lines

Quoted 10 lines

---

Quoted 3 lines

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.

Quoted 1 line

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".

Quoted 7 lines

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?

Quoted 26 lines · Peter Maydell

Quoted 9 lines · Cleber Rosa

Quoted 6 lines · Eduardo Habkost

Quoted 4 lines · Peter Maydell

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.

Quoted 4 lines

Quoted 4 lines

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.)

Quoted 4 lines

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.

Quoted 5 lines

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

Quoted 37 lines · Eduardo Habkost

Quoted 26 lines · Peter Maydell

Quoted 9 lines · Cleber Rosa

Quoted 6 lines · Eduardo Habkost

Quoted 4 lines · Peter Maydell

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.

Quoted 4 lines

Quoted 4 lines

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.)

Quoted 4 lines

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.

Quoted 5 lines

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?

Quoted 8 lines

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

On 10/17/18 4:46 PM, Murilo Opsfelder Araujo wrote:

Quoted 11 lines

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.

Quoted 1 line · Cleber Rosa

On 10/17/18 4:46 PM, Murilo Opsfelder Araujo wrote:

[...]

Quoted 16 lines

Quoted 11 lines

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.

Quoted 2 lines

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?

Quoted 19 lines · Eduardo Habkost

Quoted 1 line · Cleber Rosa

On 10/17/18 4:46 PM, Murilo Opsfelder Araujo wrote:

[...]

Quoted 16 lines

Quoted 11 lines

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 6 lines

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.

Quoted 9 lines · Cleber Rosa

Quoted 6 lines · Eduardo Habkost

Quoted 4 lines · Peter Maydell

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.

Quoted 4 lines

Quoted 4 lines

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.)

Quoted 4 lines

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

Quoted 26 lines · Peter Maydell

Quoted 9 lines · Cleber Rosa

Quoted 6 lines · Eduardo Habkost

Quoted 4 lines · Peter Maydell

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.

Quoted 4 lines

Quoted 4 lines

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.)

Quoted 4 lines

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.

Quoted 9 lines

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.

Quoted 36 lines · Murilo Opsfelder Araujo

Quoted 22 lines · Peter Maydell

Quoted 11 lines · Cleber Rosa

Quoted 9 lines · Eduardo Habkost

Quoted 4 lines · Peter Maydell

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.)

Quoted 4 lines

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.

Quoted 9 lines

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.

Quoted 3 lines

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.

Quoted 46 lines · Eduardo Habkost

Quoted 45 lines · Murilo Opsfelder Araujo

Quoted 24 lines · Peter Maydell

Quoted 11 lines · Cleber Rosa

Quoted 9 lines · Eduardo Habkost

Quoted 4 lines · Peter Maydell

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.)

Quoted 6 lines

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.

Quoted 9 lines

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.

Quoted 3 lines

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.

Quoted 52 lines · Wainer dos Santos Moschetta

Quoted 49 lines · Eduardo Habkost

Quoted 40 lines · Murilo Opsfelder Araujo

Quoted 24 lines · Peter Maydell

Quoted 11 lines · Cleber Rosa

Quoted 9 lines · Eduardo Habkost

Quoted 4 lines · Peter Maydell

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.)

Quoted 6 lines

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.

Quoted 9 lines

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.

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 4 lines

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?

Quoted 7 lines

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.

Quoted 40 lines · Eduardo Habkost

Quoted 39 lines · Murilo Opsfelder Araujo

Quoted 22 lines · Peter Maydell

Quoted 11 lines · Cleber Rosa

Quoted 9 lines · Eduardo Habkost

Quoted 4 lines · Peter Maydell

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.)

Quoted 4 lines

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.

Quoted 9 lines

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.

Quoted 3 lines

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.

Quoted 42 lines · Wainer dos Santos Moschetta

Quoted 40 lines · Eduardo Habkost

Quoted 39 lines · Murilo Opsfelder Araujo

Quoted 22 lines · Peter Maydell

Quoted 11 lines · Cleber Rosa

Quoted 9 lines · Eduardo Habkost

Quoted 4 lines · Peter Maydell

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.)

Quoted 4 lines

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.

Quoted 9 lines

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.

Quoted 6 lines

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 9 lines

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.

Quoted 3 lines · Cleber Rosa

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

Quoted 3 lines

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

Quoted 4 lines · Peter Maydell

Quoted 3 lines · Cleber Rosa

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

Quoted 9 lines

Quoted 3 lines

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

Quoted 24 lines · Cleber Rosa

Quoted 4 lines · Peter Maydell

Quoted 3 lines · Cleber Rosa

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

Quoted 9 lines

Quoted 3 lines

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.

Quoted 4 lines

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

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 <[email protected]>

---
  configure                                 |  2 ++
  tests/acceptance/avocado_qemu/__init__.py | 23 +++++++++++++++++++++++
  2 files changed, 25 insertions(+)

configure +2 -0

index 8af2be959f..e029b756d4 100755
@@ -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"

tests/acceptance/avocado_qemu/__init__.py +1 -0

index 1e54fd5932..d9bc4736ec 100644
@@ -19,6 +19,28 @@ sys.path.append(os.path.join(SRC_ROOT_DIR, 'scripts'))
 
  from qemu import QEMUMachine
 
+

tests/acceptance/avocado_qemu/__init__.py +7 -0

+    '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):

Hello Cleber!

Quoted 39 lines · 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 <[email protected]>

---
  configure                                 |  2 ++ tests/acceptance/avocado_qemu/__init__.py | 23 +++++++++++++++++++++++

  2 files changed, 25 insertions(+)

configure +0 -0

index 8af2be959f..e029b756d4 100755
@@ -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"

tests/acceptance/avocado_qemu/__init__.py +0 -0

index 1e54fd5932..d9bc4736ec 100644
@@ -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

tests/acceptance/avocado_qemu/__init__.py sys.path.append(os.path.join(SRC_ROOT_DIR, 'scripts')) · line 32

Quoted 7 lines

tests/acceptance/avocado_qemu/__init__.py +7 -0

+#: 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 ;;

Quoted 5 lines

+    '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.

Quoted 16 lines

+    'sh4eb': 'sh4',
+    'sparc32plus': 'sparc64',
+    'xtensaeb': 'xtensa'
+    }

tests/acceptance/avocado_qemu/__init__.py +2 -0

+
+

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