diff --git a/.github/workflows/locallibrary-scheduled.yml b/.github/workflows/locallibrary-scheduled.yml index 975a336e7..155ee8f61 100644 --- a/.github/workflows/locallibrary-scheduled.yml +++ b/.github/workflows/locallibrary-scheduled.yml @@ -2,10 +2,11 @@ name: locallibrary scheduled # The slow suites, weekly and on demand (Actions > locallibrary scheduled > Run workflow, on any # branch), so pull requests only wait for locallibrary-ci.yml's quick checks: every feature -# combination, Bombadil, mutation testing, fuzzing, the Hegel property tests on fresh inputs, the -# CLI's timings against a week earlier and a release build of every target. A week that changed -# none of what they test skips them (see the changes job). Also a weekly RustSec check, which -# catches advisories published against Cargo.lock between pushes, so it runs every week. +# combination, formatting and ejecting disk images, Bombadil, mutation testing, fuzzing, the Hegel +# property tests on fresh inputs, the CLI's timings against a week earlier and a release build of +# every target. A week that changed none of what they test skips them (see the changes job). Also +# a weekly RustSec check, which catches advisories published against Cargo.lock between pushes, so +# it runs every week. on: schedule: - cron: "17 5 * * 1" @@ -36,6 +37,8 @@ jobs: contents: read outputs: run: ${{ github.event_name != 'schedule' || steps.base.outputs.sha == '' || steps.filter.outputs.locallibrary == 'true' || steps.filter.outputs.readmes == 'true' }} + # The disk image tests, on format/ alone: they test nothing else. + format: ${{ github.event_name != 'schedule' || steps.base.outputs.sha == '' || steps.filter.outputs.format == 'true' }} steps: - if: github.event_name == 'schedule' uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -78,6 +81,9 @@ jobs: - '!**/*.md' readmes: - 'crates/*/README.md' + format: + - 'crates/locallibrary/src/format/**' + - '.github/workflows/locallibrary-scheduled.yml' # Every week whatever changed: advisories are published without a commit here. advisories: @@ -123,7 +129,8 @@ jobs: # Bombadil explores the TUI and the mix report (property-based UI testing; see # apps/locallibrary/bombadil/). - # macOS and Linux only: Bombadil has no Windows build. + # macOS and Linux only: Bombadil has no Windows build. Its last steps are the disk image tests + # on those two systems (see disk-images), here so they don't start runners of their own. bombadil: needs: changes if: needs.changes.outputs.run == 'true' @@ -173,6 +180,47 @@ jobs: with: name: bombadil-${{ matrix.os }} path: ${{ runner.temp }}/bombadil-* + # Even when Bombadil failed: the disk image tests don't use it. + - if: ${{ !cancelled() && needs.changes.outputs.format == 'true' }} + uses: taiki-e/install-action@8cca0918f8471c33515202b73f56ab27f2bed283 # v2.87.29 + with: + tool: cargo-nextest + - if: ${{ !cancelled() && needs.changes.outputs.format == 'true' && runner.os == 'Linux' }} + run: sudo apt-get update && sudo apt-get install -y dosfstools exfatprogs + - if: ${{ !cancelled() && needs.changes.outputs.format == 'true' }} + run: cargo nextest run -p locallibrary --lib --locked --no-fail-fast -E 'test(/^format::disk_image::/)' + env: + LOCALLIBRARY_DISK_IMAGE_TESTS: "1" + NEXTEST_PROFILE: ci + # losetup, sfdisk and mkfs need root on Linux, so the test binary runs under sudo there. + # Only the Linux target reads this. + CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_RUNNER: sudo --preserve-env + + # format::apply and eject on a real disk: each test creates a small disk image, attaches it + # (hdiutil, a loop device, a VHD), formats or ejects it, and detaches it + # (crates/locallibrary/src/format/tests/disk_image.rs). Without LOCALLIBRARY_DISK_IMAGE_TESTS they + # return at once, so no other run touches a device. macOS and Linux run them at the end of + # bombadil; this is Windows, which has no other job here. Windows runners are administrators + # already. + disk-images: + needs: changes + if: needs.changes.outputs.format == 'true' + runs-on: windows-latest + timeout-minutes: 45 + env: + LOCALLIBRARY_DISK_IMAGE_TESTS: "1" + # nextest's `ci` profile, as the features job has it. + NEXTEST_PROFILE: ci + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + - run: rustup toolchain install + - uses: Swatinem/rust-cache@6323deb102c322ba6fcbdcafc7e3dddab59af2b6 # v2.9.2 + - uses: taiki-e/install-action@8cca0918f8471c33515202b73f56ab27f2bed283 # v2.87.29 + with: + tool: cargo-nextest + - run: cargo nextest run -p locallibrary --lib --locked --no-fail-fast -E 'test(/^format::disk_image::/)' # Mutation testing with cargo-mutants (settings in .cargo/mutants.toml): it makes small changes # to the code (a function returning a default, a flipped comparison) and runs the package's @@ -519,10 +567,8 @@ jobs: # that passes closes it (scheduled-issue.yml). Skipped suites count as passed. report: needs: - [changes, advisories, features, bombadil, mutants, fuzz, hegel, cli-timings, release-plan, release-build, release-global] - # A week the suites skip follows one that passed, so no issue is open, and only the advisories - # can fail. - if: ${{ !cancelled() && github.event_name == 'schedule' && (needs.changes.outputs.run == 'true' || needs.advisories.result != 'success') }} + [changes, advisories, features, bombadil, disk-images, mutants, fuzz, hegel, cli-timings, release-plan, release-build, release-global] + if: ${{ !cancelled() && github.event_name == 'schedule' }} permissions: actions: read issues: write diff --git a/apps/locallibrary/CONTRIBUTING.md b/apps/locallibrary/CONTRIBUTING.md index 5b33b3271..d3ee213c5 100644 --- a/apps/locallibrary/CONTRIBUTING.md +++ b/apps/locallibrary/CONTRIBUTING.md @@ -98,6 +98,11 @@ branch from the Actions tab (locallibrary scheduled, Run workflow): - the builds without the default features (on Linux, without the ALSA headers): the tests of the library and the CLI with none and of the CLI with only `account`, and clippy over every combination of features with [cargo-hack](https://github.com/taiki-e/cargo-hack); +- `format` and `eject` on a disk image each test creates and attaches (hdiutil on macOS, a loop + device on Linux, a VHD on Windows; `crates/locallibrary/src/format/tests/disk_image.rs`). They skip + themselves without `LOCALLIBRARY_DISK_IMAGE_TESTS=1`. On a Mac, + `LOCALLIBRARY_DISK_IMAGE_TESTS=1 cargo nextest run -p locallibrary --lib -E 'test(/^format::disk_image::/)'` + runs them; - the Bombadil explorations below; - mutation testing of the past week's changes to the `loclib-*` crates, the plugins and `rekordbox-pdb` with [cargo-mutants](https://mutants.rs) (settings in `.cargo/mutants.toml`), in diff --git a/apps/locallibrary/TODO.md b/apps/locallibrary/TODO.md index 87e28554c..a2b7fdee7 100644 --- a/apps/locallibrary/TODO.md +++ b/apps/locallibrary/TODO.md @@ -165,14 +165,6 @@ Open work left by the 2026-10-11 and 2026-10-12 sessions was added on 2026-10-12 - [ ] The opener (`rundll32 url.dll,FileProtocolHandler`, used by sign-in, `mix --open` and the player's mix report) with a report path that has spaces. -## Tests to add - -- [ ] A CI-only `format::apply` test on a disk image per OS (hdiutil, a diskpart vdisk, losetup). - Linux first needs loop devices treated as removable under `LOCALLIBRARY_FORMAT_VIRTUAL` - (`format/linux.rs`). -- [ ] `eject` on a two-partition image on the macOS leg of that job: refusing when the store is on - another volume, and naming the volumes it unmounts. - ## Open bugs and gaps - [ ] Found while raising coverage, each a decision: a lenient replay that skips diff --git a/apps/locallibrary/changes/disk-image-tests.md b/apps/locallibrary/changes/disk-image-tests.md new file mode 100644 index 000000000..a7c24414c --- /dev/null +++ b/apps/locallibrary/changes/disk-image-tests.md @@ -0,0 +1,6 @@ +### Changed + +- `format` always refuses disk images, loop devices and virtual disks: + `LOCALLIBRARY_FORMAT_VIRTUAL`, which let it format any of them, is gone. The weekly scheduled + run tests `format` and `eject` on a disk image it attaches (hdiutil, a loop device, a VHD), and + only a test build lets that one image through. diff --git a/crates/locallibrary/src/format/linux.rs b/crates/locallibrary/src/format/linux.rs index 9b198b08f..e59f0f24a 100644 --- a/crates/locallibrary/src/format/linux.rs +++ b/crates/locallibrary/src/format/linux.rs @@ -69,11 +69,13 @@ fn mounts(node: &Value, out: &mut Vec<(String, String)>) { const SYSTEM_MOUNTS: [&str; 7] = ["/", "/boot", "/boot/efi", "/efi", "/usr", "/var", "[SWAP]"]; pub(super) fn parse_disk(d: &Value) -> Option { - if json::str(d, "type").as_deref() != Some("disk") { - return None; - } let name = json::str(d, "name").unwrap_or_default(); let path = json::str(d, "path").unwrap_or_else(|| format!("/dev/{name}")); + // A loop device only when it's the disk image a test attached. + let test_image = || json::str(d, "type").as_deref() == Some("loop") && super::is_test_image(&path); + if json::str(d, "type").as_deref() != Some("disk") && !test_image() { + return None; + } let mut all = Vec::new(); mounts(d, &mut all); let system = all.iter().any(|(_, m)| SYSTEM_MOUNTS.contains(&m.as_str())); diff --git a/crates/locallibrary/src/format/linux/tests.rs b/crates/locallibrary/src/format/linux/tests.rs index fe7a31085..3926a6a8b 100644 --- a/crates/locallibrary/src/format/linux/tests.rs +++ b/crates/locallibrary/src/format/linux/tests.rs @@ -28,6 +28,29 @@ fn parses_lsblk_old_and_new() { } } +/// A loop device isn't a disk, unless it's the disk image this test attached. +#[test] +fn only_the_tests_own_loop_device_is_a_disk() { + let lsblk = |path: &str| { + json!({ + "name": path.trim_start_matches("/dev/"), "path": path, "type": "loop", "size": 314572800u64, + "rm": false, "hotplug": false, "ro": false, "tran": null, "log-sec": 512, "pttype": "dos", + "children": [{ + "name": "loop7p1", "path": format!("{path}p1"), "type": "part", "size": 313524224u64, + "fstype": "vfat", "fsver": "FAT32", "label": "LTF", "mountpoint": null + }] + }) + }; + assert!(parse_disk(&lsblk("/dev/loop7")).is_none()); + super::super::TEST_IMAGE.set(Some("/dev/loop7".into())); + let d = parse_disk(&lsblk("/dev/loop7")).unwrap(); + assert!(d.virtual_disk && !d.removable && d.internal); + assert_eq!(d.problem(&[]), None); + assert_eq!(d.volumes[0].filesystem, Some(FsKind::Fat32)); + assert!(parse_disk(&lsblk("/dev/loop8")).is_none()); + super::super::TEST_IMAGE.set(None); +} + #[test] fn system_disks_are_flagged() { let mut v = usb_drive(false); diff --git a/crates/locallibrary/src/format/macos.rs b/crates/locallibrary/src/format/macos.rs index 970e8dd3b..b71e2ef31 100644 --- a/crates/locallibrary/src/format/macos.rs +++ b/crates/locallibrary/src/format/macos.rs @@ -22,10 +22,7 @@ fn diskutil(args: &[&str]) -> Result { } pub fn list() -> Result> { - let filter: &[&str] = if super::virtual_disks_allowed() { &["external"] } else { &["external", "physical"] }; - let mut args = vec!["list"]; - args.extend_from_slice(filter); - let listing = diskutil(&args)?; + let listing = diskutil(&["list", "external", "physical"])?; let boot = boot_disks(); let usb = usb_devices(); json::array(listing.get("WholeDisks")) diff --git a/crates/locallibrary/src/format/mod.rs b/crates/locallibrary/src/format/mod.rs index 1e1da2e17..336cf983f 100644 --- a/crates/locallibrary/src/format/mod.rs +++ b/crates/locallibrary/src/format/mod.rs @@ -240,10 +240,13 @@ impl Disk { if self.system { return Some("it holds the running operating system".into()); } - if self.internal || !self.removable { + // The disk image a test attached is let through, and nothing else: it's virtual, and on + // Linux (a loop device) not removable either. + let test_image = self.virtual_disk && is_test_image(&self.id); + if (self.internal || !self.removable) && !test_image { return Some("it's an internal disk (only external and removable drives can be formatted)".into()); } - if self.virtual_disk && !virtual_disks_allowed() { + if self.virtual_disk && !test_image { return Some("it's a disk image or virtual disk".into()); } if self.read_only { @@ -274,9 +277,22 @@ impl Disk { } } -/// Tests format disk images; set `LOCALLIBRARY_FORMAT_VIRTUAL=1` to allow them. -fn virtual_disks_allowed() -> bool { - std::env::var_os("LOCALLIBRARY_FORMAT_VIRTUAL").is_some_and(|v| v == "1") +#[cfg(test)] +thread_local! { + /// The whole-disk identifier of the disk image this test thread attached ([`disk_image`]), + /// the one virtual disk it may format. + pub(crate) static TEST_IMAGE: std::cell::RefCell> = const { std::cell::RefCell::new(None) }; +} + +/// Whether `id` names the disk image this test thread attached. Never, outside tests. +#[cfg(test)] +fn is_test_image(id: &str) -> bool { + TEST_IMAGE.with_borrow(|t| t.as_deref() == Some(id)) +} + +#[cfg(not(test))] +fn is_test_image(_: &str) -> bool { + false } /// A disk, how players would get on with it as it is, and whether it can be formatted. @@ -298,7 +314,7 @@ pub fn list(protected: &[PathBuf]) -> Result> { fn views(disks: Vec, protected: &[PathBuf]) -> Vec { disks .into_iter() - .filter(|d| d.removable && !d.internal && (!d.virtual_disk || virtual_disks_allowed())) + .filter(|d| d.removable && !d.internal && !d.virtual_disk) .map(|disk| DiskView { identity: disk.identity(), compatibility: players::assess_disk(&disk), @@ -823,3 +839,7 @@ pub(crate) mod json { #[cfg(test)] pub(crate) mod tests; + +#[cfg(test)] +#[path = "tests/disk_image.rs"] +mod disk_image; diff --git a/crates/locallibrary/src/format/tests.rs b/crates/locallibrary/src/format/tests.rs index fe95e6cc6..036505995 100644 --- a/crates/locallibrary/src/format/tests.rs +++ b/crates/locallibrary/src/format/tests.rs @@ -198,8 +198,7 @@ fn file_systems_are_named_and_small_volumes_get_small_clusters() { assert_eq!(Filesystem::Hfsplus.label(""), "REKORDBOX"); } -/// A disk image is refused unless tests allow them, and so is a disk holding `/` whatever it -/// says about itself. +/// A disk image is refused, and so is a disk holding `/` whatever it says about itself. #[test] fn refuses_disk_images_and_the_root_volume() { let image = Disk { virtual_disk: true, ..sample_disk() }; @@ -209,6 +208,30 @@ fn refuses_disk_images_and_the_root_volume() { assert_eq!(root.problem(&[]).as_deref(), Some("it holds the running operating system")); } +/// The disk image a test attached is let through, and only that one: a disk image under another +/// identifier is refused, and so is a disk that isn't an image under the image's identifier. +#[test] +fn only_the_tests_own_image_is_let_through() { + let image = Disk { virtual_disk: true, internal: true, removable: false, ..sample_disk() }; + TEST_IMAGE.set(Some(image.id.clone())); + assert_eq!(image.problem(&[]), None); + let other = Disk { id: "disk9".into(), ..image.clone() }; + assert_eq!( + other.problem(&[]).as_deref(), + Some("it's an internal disk (only external and removable drives can be formatted)") + ); + let real = Disk { virtual_disk: false, ..image.clone() }; + assert!(real.problem(&[]).unwrap().contains("internal")); + // Read-only, holding the system or a protected path: refused all the same. + assert_eq!(Disk { read_only: true, ..image.clone() }.problem(&[]).as_deref(), Some("it's write-protected")); + assert!(Disk { system: true, ..image.clone() }.problem(&[]).unwrap().contains("operating system")); + assert!(image.problem(&[PathBuf::from("/Volumes/DNA/store")]).unwrap().contains("holds")); + // Nor is it listed: tests name it by its identifier. + assert!(views(vec![image.clone()], &[]).is_empty()); + TEST_IMAGE.set(None); + assert!(image.problem(&[]).unwrap().contains("internal")); +} + /// Only external and removable physical disks are listed, each with why it can't be /// formatted when it can't. #[test] diff --git a/crates/locallibrary/src/format/tests/disk_image.rs b/crates/locallibrary/src/format/tests/disk_image.rs new file mode 100644 index 000000000..3df81d498 --- /dev/null +++ b/crates/locallibrary/src/format/tests/disk_image.rs @@ -0,0 +1,261 @@ +//! Disk images for the tests that format and eject a real disk. [`TestImage`] creates an empty +//! image, attaches it, and detaches it when it goes, also when the test fails. +//! +//! Only the scheduled workflow runs these tests, with `LOCALLIBRARY_DISK_IMAGE_TESTS=1` on each +//! system's runner: without it they return at once, so an ordinary `cargo test` never touches a +//! device. Linux and Windows need administrator rights (`losetup`, `Mount-DiskImage`). +//! +//! The format code refuses every disk image. [`TestImage::admit`] lets through the one a test +//! attached, by its exact identifier, on that test's thread only ([`super::TEST_IMAGE`]). + +use std::path::{Path, PathBuf}; +use std::sync::atomic::{AtomicU32, Ordering}; + +use super::{Cmd, Disk}; + +/// Whether this run may attach and format disk images. +fn enabled() -> bool { + std::env::var_os("LOCALLIBRARY_DISK_IMAGE_TESTS").is_some_and(|v| v == "1") +} + +/// True, and says so, when this run may not attach disk images: the test then returns. +pub(super) fn skipped() -> bool { + if enabled() { + return false; + } + eprintln!("skipped: set LOCALLIBRARY_DISK_IMAGE_TESTS=1 to attach and format disk images"); + true +} + +/// A volume label no other test running on this machine uses (the OS mounts volumes by label, +/// `/Volumes/