| Message ID | 20181016232201.16829-1-crosa@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Acceptance tests: host arch to target arch name mapping | expand |
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.
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
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):
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. >
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.
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 >
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
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.
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 >
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): >
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
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 >
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).
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
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.
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
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?
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.
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.
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
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.
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 >
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.
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.
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.
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. >
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.
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?
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.
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.
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".
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!
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):
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(+)