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