Utilities for X11 image capture and acceptance testing #22

Merged
trurl merged 1 commits from trurl/wmaker:refactor/riir.headless-utils into refactor/riir 2026-03-26 18:47:37 -04:00
Owner

These are put in their own crate so that we can link tests against libWINGs freely.

Once libWINGs is entirely rewritten in Rust, we should be able to move these tests into the wings-rs crate.

These are put in their own crate so that we can link tests against libWINGs freely. Once libWINGs is entirely rewritten in Rust, we should be able to move these tests into the wings-rs crate.
trurl added 1 commit 2026-03-13 15:25:06 -04:00
These are put in their own crate so that we can link tests against libwings freely.

Once libwings is entirely rewritten in Rust, we should be able to move these tests back into the wings-rs crate.
trurl force-pushed refactor/riir.headless-utils from 5be0550532 to bbdcb672e6 2026-03-14 01:00:36 -04:00 Compare
trurl force-pushed refactor/riir.headless-utils from bbdcb672e6 to df3cfb5b40 2026-03-14 01:06:42 -04:00 Compare
trurl force-pushed refactor/riir.headless-utils from df3cfb5b40 to 7e000be765 2026-03-14 01:11:05 -04:00 Compare
trurl force-pushed refactor/riir.headless-utils from 7e000be765 to a2f7db3553 2026-03-14 01:16:44 -04:00 Compare
trurl requested review from cross 2026-03-14 01:17:55 -04:00
cross reviewed 2026-03-16 09:51:27 -04:00
@@ -0,0 +26,4 @@
impl Lock {
pub(crate) fn new(display: u16, file: File) -> Result<Self, LockError> {
match file.try_lock() {
Ok(_) => return Ok(Lock { display, file }),
Member

I think the return statements here are redundant: the match is the only expression in this function.

I think the `return` statements here are redundant: the match is the only expression in this function.
Author
Owner

Right you are. Thanks!

Right you are. Thanks!
trurl marked this conversation as resolved
trurl force-pushed refactor/riir.headless-utils from a2f7db3553 to 09c9243930 2026-03-16 14:28:04 -04:00 Compare
cross reviewed 2026-03-16 15:22:22 -04:00
@@ -0,0 +74,4 @@
/// `/tmp/.X*-lock`. If such a file exists, its `DISPLAY` value will not be
/// used.
///
/// Note also that this approach is far from perfect (and prone to TOCTOU
Member

So...A couple of things you can do to make this a bit more robust.

  1. Use your own subdirectory under /tmp for your lock files, to avoid conflicts against an actual X server.
  2. Open the file with O_EXCL; there's some wrapper in fs::create that does this, and lets you see the moral equivalent of EEXISTS. O_EXCL is atomic on Unix, and prevents
  3. You could also flock the file once it's open, but that's probably overkill.
So...A couple of things you can do to make this a bit more robust. 1. Use your own subdirectory under /tmp for your lock files, to avoid conflicts against an actual X server. 2. Open the file with `O_EXCL`; there's some wrapper in `fs::create` that does this, and lets you see the moral equivalent of EEXISTS. O_EXCL is atomic on Unix, and prevents 3. You could also `flock` the file once it's open, but that's probably overkill.
Member

Oh! You do the first. I got confused by searching for /tmp/.X{}-lock, but that's to try and avoid racing against another process starting an X server.

The big issue I'd worry about, I suppose, is either one of the local servers, or an ssh server process providing X forwarding. Starting displays at 32, as you do, will almost certainly never collide with anything.

Oh! You do the first. I got confused by searching for /tmp/.X{}-lock, but that's to try and avoid racing against another process starting an X server. The big issue I'd worry about, I suppose, is either one of the local servers, or an `ssh` server process providing X forwarding. Starting displays at 32, as you do, will almost certainly never collide with anything.
Author
Owner

As a point of reference, https://man.archlinux.org/man/xvfb-run.1.en starts at display number 99, so we could go higher. But I think 32 is fine.

RE checking/claiming a lockfile atomically: we both left longer comments further down, so I'll resolve this one.

As a point of reference, https://man.archlinux.org/man/xvfb-run.1.en starts at display number 99, so we could go higher. But I think 32 is fine. RE checking/claiming a lockfile atomically: we both left longer comments further down, so I'll resolve this one.
trurl marked this conversation as resolved
cross reviewed 2026-03-16 15:28:18 -04:00
@@ -0,0 +86,4 @@
return None;
}
let next = prev + 1;
if let Ok(exists) = fs::exists(Path::new(&format!("/tmp/.X{}-lock", next))) && exists {
Member

Given that you do Not want to collide with an actual X server process (or ssh daemon simulating one), why not use these names as the lock file directly? It's not just that you have your own namespace to protect colliding with an already-running server, but also that a server started subsequently to you running could collide with you (though since it involves binding a socket, that is unlikely). But an O_EXCL create here would pretty much guarantee isolation between actual X servers and you.

Given that you do Not want to collide with an actual X server process (or ssh daemon simulating one), why not use these names as the lock file directly? It's not just that you have your own namespace to protect colliding with an already-running server, but also that a server started subsequently to you running could collide with you (though since it involves binding a socket, that is unlikely). But an `O_EXCL` create here would pretty much guarantee isolation between actual X servers and you.
Author
Owner

Done.

Done.
trurl marked this conversation as resolved
cross reviewed 2026-03-16 15:37:33 -04:00
@@ -0,0 +42,4 @@
fn drop(&mut self) {
// `file` should be unlocked on drop, but we explicitly unlock it and
// unwrap the result so that errors aren't dropped silently.
self.file.unlock().expect("could not unlock file");
Member

So this gets into some of the weird semantics between POSIX locks and flock, but do you want to lock against threads in your own process, or other processes? Or both? The use of Atomics suggests that the former is at least on your radar.

I think what I would consider doing is having a "display allocator" that always starts counting from the lower bound, and attempts to allocate a "display" number by creating a temporary file, and keeps track of what numbers have been allocated (e.g., in a bit vector or something), all protected by a mutex. It doesn't have to be terribly general purpose: you won't have more than a handful running at a time; you could make the "allocator" a bitmap in a u128. The method would basically be,

  • lock the mutex
  • find the position of the first unset bit in the allocator variable,
  • If all bits are allocated, panic!().
  • Set the display candidate number to the bit position + 32.
  • Attempt to create a lock file as /tmp/.X{n}-lock (if that fails, continue)
  • Set the bit
  • Unlock the mutex
    The allocated display number is now bitno + 32.

And then the drop is,

  • Lock the mutex
  • Clear the bit (display number - 32) in the allocator variable
  • Close and unlink the lock file (/tmp/.X{dispno}-lock)
  • Unlock the mutex
So this gets into some of the weird semantics between POSIX locks and `flock`, but do you want to lock against threads in your own process, or other processes? Or both? The use of Atomics suggests that the former is at least on your radar. I think what I would consider doing is having a "display allocator" that always starts counting from the lower bound, and attempts to allocate a "display" number by creating a temporary file, and keeps track of what numbers have been allocated (e.g., in a bit vector or something), all protected by a mutex. It doesn't have to be terribly general purpose: you won't have more than a handful running at a time; you could make the "allocator" a bitmap in a u128. The method would basically be, - lock the mutex - find the position of the first unset bit in the allocator variable, - If all bits are allocated, panic!(). - Set the display candidate number to the bit position + 32. - Attempt to create a lock file as `/tmp/.X{n}-lock` (if that fails, continue) - Set the bit - Unlock the mutex The allocated display number is now bitno + 32. And then the drop is, - Lock the mutex - Clear the bit (display number - 32) in the allocator variable - Close and unlink the lock file (`/tmp/.X{dispno}-lock`) - Unlock the mutex -
Author
Owner

(Ok, so: the default Asahi Linux config powers your system completely off if you hit the power button, and Apple put the power button right next to backspace. I'll do my best to rewrite everything I just lost in a reply here, but please forgive me if it's a little terse.)

I think we're on the same page. I should definitely write better rustdoc comments.

I am trying to guard against other processes and threads in the same process. (https://nexte.st/, for example, runs each test in its own process.)

Attempt to create a lock file as /tmp/.X{n}-lock (if that fails, continue)

This looks good! It doesn't look like Xvfb cares if you have an empty /tmp/X{n}-lock, and OpenOptions::create_new should enable atomically checking and claiming a DISPLAY value. This simplifies things substantially (with no need for locking). Thank you!

Regarding the use of a bitfield: I don't think we need to reclaim DISPLAY values within the same process. Tracking the likely next unallocated DISPLAY value for the process should help to avoid having to scan a range of lockfiles that are in use each time a new DISPLAY value is needed. I created over 50 stale /tmp/.X{n}-lock files while developing this library, and failed test runs could also conceivably leave them around. Using an atomic reduces the cost of scanning a chunk of DISPLAY value space redundantly each time we want to provide a new value. So I'm in favor of simply incrementing an atomic.

(Ok, so: the default Asahi Linux config powers your system completely off if you hit the power button, and Apple put the power button right next to backspace. I'll do my best to rewrite everything I just lost in a reply here, but please forgive me if it's a little terse.) I think we're on the same page. I should definitely write better rustdoc comments. I am trying to guard against other processes and threads in the same process. (https://nexte.st/, for example, runs each test in its own process.) > Attempt to create a lock file as `/tmp/.X{n}-lock` (if that fails, continue) This looks good! It doesn't look like `Xvfb` cares if you have an empty `/tmp/X{n}-lock`, and `OpenOptions::create_new` should enable atomically checking and claiming a `DISPLAY` value. This simplifies things substantially (with no need for locking). Thank you! Regarding the use of a bitfield: I don't think we need to reclaim `DISPLAY` values within the same process. Tracking the likely next unallocated `DISPLAY` value for the process should help to avoid having to scan a range of lockfiles that are in use each time a new `DISPLAY` value is needed. I created over 50 stale `/tmp/.X{n}-lock` files while developing this library, and failed test runs could also conceivably leave them around. Using an atomic reduces the cost of scanning a chunk of `DISPLAY` value space redundantly each time we want to provide a new value. So I'm in favor of simply incrementing an atomic.
trurl marked this conversation as resolved
trurl force-pushed refactor/riir.headless-utils from 09c9243930 to bcbba911f7 2026-03-18 12:53:18 -04:00 Compare
trurl force-pushed refactor/riir.headless-utils from bcbba911f7 to d0fcb85ccd 2026-03-19 16:27:02 -04:00 Compare
Author
Owner

Thanks again for the review. I have given this more of a shakedown in downstream branches with tests and tweaked things a little (e.g., using prctl(2) to ensure that child processes are killed if the test process gets killed). I think this is ready.

Thanks again for the review. I have given this more of a shakedown in downstream branches with tests and tweaked things a little (e.g., using `prctl(2)` to ensure that child processes are killed if the test process gets killed). I think this is ready.
trurl force-pushed refactor/riir.headless-utils from d0fcb85ccd to 830924e251 2026-03-26 16:46:39 -04:00 Compare
trurl force-pushed refactor/riir.headless-utils from 830924e251 to 8fdd27758b 2026-03-26 16:56:44 -04:00 Compare
trurl force-pushed refactor/riir.headless-utils from 8fdd27758b to ec1761f4f3 2026-03-26 17:01:22 -04:00 Compare
Author
Owner

I still want to merge this so that other work can be unblocked, but I'll leave a note for posterity: when running with cargo-nextest, a test failed because it produced 100% the wrong image... which should have shown up for a different test! cargo-nextest runs tests in separate processes, so it looks like something may be going wrong with lockfile-based display number claims. I'll have to deal with this separately.

I still want to merge this so that other work can be unblocked, but I'll leave a note for posterity: when running with `cargo-nextest`, a test failed because it produced 100% the wrong image... which should have shown up for a different test! `cargo-nextest` runs tests in separate processes, so it looks like something may be going wrong with lockfile-based display number claims. I'll have to deal with this separately.
trurl merged commit ec1761f4f3 into refactor/riir 2026-03-26 18:47:37 -04:00
trurl deleted branch refactor/riir.headless-utils 2026-03-26 18:47:38 -04:00
Sign in to join this conversation.
No Reviewers
No Label
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: vitrine/wmaker#22