diff --git a/Dockerfile b/Dockerfile index 9291180..dfe0299 100644 --- a/Dockerfile +++ b/Dockerfile @@ -26,6 +26,9 @@ ENTRYPOINT ["music-mirror"] FROM runtime AS test RUN pip install --no-cache-dir pytest +# sync-to-ipod.sh and its tests need these; the runtime image deliberately does +# not carry them, and neither does the base. +RUN apk add --no-cache bash rsync findmnt COPY pytest.ini ./ # Host-side tools; not in the runtime image, but the suite covers them. COPY tools ./tools diff --git a/README.md b/README.md index 12df336..8f694cb 100644 --- a/README.md +++ b/README.md @@ -150,9 +150,15 @@ tools/sync-to-ipod.sh /mnt/tank/media/music-mp3 /media/IPOD/Music tools/sync-to-ipod.sh -n /mnt/tank/media/music-mp3 /media/IPOD/Music # dry run ``` -It refuses to start unless the destination is a mounted FAT filesystem that is -its own mount point, because `--delete` aimed at the wrong directory empties it -and does not announce itself. It also excludes `/.rockbox`, the scrobbler logs +The destination is where the artist folders should end up — normally a +subdirectory such as `/media/IPOD/Music`, not the card root. A subdirectory is +the better target: `--delete` is confined to it, and the device path budget is +derived from it rather than configured, so the two cannot disagree. + +It refuses to start unless the destination is on a mounted FAT filesystem. That +check is also what catches an unmounted device — `/media/IPOD/Music` then +resolves to the host's own root filesystem, and this refuses to empty that. +`--help` says all of it. It also excludes `/.rockbox`, the scrobbler logs and the various filesystem metadata directories from deletion — the mirror does not contain them, and without the exclusion a sync to the card root would remove the Rockbox install. diff --git a/tests/test_sync_to_ipod.py b/tests/test_sync_to_ipod.py new file mode 100644 index 0000000..2aef186 --- /dev/null +++ b/tests/test_sync_to_ipod.py @@ -0,0 +1,176 @@ +"""The guards on sync-to-ipod.sh, which are the substance of the script. + +rsync --delete is being aimed at a whole filesystem, so every refusal here is +protecting against emptying the wrong directory -- a mistake that does not +announce itself. +""" + +import shutil +import subprocess +from pathlib import Path + +import pytest + +SCRIPT = Path(__file__).resolve().parent.parent / "tools" / "sync-to-ipod.sh" + +# Skipped rather than failed where the tools are absent: this is a host-side +# script, and a machine without rsync is not a machine that would run it. +REQUIRED = ("bash", "rsync", "findmnt") +pytestmark = pytest.mark.skipif( + not all(shutil.which(tool) for tool in REQUIRED), + reason=f"needs {', '.join(REQUIRED)} on PATH", +) + + +def run(*arguments): + return subprocess.run( + ["bash", str(SCRIPT), *arguments], capture_output=True, text=True + ) + + +@pytest.fixture +def mirror(tmp_path): + source = tmp_path / "mirror" + (source / "Album").mkdir(parents=True) + (source / "Album" / "track.mp3").write_bytes(b"x") + return source + + +def test_the_host_root_is_refused(mirror): + """Stripping the trailing slash from "/" leaves an empty string, and an + earlier version then reported it as "not a directory" instead.""" + result = run(str(mirror), "/") + + assert result.returncode == 1 + assert "refusing to sync onto /" in result.stderr + + +def test_an_empty_mirror_is_refused(tmp_path): + """Mirroring nothing onto the device would delete everything on it.""" + empty = tmp_path / "empty" + empty.mkdir() + destination = tmp_path / "dest" + destination.mkdir() + + result = run(str(empty), str(destination)) + + assert result.returncode == 1 + assert "refusing to mirror nothing" in result.stderr + + +def test_syncing_a_directory_onto_itself_is_refused(mirror): + result = run(str(mirror), str(mirror)) + + assert result.returncode == 1 + assert "same directory" in result.stderr + + +def test_a_non_fat_destination_is_refused(mirror, tmp_path): + """Which is also how an unmounted device is caught: /media/IPOD/Music then + resolves to the host's own root filesystem.""" + destination = tmp_path / "dest" + destination.mkdir() + + result = run(str(mirror), str(destination)) + + assert result.returncode == 1 + assert "not FAT" in result.stderr + assert "Is the device mounted?" in result.stderr + + +def test_a_missing_destination_is_refused(mirror, tmp_path): + result = run(str(mirror), str(tmp_path / "nowhere")) + + assert result.returncode == 1 + assert "not a directory" in result.stderr + + +def test_a_subdirectory_of_the_device_is_a_valid_target(mirror, tmp_path): + """The better target, in fact: --delete is confined to it.""" + destination = tmp_path / "dest" / "Music" + destination.mkdir(parents=True) + + result = run("-f", "-n", str(mirror), str(destination)) + + assert result.returncode == 0, result.stderr + assert "dry run, nothing was written" in result.stderr + + +def test_the_device_prefix_is_derived_from_the_destination(mirror, tmp_path): + """Derived rather than configured, so it cannot disagree with where the + files are actually going -- and the device's path limit applies to it.""" + destination = tmp_path / "dest" / "Music" + destination.mkdir(parents=True) + + result = run("-f", "-n", str(mirror), str(destination)) + + assert "the device will see this as /" in result.stderr + + +def test_a_dry_run_writes_nothing(mirror, tmp_path): + destination = tmp_path / "dest" + destination.mkdir() + + run("-f", "-n", str(mirror), str(destination)) + + assert list(destination.iterdir()) == [] + + +def test_rockbox_is_never_deleted(mirror, tmp_path): + """A sync to the card root would otherwise remove the Rockbox install, + since the mirror does not contain it.""" + destination = tmp_path / "dest" + destination.mkdir() + (destination / ".rockbox").mkdir() + (destination / ".rockbox" / "rockbox.ipod").write_bytes(b"firmware") + (destination / ".scrobbler.log").write_bytes(b"#AUDIOSCROBBLER/1.1\n") + (destination / "Stale.mp3").write_bytes(b"old") + + result = run("-f", "-S", "-U", str(mirror), str(destination)) + + assert result.returncode == 0, result.stderr + assert (destination / ".rockbox" / "rockbox.ipod").is_file() + assert (destination / ".scrobbler.log").is_file() + # But a track whose source has gone is still removed. That is the point. + assert not (destination / "Stale.mp3").exists() + assert (destination / "Album" / "track.mp3").is_file() + + +def test_help_goes_to_stdout_and_exits_clean(): + """Asking for help is not an error; getting the arguments wrong is.""" + result = run("--help") + + assert result.returncode == 0 + assert result.stdout.startswith("usage:") + assert result.stderr == "" + + +def test_short_help_behaves_the_same(): + result = run("-h") + + assert result.returncode == 0 + assert result.stdout.startswith("usage:") + + +def test_misuse_goes_to_stderr_and_does_not(): + result = run("only-one-argument") + + assert result.returncode == 2 + assert result.stderr.startswith("usage:") + assert result.stdout == "" + + +def test_the_help_explains_what_the_destination_should_be(): + """The question this script actually gets asked.""" + help_text = run("--help").stdout + + assert "/media/IPOD/Music" in help_text + assert "artist folders" in help_text + assert ".rockbox" in help_text + + +def test_the_help_says_how_to_reach_and_leave_disk_mode(): + help_text = run("--help").stdout + + assert "Menu+Select" in help_text + assert "holding Play" in help_text diff --git a/tools/sync-to-ipod.sh b/tools/sync-to-ipod.sh index 2253c31..ca81d4c 100755 --- a/tools/sync-to-ipod.sh +++ b/tools/sync-to-ipod.sh @@ -12,7 +12,13 @@ set -euo pipefail usage() { - cat >&2 <<'USAGE' + # Help goes to stdout and exits clean; misuse goes to stderr and does not. + local stream=2 code=2 + if [ "${1:-}" = "help" ]; then + stream=1 + code=0 + fi + cat >&"$stream" <<'USAGE' usage: sync-to-ipod.sh [options] -n dry run; show what would change and touch nothing @@ -24,34 +30,60 @@ Submitting scrobbles needs LASTFM_API_KEY and LASTFM_API_SECRET; it is skipped with a note when they are unset. Scrobbling is a write method and needs the secret, unlike the read-only calls elsewhere in these projects. -The destination must be a mounted FAT filesystem. Reach it with the Apple -firmware's disk mode: Menu+Select to reboot, then immediately Select+Play. +The mirror is the directory holding the artist folders. The destination is +where those folders should end up on the device -- not the card root, unless +that is genuinely where you want them: + + sync-to-ipod.sh /mnt/tank/media/music-mp3 /media/IPOD/Music + +A subdirectory is the better target: --delete is confined to it, and the path +budget is derived from it, since the device's 260-character limit counts the +whole path as the device sees it. /.rockbox and the scrobbler logs are never +deleted wherever you point this. + +The destination must be on a mounted FAT filesystem. That check is also what +catches an unmounted device: /media/IPOD/Music then resolves to the host's own +root filesystem, and this refuses to empty that. + +Reach the device with the Apple firmware's disk mode: Menu+Select to reboot, +then immediately Select+Play. Power off afterwards by holding Play. USAGE - exit 2 + exit "$code" } dry_run=false force=false unmount=true scrobble=true +for argument in "$@"; do + [ "$argument" = "--help" ] && usage help +done while getopts ":nfSUh" option; do case "$option" in n) dry_run=true ;; f) force=true ;; S) scrobble=false ;; U) unmount=false ;; + h) usage help ;; *) usage ;; esac done shift $((OPTIND - 1)) [ $# -eq 2 ] || usage +# Trailing slashes are stripped for tidiness, but stripping one from "/" leaves +# an empty string, and the guard below would then never see the root it is +# there to refuse. mirror=${1%/} +mirror=${mirror:-/} destination=${2%/} +destination=${destination:-/} here=$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd) die() { - printf 'sync-to-ipod: %s\n' "$1" >&2 + # Every argument, not just the first: the second half of a message is + # usually the half that says what to do about it. + printf 'sync-to-ipod: %s\n' "$*" >&2 exit 1 } @@ -59,28 +91,41 @@ die() { [ -n "$(ls -A "$mirror")" ] || die "mirror $mirror is empty; refusing to mirror nothing" [ -d "$destination" ] || die "destination $destination is not a directory" -# --delete makes every one of these load-bearing. A destination that is not its -# own mount point means the path is wrong, and emptying the wrong directory is -# not a mistake that announces itself. +# --delete makes every one of these load-bearing. Emptying the wrong directory +# is not a mistake that announces itself. case "$destination" in "" | "/" | "$HOME") die "refusing to sync onto $destination" ;; esac [ "$(readlink -f "$mirror")" != "$(readlink -f "$destination")" ] || die "mirror and destination are the same directory" -mountpoint -q -- "$destination" || die "$destination is not a mount point" +# The filesystem the destination sits on, which is the check that matters: a +# subdirectory of the card is a perfectly good target, and is the better one, +# because --delete is then confined to it. Being FAT is also what proves the +# card is mounted at all -- an unmounted /media/IPOD/Music resolves to the +# host's own root filesystem, and this refuses to empty that. filesystem=$(findmnt -no FSTYPE --target "$destination") +mounted_on=$(findmnt -no TARGET --target "$destination") case "$filesystem" in vfat | exfat) ;; *) - $force || die "$destination is $filesystem, not FAT; pass -f if that is deliberate" + $force || + die "$destination is on a $filesystem filesystem, not FAT." \ + "Is the device mounted? Pass -f if this is deliberate." printf 'sync-to-ipod: destination is %s, not FAT\n' "$filesystem" >&2 ;; esac +# What the device will call this directory, which is what its path limit +# applies to. Derived rather than configured, so it cannot disagree with where +# the files are actually going. +device_prefix=${destination#"$mounted_on"} +device_prefix="/${device_prefix#/}" +printf 'sync-to-ipod: the device will see this as %s\n' "$device_prefix" >&2 + if $force; then printf 'sync-to-ipod: skipping the FAT32 check\n' >&2 -elif ! python3 "$here/check_fat32.py" "$mirror"; then +elif ! python3 "$here/check_fat32.py" --device-prefix "$device_prefix" "$mirror"; then die "the mirror holds paths FAT32 will not take; run music-mirror with --fat32-safe" fi