feat: Add QEMU disk image parameter - #139
Conversation
There was a problem hiding this comment.
Pull request overview
Adds optional QEMU disk-image support for integration tests, including architecture-aware attachment, overlay handling, and ext4 test fixtures.
Changes:
- Propagates
--qemu-diskthrough the QEMU plugin and process. - Adds disk-image generation and integration coverage.
- Pins the required imagefs dependency.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Summary | Review notes |
|---|---|---|
test/unit/test_qemu.py |
Tests disk argument generation. | Rename the read-only test or add a real read-only assertion. |
test/resources/qemu_disk_content.txt |
Provides disk fixture content. | — |
test/resources/BUILD |
Builds the ext4 disk image. | — |
test/integration/test_qemu_disk.py |
Tests disk visibility, mounting, and writing. | — |
test/integration/BUILD |
Registers disk integration tests. | Add the Linux compatibility constraint, including at line 280. |
score/itf/plugins/qemu/qemu.py |
Builds architecture-aware disk arguments. | — |
score/itf/plugins/qemu/qemu_target.py |
Forwards disk configuration. | — |
score/itf/plugins/qemu/qemu_process.py |
Passes and logs the disk image. | — |
score/itf/plugins/qemu/__init__.py |
Adds CLI handling and overlay lifecycle. | Ensure both overlay creations are covered by cleanup handling. |
MODULE.bazel |
Pins the required imagefs revision. | — |
Suppressed comments (2)
score/itf/plugins/qemu/qemu.py:175
- The new architecture-dependent ordering is not covered by the unit tests: the combined test calls
__disk_argsdirectly and never exercises__disks_args, including the aarch64 reversal. Add unit assertions through__disks_args()for both machines so a regression in the/dev/vda//dev/vdbmapping is caught without relying only on the manual integration tests.
if self.__arch_config["block_device_order"] == "descending":
disks = list(reversed(disks))
test/integration/BUILD:280
- This test depends on
//test/resources:qemu_disk_image, which is explicitly Linux-only (test/resources/BUILD:115). Without a matchingtarget_compatible_withhere, analyzing or building this integration target on a non-Linux platform fails on the incompatible data dependency instead of skipping it; add the Linux constraint used by the other host-dependent tests.
py_itf_test(
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
For running integration tests the binaries and their data have to be made available in the image. Mounting the data via an extra image is the easiest and fastest way. It has lesser system requirements compared to e.g. mounting and modifying a QCOW2 image.
d2fbe5b to
94f579b
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The optional parameter currently introduces breaking requirements across public QEMU APIs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 12/13 changed files
- Comments generated: 4
- Review effort level: Balanced
| port_forwarding, | ||
| rootfs, | ||
| kernel_cmdline, | ||
| disk, |
There was a problem hiding this comment.
it is optional at the interface of __init__.py, where the default is None. IMHO carrying this into each and every function call with default parameters, makes the code more complicated.
| machine, | ||
| rootfs, | ||
| kernel_cmdline, | ||
| disk, |
There was a problem hiding this comment.
it is optional at the interface of __init__.py, where the default is None. IMHO carrying this into each and every function call with default parameters, makes the code more complicated.
| machine=test_config.qemu_config.qemu_machine, | ||
| rootfs=test_config.qemu_rootfs, | ||
| kernel_cmdline=test_config.qemu_config.qemu_kernel_cmdline, | ||
| disk=test_config.qemu_disk, |
There was a problem hiding this comment.
it is optional at the interface of __init__.py, where the default is None. IMHO carrying this into each and every function call with default parameters, makes the code more complicated.
| ) | ||
| use_repo(imagefs, "score_qnx_x86_64_ifs_toolchain") | ||
| imagefs.toolchain( | ||
| name = "score_qnx_x86_64_ext4_toolchain", |
github-project-automation
Bot
moved this from Backlog
to Done
in TST - Testing Community
For running integration tests the binaries and their data have to be made available in the image.
Mounting the data via an extra image is the easiest and fastest way. It has lesser system requirements compared to e.g. mounting and modifying a QCOW2 image.
Depends on eclipse-score/rules_imagefs#16