Message ID | 20190117185628.21862-1-crosa@redhat.com |
---|---|
Headers | show |
Series | Acceptance Tests: target architecture support | expand |
> From: Cleber Rosa <crosa@redhat.com> > class My(Test): > def test_nx_cpu_flag(self): > """ > :avocado: tags=arch:x86_64 > """ > test_code() > The value of the "arch" key, in this case, "x86_64" will be used when > selecting the QEMU binary to use in the test. At the same time, if > "x86_64-softmmu" is not a built target, the test will be filtered out > by "make check-acceptance"[3]. I think, the term "arch" is a little problematic in QEMU parlance. IMHO, "target" should be used instead. ("arch" is used in Linux kernel community) The overall structure of the "tags" should be a little different. My suggestion: "target" "isa" (instruction set architecture, determeines how the kernel and rootfs are built) "cpu" "machine" This would allow clear view what a particular acceptance test tests, and will enforce consistency and clarity in the test organization across the board. That said, I am very excited about this series. Regards, Aleksandar
Hi Aleksandar, On 1/21/19 11:15 PM, Aleksandar Markovic wrote: >> From: Cleber Rosa <crosa@redhat.com> > >> class My(Test): >> def test_nx_cpu_flag(self): >> """ >> :avocado: tags=arch:x86_64 >> """ >> test_code() > >> The value of the "arch" key, in this case, "x86_64" will be used when >> selecting the QEMU binary to use in the test. At the same time, if >> "x86_64-softmmu" is not a built target, the test will be filtered out >> by "make check-acceptance"[3]. > > I think, the term "arch" is a little problematic in QEMU parlance. IMHO, > "target" should be used instead. ("arch" is used in Linux kernel community) Using target_arch/host_arch might be more explicit, but host_arch is not very relevant regarding Avocado (except to choose the correct TCG binary), so usually arch implies target_arch. I mean, it is unlikely you enforce --host_arch on the command line to run tests, this using --arch instead of --target_arch looks OK to me, since host_arch can be guessed. > > The overall structure of the "tags" should be a little different. My > suggestion: > > "target" > "isa" (instruction set architecture, determeines how the kernel and rootfs are built) In QEMU, "isa" is implicit with CPU, isnt' it? I.e. I use: $ mips64el-softmmu/qemu-system-mips64el -M Malta -cpu mips64dspr2 > "cpu" > "machine" > > This would allow clear view what a particular acceptance test tests, and will > enforce consistency and clarity in the test organization across the board. Maybe the problem you are pointing out is not Avocado test organization but QEMU CPU organization... (which also bother me with boards able to use different SoC, with different peripherals or cpus). What is your idea on passing arch/cpu/isa to start QEMU? > > That said, I am very excited about this series. Me too :) Regards, Phil.
On 1/22/19 5:48 AM, Philippe Mathieu-Daudé wrote: > Hi Aleksandar, > > On 1/21/19 11:15 PM, Aleksandar Markovic wrote: >>> From: Cleber Rosa <crosa@redhat.com> >> >>> class My(Test): >>> def test_nx_cpu_flag(self): >>> """ >>> :avocado: tags=arch:x86_64 >>> """ >>> test_code() >> >>> The value of the "arch" key, in this case, "x86_64" will be used when >>> selecting the QEMU binary to use in the test. At the same time, if >>> "x86_64-softmmu" is not a built target, the test will be filtered out >>> by "make check-acceptance"[3]. >> >> I think, the term "arch" is a little problematic in QEMU parlance. IMHO, >> "target" should be used instead. ("arch" is used in Linux kernel community) > > Using target_arch/host_arch might be more explicit, but host_arch is not > very relevant regarding Avocado (except to choose the correct TCG > binary), so usually arch implies target_arch. > > I mean, it is unlikely you enforce --host_arch on the command line to > run tests, this using --arch instead of --target_arch looks OK to me, > since host_arch can be guessed. > Naming things is hard, so this is a valid discussion. But, I have to say that I also find "arch" in this context to be descriptive enough. >> >> The overall structure of the "tags" should be a little different. My >> suggestion: >> >> "target" >> "isa" (instruction set architecture, determeines how the kernel and rootfs are built) > > In QEMU, "isa" is implicit with CPU, isnt' it? > > I.e. I use: > > $ mips64el-softmmu/qemu-system-mips64el -M Malta -cpu mips64dspr2 > >> "cpu" >> "machine" >> >> This would allow clear view what a particular acceptance test tests, and will >> enforce consistency and clarity in the test organization across the board. > > Maybe the problem you are pointing out is not Avocado test organization > but QEMU CPU organization... (which also bother me with boards able to > use different SoC, with different peripherals or cpus). > > What is your idea on passing arch/cpu/isa to start QEMU? > >> >> That said, I am very excited about this series. > > Me too :) > > Regards, > > Phil. > Thanks! For the encouragement, help and input! - Cleber.
Patchew URL: https://patchew.org/QEMU/20190117185628.21862-1-crosa@redhat.com/ Hi, This series seems to have some coding style problems. See output below for more information: Subject: [Qemu-devel] [PATCH 00/18] Acceptance Tests: target architecture support Type: series Message-id: 20190117185628.21862-1-crosa@redhat.com === TEST SCRIPT BEGIN === #!/bin/bash git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test' a27efa3fec Boot Linux Console Test: add a test for alpha + clipper 9c5d101f9b Boot Linux Console Test: add a test for s390x + s390-ccw-virtio 7d584982d7 Boot Linux Console Test: add a test for arm + virt 039f09b5e5 Boot Linux Console Test: add a test for aarch64 + virt a52a2dd405 Boot Linux Console Test: add a test for ppc64 + pseries 1995edd0d0 Boot Linux Console Test: add a test for mips64el + malta cbd4fb29d7 Boot Linux Console Test: add a test for mips + malta 62ca82a551 scripts/qemu.py: support adding a console with the default serial device 8f31626ddd Boot Linux Console Test: refactor the console watcher into utility method c257e982f1 Boot Linux Console Test: update the x86_64 kernel 9859ac0034 Boot Linux Console Test: rename the x86_64 after the arch and machine 4fa0bc613a Acceptance tests: look for target architecture in test tags first 6051a67e66 Acceptance tests: use "arch:" tag to filter target specific tests 8e86083ca9 Acceptance tests: introduce arch parameter and attribute 6ec8dc9d2e Acceptance tests: fix doc reference to avocado_qemu directory 5c8a9897ba Acceptance tests: improve docstring on pick_default_qemu_bin() 0e98f722c6 Acceptance tests: show avocado test execution by default 7f31c568c2 scripts/qemu.py: log QEMU launch command line === OUTPUT BEGIN === 1/18 Checking commit 7f31c568c2e0 (scripts/qemu.py: log QEMU launch command line) 2/18 Checking commit 0e98f722c669 (Acceptance tests: show avocado test execution by default) 3/18 Checking commit 5c8a9897ba6a (Acceptance tests: improve docstring on pick_default_qemu_bin()) 4/18 Checking commit 6ec8dc9d2eed (Acceptance tests: fix doc reference to avocado_qemu directory) 5/18 Checking commit 8e86083ca9e8 (Acceptance tests: introduce arch parameter and attribute) WARNING: line over 80 characters #96: FILE: tests/acceptance/avocado_qemu/__init__.py:58: + default=pick_default_qemu_bin(self.arch)) total: 0 errors, 1 warnings, 64 lines checked Patch 5/18 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 6/18 Checking commit 6051a67e66ea (Acceptance tests: use "arch:" tag to filter target specific tests) 7/18 Checking commit 4fa0bc613a2e (Acceptance tests: look for target architecture in test tags first) 8/18 Checking commit 9859ac003400 (Boot Linux Console Test: rename the x86_64 after the arch and machine) 9/18 Checking commit c257e982f129 (Boot Linux Console Test: update the x86_64 kernel) 10/18 Checking commit 8f31626ddd45 (Boot Linux Console Test: refactor the console watcher into utility method) 11/18 Checking commit 62ca82a551c2 (scripts/qemu.py: support adding a console with the default serial device) 12/18 Checking commit cbd4fb29d7c8 (Boot Linux Console Test: add a test for mips + malta) 13/18 Checking commit 1995edd0d009 (Boot Linux Console Test: add a test for mips64el + malta) ERROR: line over 90 characters #66: FILE: tests/acceptance/boot_linux_console.py:95: + [1] https://kernel-team.pages.debian.net/kernel-handbook/ch-common-tasks.html#s-common-official ERROR: line over 90 characters #67: FILE: tests/acceptance/boot_linux_console.py:96: + [2] http://snapshot.debian.org/package/linux-2.6/2.6.32-48/#linux-source-2.6.32_2.6.32-48 total: 2 errors, 0 warnings, 58 lines checked Patch 13/18 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 14/18 Checking commit a52a2dd405ae (Boot Linux Console Test: add a test for ppc64 + pseries) 15/18 Checking commit 039f09b5e5a4 (Boot Linux Console Test: add a test for aarch64 + virt) 16/18 Checking commit 7d584982d75c (Boot Linux Console Test: add a test for arm + virt) 17/18 Checking commit 9c5d101f9bfb (Boot Linux Console Test: add a test for s390x + s390-ccw-virtio) 18/18 Checking commit a27efa3fec59 (Boot Linux Console Test: add a test for alpha + clipper) === OUTPUT END === Test command exited with code: 1 The full log is available at http://patchew.org/logs/20190117185628.21862-1-crosa@redhat.com/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
>> I think, the term "arch" is a little problematic in QEMU parlance. IMHO, >> "target" should be used instead. ("arch" is used in Linux kernel community) > Naming things is hard, so this is a valid discussion. But, I have to > say that I also find "arch" in this context to be descriptive enough. This is not a "naming" problem, but a fundamental terminology convention used in QEMU. I am truly dissapointed that you chose to disrespect it. Aleksandar
On 2/1/19 12:32 AM, Aleksandar Markovic wrote: >>> I think, the term "arch" is a little problematic in QEMU parlance. IMHO, >>> "target" should be used instead. ("arch" is used in Linux kernel community) > >> Naming things is hard, so this is a valid discussion. But, I have to >> say that I also find "arch" in this context to be descriptive enough. > > This is not a "naming" problem, but a fundamental terminology convention used in QEMU. I am truly dissapointed that you chose to disrespect it. > > Aleksandar > Hi Aleksandar, I'm basically listening to more experienced people on the matter, as I clearly lack the background on the "terminology conventions", and thus, I'm unable to make the best naming choices by myself. Now, we have had participation from few people, and there doesn't seem to be consensus. So, to make it clear, I didn't *choose* to disrespect it, I chose to respect how I perceived the community input. If this makes you truly disappointed, please ask more people to give their input, and I'll gladly steer the direction of this humble ship, I mean, patch series. :) Regards, - Cleber.