Conversation
Review dismissed automatically
d4a5e87 to
237f1b0
Compare
237f1b0 to
63cd5c4
Compare
|
@jfly This should be ready for review. |
|
Thanks for the ping, @booxter. Unfortunately, I'm really swamped this week, will try to get to it, but wouldn't be surprised if it slips to next week. |
|
@jfly np! It's nixpkgs, next month is fine too. ;) |
63cd5c4 to
25f63ae
Compare
Logind reports CanGraphical=no for an nspawn seat because the container has no display hardware. LightDM consequently stops after "Monitoring logind for seats" and never starts X. The dummy X server does not need display hardware, so disable LightDM's graphical-seat check for nspawn tests. Assisted-by: Codex gpt-5.6-sol xhigh
The USER and HOME assignments currently only apply to the `source' command. They are consequently absent from the commands run after the profile has been loaded. Export them so the complete nspawn command runs with the intended shell environment. Assisted-by: Codex gpt-5.6-sol xhigh
The QEMU backdoor provides DISPLAY=:0.0, but commands run through nspawn have no DISPLAY. X11 clients consequently fail with "unable to open display" even after the X server is running. Set DISPLAY for nspawn commands as well. Assisted-by: Codex gpt-5.6-sol xhigh
wait_for_x() is only implemented by QemuMachine, so an nspawn test that
uses it fails type checking with:
NspawnMachine has no attribute wait_for_x
Provide a backend-neutral implementation that probes the X root window.
Keep QEMU's existing graphical-target check as its override.
Assisted-by: Codex gpt-5.6-sol xhigh
nspawn machines have no QEMU monitor, so the shared screenshot helper
currently fails with:
Screenshots are not supported by NspawnMachine
Capture the root window with xwd into the shared directory, then convert
it to PPM on the host for the existing PNG and OCR paths. Install xwd
only in instrumented nspawn containers.
Assisted-by: Codex gpt-5.6-sol xhigh
Keyboard input helpers currently live on QemuMachine and send keys through the QEMU monitor. Tests moved to nspawn must consequently know about the backend and invoke xdotool themselves. uinput would require exposing the host's /dev/uinput to the sandbox. Its device belongs to the shared kernel input subsystem rather than to a container-private kernel, adding a privileged host dependency. https://docs.kernel.org/input/uinput.html Define send_key on BaseMachine and move the backend-neutral send_chars helper there. Keep QEMU input unchanged, and translate the portable key vocabulary to X11 key names for nspawn. Install xdotool in instrumented nspawn containers. This lets tests continue to express keyboard input through the machine interface regardless of backend. Assisted-by: Codex gpt-5.6-sol high
The test invokes xdotool directly for two key presses. Use the shared machine input interface so the test does not depend on its backend. Assisted-by: Codex gpt-5.6-sol high
The ALSA recorder is backed by the snd_aloop kernel module. ALSA devices are not namespaced, so making this device available to nspawn would require loading it in the shared host kernel and passing its host-global /dev/snd nodes into the container. https://man7.org/linux/man-pages/man7/namespaces.7.html Use a system-wide PulseAudio null sink and capture its monitor source. This keeps audio generation and capture in user space and uses the same path independently of the test driver. Assisted-by: Codex gpt-5.6-sol xhigh
Firefox exercises LightDM and X11, plays and records audio, and takes a screenshot. The preceding changes provide these facilities without depending on VM devices. Switch the existing test to nspawn. Non-scientific benchmark: 75.75s -> 56.47s improvement. Assisted-by: Codex gpt-5.6-sol xhigh
The test only requires X11 and does not rely on VM-specific functionality. Run it in a container now that nspawn tests support X11 applications. virtualisation.diskSize only sizes the QEMU disk image. Remove its Jellyfin override because nspawn stores the container root in a directory on the builder filesystem rather than in a fixed-size virtual disk. Assisted-by: Codex gpt-5.6-sol xhigh
The test exercises Firefox through X11 and its profile files. With input delivered through xdotool, it no longer depends on QEMU-specific functionality. Switch the machine to nspawn. Assisted-by: Codex gpt-5.6-sol xhigh
Extract host graphical-display detection into display.py so QEMU and future display viewers use the same X11 and Wayland availability check. Assisted-by: Codex gpt-5.6-sol high
Move driver configuration models into config.py and give QEMU and nspawn separate model types, mirrored by the Nix option schema. This provides a place for backend-specific settings without exposing them to every machine. Assisted-by: Codex gpt-5.6-sol high
Add typed display-target metadata to test machines and mark the common X11 test module as providing :0. Pass container targets to the driver so interactive tooling can act only on machines that expose a display. Assisted-by: Codex gpt-5.6-sol high
Add configuration for nspawn display exporters and host display viewers. Separating exporters by backend and viewers by protocol allows to easily expand the list of supported protocols and viewers (e.g. RDP or SPICE). Assisted-by: Codex gpt-5.6-sol high
Only for Linux nspawn interactive driver, configure x11vnc and socat relay + remote-viewer. QEMU is not implemented. Assisted-by: Codex gpt-5.6-sol high
Display sessions are modeled independently so that if later we want to add support for a different protocol (SPICE or RDP), we can do it by merely adding a new viewer type implementation. Assisted-by: Codex gpt-5.6-sol high
Implement an nspawn X11 exporter that starts x11vnc inside the container and relays viewer connections from host namespace into test container with nsenter and socat. Assisted-by: Codex gpt-5.6-sol high
|
Thanks for the comment, I think I better understand your ideas. On vkms / uinput / alsa: sadly, those are not namespaced. One would need to put additional demands on the host and then pass devices into test containers (in case of On running nspawn tests on Darwin: I like your suggestion to run nspawn tests in a qemu. This should be more robust than guaranteeing that each and every test case would pass for different backends. I think we should explore this idea. I've updated the series as follows:
Looking forward to your comments on the remaining patches - I think there may be some dragons there because of the way I build up abstractions that are expressive enough for later expansion of display types and exporters (I think we may want to - later - express the current Let me know. |
d458196 to
6a35ec2
Compare
Explain how QEMU and nspawn present graphical tests, including the X11 and Wayland environment that sudo must preserve for container viewers. Document the separation between display targets, exporters, endpoints, viewers, and sessions, including the path to future display backends. Also remove the outdated claim that only VMs support X11 tests. Assisted-by: Codex gpt-5.6-sol high
Assisted-by: Codex gpt-5.6-sol high
6a35ec2 to
2a94326
Compare
jfly
left a comment
There was a problem hiding this comment.
I read through "nixos/test-driver: model display exporters and viewers".
Next I have some time, I will start reading again at "nixos/testing: configure interactive X11 export through VNC"
| display: str = ":0" | ||
| xauthority: Path = Path("/root/.Xauthority") |
There was a problem hiding this comment.
nit: I'm not sure the default values here are a good idea. I checked, and the surrounding nix code that generates this config should always embed these values. Including defaults here just feels like a recipe for getting bit by a typo or something that causes the surround nix code to stop providing them. Perhaps comments with example value(s) would be better?
| default = ":0"; | ||
| }; | ||
| xauthority = lib.mkOption { | ||
| type = lib.types.str; | ||
| default = "/root/.Xauthority"; |
There was a problem hiding this comment.
Harping on defaults again: do these really make sense here? If someone were to add a second "display target", wouldn't they have to specify display at least? They feel more like example values to me.
| displayTargets = lib.mkOption { | ||
| internal = true; | ||
| default = [ ]; | ||
| description = "Displays provided by this test machine."; |
There was a problem hiding this comment.
This test infrastructure cuts across a lot of layers. Is displayTargets the best name for this? I know "display" is x11 terminology that corresponds to a x11 server, but IIUC, this is written to be extensible to support wayland someday. Does "display" mean the same thing in that context? Or is it "compositor"? (not a wayland expert).
I acknowledge that improving the name might be impossible. But the description needs some elaboration to explain the nuance. At the very least, I'm confused when I see an option named "display targets" and documentation that uses the term "display". It leaves me wondering what the difference is between a display target and a plain old display.
| type = lib.types.str; | ||
| default = ":0"; | ||
| }; | ||
| xauthority = lib.mkOption { |
There was a problem hiding this comment.
Is this a layering violation? How does this option make sense to have if we hope to support wayland someday? Is the idea that it would become a nullable option that's set if and only if backend == "x11"?
Am I correctly guessing that we're implementing a sum type? Would types.attrTag be a better way to represent this?
| server: Path | ||
| relay: Path |
There was a problem hiding this comment.
I'm not sure what server and relay are. This might make sense when I read more of this PR, but the commit that introduced it ("nixos/test-driver: model display exporters and viewers") doesn't make sense by itself.
There was a problem hiding this comment.
I haven't made it far enough in this PR yet to read the code that actually sets/reads this relay value, but I've seen mentions of nsenter and socat in code I have not read yet.
Is it possible to avoid the tunneling and instead do something with a socket that we share through a bind mount with the container?
| type = types.listOf displayTarget; | ||
| default = [ ]; | ||
| }; | ||
| display_exporters = lib.mkOption { |
There was a problem hiding this comment.
IIUC, there are some rules about the keys here, yes? They must be "backends"? And for display_viewers below, they must be "protocols"?
Suggestion: I would consider more explicit names for these, such as: display_exporter_by_backend and display_viewer_by_protocol. Alternatively, some comments explaining this might be nice.
There was a problem hiding this comment.
Other questions I have (I'm just reading commit "nixos/test-driver: model display exporters and viewers" here, I suspect subsequent commits will answer this, but I also think commits should be able to stand alone)
- I'm confused about what it means for a single machine to have multiple
display_viewers. Is it in case it's running multiple x11 servers? Or could it be that we're running multiple display viewers for compatibility with multiple protocols? Ideally there would be a comment somewhere explaining this. - I'm unclear why we need to model things in terms of display exporter and display viewer. In web terms, it feels to me like talking about specific web servers and web browsers, rather than talking in terms of urls (which captures both a protocol, and details about how to connect). I'm probably misunderstanding stuff at a fundamental level, so my next question may not be worth spending any brain cycles on:
- With the existing structure, I don't see how I'd go about picking a display viewer to use to view stuff from a specific display exporter. I can't use the keys in the attrsets, as they represent different concepts (backend vs protocol).
There was a problem hiding this comment.
Commenting on this file because github doesn't let me comment on commits.
nixos/testing: configure interactive X11 export through VNC
AFAICT, this commit doesn't seem to really do anything. It threads some values down from nix to python, but they aren't used for anything. I suspect subsequent commits will clear this up, but it's making it difficult for me to read the PR commit by commit.
Yeah, I keep getting bogged down in this code. I spent several hours reading commits today, and didn't make it very far. I wonder if this PR would be more digestible if you had hardcoded assumptions at first (only supporting x11 vnc with one specific viewer), and started to break those assumptions down in subsequent PRs. Note: this is not me asking you to make this change, just thinking out loud. |
|
@jfly Thanks for the pain. Let me think; maybe I should drop some of abstractions and indirection here to simplify matters. I may have gone too far building out future infrastructure. Don't spend more time on this PR until I clean it up in some way (or confirm we indeed need all this complexity now). |
|
I appreciate it! Just to be clear: I think it's very possible the code is every bit as complex as it needs to be (or at least, that the abstractions you've created are appropriate for the full task supporting wayland, qemu, etc). I just can't tell until I've internalized more of it. |
This series implements X11 support for nspawn tests. It uses firefox suite as canary to demonstrate the approach works.
X11 sessions run in dummy Xserver. Input to be done with
xdotool. Screenshots withxwd.Interactive driver is updated to start x11vnc attached to :0 inside test containers that enable xserver option.
socatrelays connections from host netns into test container.remote-vieweris used as VNC viewer (triedtigervncand it had lots of graphical artifacts for some reason;remote-viewerhad no such problems).Wayland is out of scope - but the feature is modeled in such a way that should allow us to expand support for the following independently:
After this series is in, I plan to look at Wayland support.
Native Darwin support is out of scope too - for the simple reason that nspawn is not supported on the platform. Developers are expected to run their nspawn tests on remote Linux boxes. XQuartz works, e.g.:
I suggest to review commit by commit.
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.