Merge remote-tracking branch 'upstream/main' into m2915-merge

apps/effectcraft-cli/src/main.rs

@@ -48,6 +48,7 @@
 
 use std::io::Write;
 
+mod project_lock;
 mod template;
 
 use effectcraft_automation::tools::{self, Reply};
@@ -515,6 +516,7 @@ fn script_cmd(args: &Args, json_out: bool) -> Result<(), Failure> {
         (None, Some(f)) => (std::fs::read_to_string(f).map_err(|e| Failure::Error(format!("cannot read {f}: {e}")))?, f.clone()),
         (None, None) => return usage_err("script needs a .jsx/.js file or --eval CODE"),
     };
+    let _lock = project_lock::for_save(args)?;
     let mut b = backend(args, false)?;
     let r = b.exec("script.run", json!({"code": code, "name": name}))?;
     let saved = maybe_save(&mut b, args)?;
@@ -618,6 +620,7 @@ fn run(cmd: &str, args: &Args, json_out: bool) -> Result<(), Failure> {
                 (Some(p), _) | (None, Some(p)) => serde_json::from_str(p).map_err(|e| Failure::Usage(format!("params are not valid JSON: {e}")))?,
                 (None, None) => json!({}),
             };
+            let _lock = project_lock::for_save(args)?;
             let mut b = backend(args, true)?;
             let r = b.exec(&id, params)?;
             if let Some(e) = failed_render(&id, &r) {
@@ -661,6 +664,7 @@ fn run(cmd: &str, args: &Args, json_out: bool) -> Result<(), Failure> {
             if steps.is_empty() {
                 return Err(Failure::Usage(format!("--commands {}: no commands", sources.join(", "))));
             }
+            let _lock = project_lock::for_save(args)?;
             let mut b = backend(args, true)?;
             let mut results = vec![];
             // The step's index closes the message: a generated list repeats ids, and callers match
@@ -697,7 +701,7 @@ fn run(cmd: &str, args: &Args, json_out: bool) -> Result<(), Failure> {
                     if let Some(e) = p.get("expression").and_then(Value::as_str) {
                         extra += &format!("  expr: {e}");
                     }
-                    say!("  {:<44} {:<8} {}{extra}", p["path"].as_str().unwrap_or(""), p["type"].as_str().unwrap_or(""), p["value"]);
+                    say!("  {:<44} {:<8} {}{extra}", p["path"].as_str().unwrap_or(""), p["type"].as_str().unwrap_or(""), value_text(p));
                 }
             }
         }
@@ -715,6 +719,7 @@ fn run(cmd: &str, args: &Args, json_out: bool) -> Result<(), Failure> {
             }
             let (comp, layer, path) = (args.pos[0].clone(), args.pos[1].clone(), args.pos[2].clone());
             let value = args.pos.get(3).map(|v| value_arg(v));
+            let _lock = project_lock::for_save(args)?;
             let mut b = backend(args, true)?;
             let a =
                 json!({"comp": reference(&comp), "layer": reference(&layer), "path": path, "value": value, "time": args.num("--time")?, "expression": expr});
@@ -1548,6 +1553,16 @@ fn bench_gpu(s: &Session, args: &Args) -> Result<(), Failure> {
     Ok(())
 }
 
+/// A flat property's value for the text listing: an enum's number with the label `options` gives it
+/// (#2928), since the numbers are 0-based where After Effects scripting counts from 1.
+fn value_text(p: &Value) -> String {
+    let label = p["options"].as_array().and_then(|o| o.iter().find(|o| o["value"] == p["value"])).and_then(|o| o["label"].as_str());
+    match label {
+        Some(l) => format!("{} ({l})", p["value"]),
+        None => p["value"].to_string(),
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

apps/effectcraft-cli/src/project_lock.rs

@@ -0,0 +1,150 @@
+//! One saving run at a time per project file.
+//!
+//! A `--save` run reads the project, edits it and writes the whole file back, so two at once would
+//! silently drop the edits of whichever saved first (#2851). Each run holds an OS lock from before
+//! its read until it exits; the next one waits, then reads what the first one saved. This is how
+//! filmcraft-cli does it (storytold/filmcraft#3926).
+//!
+//! The lock is on a file next to the project, not on the project: a save replaces the project's
+//! inode, and on Windows a locked file can't be read by other processes. The OS drops the lock when
+//! the process ends, however it ends, so a crash leaves nothing stale. The file itself stays, since
+//! removing it would let one run wait on the old file while another locks a new one.
+
+use crate::{Args, Failure};
+use std::fs::{File, TryLockError};
+use std::io::Write;
+use std::path::{Path, PathBuf};
+
+/// Most bytes of the project's file name kept in the lock's name; the rest of the 255-byte limit
+/// is left for the leading dot and `.lock`.
+const MAX_NAME_BYTES: usize = 200;
+
+fn lock_path(project: &Path) -> PathBuf {
+    let name = project.file_name().map(|n| n.to_string_lossy().into_owned()).unwrap_or_default();
+    project.with_file_name(format!(".{}.lock", &name[..name.floor_char_boundary(MAX_NAME_BYTES)]))
+}
+
+/// Do two paths name one existing file?
+fn same_file(a: &str, b: &str) -> bool {
+    a == b || matches!((std::fs::canonicalize(a), std::fs::canonicalize(b)), (Ok(a), Ok(b)) if a == b)
+}
+
+/// The path this run saves over, when that is the file it opens. `--save-as` somewhere else replaces
+/// that file whatever it held, so there is no edit of its to lose.
+///
+/// The lock goes by this path as written, not by where a symlink points: runs that name the
+/// project the same way share one lock.
+///
+/// A missing project is an error here, before anything else, the same one opening it would give.
+/// If the run went on, another one could create the project before it is read, and this run would
+/// edit and save it with no lock. Checking first also means the paths compared below exist, so
+/// `canonicalize` tells exactly whether they are one file, however they are spelt. A project that
+/// exists but isn't a file gets no lock file beside it, and opening it reports why.
+fn saved_in_place(a: &Args) -> Result<Option<&Path>, String> {
+    // `--bridge` with a project is a usage error, which `backend` reports
+    let Some(project) = a.project.as_deref().filter(|_| a.opt("--bridge").is_none()) else { return Ok(None) };
+    match Path::new(project).metadata() {
+        Ok(meta) if meta.is_file() => {}
+        Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Err(format!("cannot read {project}: {e}")),
+        _ => return Ok(None),
+    }
+    match a.opt("--save-as") {
+        Some(target) if same_file(target, project) => Ok(Some(Path::new(target))),
+        None if a.flag("--save") => Ok(Some(Path::new(project))),
+        _ => Ok(None),
+    }
+}
+
+/// Lock `project`, calling `waiting` first if another run holds the lock.
+fn lock(project: &Path, waiting: impl FnOnce()) -> std::io::Result<File> {
+    let file = File::options().create(true).truncate(false).read(true).write(true).open(lock_path(project))?;
+    match file.try_lock() {
+        Ok(()) => {}
+        Err(TryLockError::WouldBlock) => {
+            waiting();
+            file.lock()?;
+        }
+        Err(TryLockError::Error(e)) => return Err(e),
+    }
+    Ok(file)
+}
+
+/// Take the project's lock if this run saves over the file it opens, or fail if the project is
+/// missing. Call it before opening the project and keep the result until the save is done. Blocks
+/// while another run holds the lock, which is fine here: nothing else is running yet. A lock that
+/// can't be had (a read-only folder, a file system without locks) is a warning and not an error, so
+/// that saving works where it did.
+pub(crate) fn for_save(a: &Args) -> Result<Option<File>, Failure> {
+    let Some(project) = saved_in_place(a).map_err(Failure::Error)? else { return Ok(None) };
+    let shown = project.display();
+    let waiting = || {
+        let _ = writeln!(std::io::stderr(), "effectcraft-cli: waiting for another run saving {shown}");
+    };
+    match lock(project, waiting) {
+        Ok(file) => Ok(Some(file)),
+        Err(e) => {
+            let _ = writeln!(std::io::stderr(), "effectcraft-cli: warning: can't lock {shown} ({e}); another run saving it now could overwrite these edits");
+            Ok(None)
+        }
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    fn args(list: &[&str]) -> Args {
+        Args::parse(list.iter().map(|s| s.to_string()).collect()).unwrap()
+    }
+
+    #[test]
+    fn the_lock_sits_next_to_the_project() {
+        assert_eq!(lock_path(Path::new("/a/b/p.ecproj")), Path::new("/a/b/.p.ecproj.lock"));
+        assert_eq!(lock_path(Path::new("p.ecproj")), Path::new(".p.ecproj.lock"));
+    }
+
+    #[test]
+    fn a_long_project_name_still_gets_a_lock_name_the_file_system_takes() {
+        for name in ["a".repeat(255), "é".repeat(127)] {
+            let lock = lock_path(Path::new(&name));
+            let lock = lock.to_str().unwrap();
+            assert!(lock.len() <= 255, "{} bytes", lock.len());
+            assert!(lock.ends_with(".lock"));
+        }
+    }
+
+    /// The moment between this check and opening the project can't be hit on demand, so this pins
+    /// the decision that closes it: a run never goes on to open a project that wasn't there, however
+    /// its paths are spelt and whatever it saves.
+    #[test]
+    fn a_missing_project_stops_the_run_before_anything_opens_it() {
+        let name = format!("ec-cli-gone-{}.ecproj", std::process::id());
+        let gone = std::env::temp_dir().join(&name);
+        let gone = gone.to_str().unwrap();
+        let alias = std::env::temp_dir().join(".").join(&name);
+        let alias = alias.to_str().unwrap();
+        let dotted = format!("./{name}");
+        // in a folder that doesn't exist either, so no part of either spelling can be resolved
+        let new_dir = std::env::temp_dir().join(format!("ec-cli-new-dir-{}", std::process::id()));
+        let in_new_dir = new_dir.join("p.ecproj");
+        let in_new_dir = in_new_dir.to_str().unwrap();
+        let in_new_dir_alias = new_dir.join(".").join("p.ecproj");
+        let in_new_dir_alias = in_new_dir_alias.to_str().unwrap();
+        for (project, list) in [
+            (gone, &["exec", "--project", gone, "--save"][..]),
+            (gone, &["exec", gone, "--save"]),
+            (gone, &["exec", "--project", gone, "--save-as", gone]),
+            (gone, &["exec", "--project", gone, "--save-as", alias]),
+            (name.as_str(), &["exec", "--project", &name, "--save-as", &dotted]),
+            (in_new_dir, &["exec", "--project", in_new_dir, "--save-as", in_new_dir_alias]),
+            (gone, &["exec", "--project", gone, "--save-as", "other.ecproj"]),
+            (gone, &["exec", "--project", gone]),
+        ] {
+            let error = saved_in_place(&args(list)).unwrap_err();
+            assert_eq!(error.split_once(": ").map(|(head, _)| head), Some(format!("cannot read {project}").as_str()), "{list:?}: {error}");
+        }
+        // a project with --bridge is a usage error, which `backend` reports; no project, no lock
+        assert_eq!(saved_in_place(&args(&["exec", "--bridge", "9877", "--project", gone, "--save"])), Ok(None));
+        assert_eq!(saved_in_place(&args(&["exec", "--demo", "--save-as", gone])), Ok(None));
+    }
+}

apps/effectcraft-cli/tests/it/cli.rs

@@ -110,6 +110,197 @@ fn exec_set_get_with_saved_project() {
     assert_eq!(v[1]["result"]["time"], v[0]["result"]["time"], "{v}");
 }
 
+/// #2928: the text listing gives an enum's label next to its number (0-based, unlike After
+/// Effects scripting), the way `props --json` lists its `options`.
+#[test]
+fn props_lists_enum_labels() {
+    let proj = tmp("enum-labels.ecproj");
+    let p = proj.to_str().unwrap();
+    let anim = r#"{"layer":"t","properties":["opacity"]}"#;
+    ok_json(&[
+        "run",
+        "comp.new",
+        r#"{"name":"T","width":64,"height":36}"#,
+        "layer.newText",
+        r#"{"text":"AB","name":"t"}"#,
+        "layer.addTextAnimator",
+        anim,
+        "--empty",
+        "--save-as",
+        p,
+    ]);
+    ok_json(&["set", "T", "t", "text/animators/animator/selectors/rangeSelector/advanced/shape", "1", p, "--save"]);
+    let out = bin().args(["props", "T", "t", p]).output().unwrap();
+    let text = String::from_utf8_lossy(&out.stdout);
+    let shape = text.lines().find(|l| l.contains("advanced/shape")).unwrap_or_else(|| panic!("no shape line in:\n{text}"));
+    assert!(shape.trim_end().ends_with("enum     1 (Ramp Up)"), "{shape}");
+    let units = text.lines().find(|l| l.contains("advanced/units")).unwrap_or_default();
+    assert!(units.trim_end().ends_with("0 (Percentage)"), "{units}");
+    let size = text.lines().find(|l| l.contains("transform/opacity")).unwrap_or_default();
+    assert!(size.trim_end().ends_with("100.0"), "a number without options stays bare: {size}");
+}
+
+/// A project in a new folder of its own, for the save lock tests (#2851).
+fn lock_test_project(tag: &str) -> (PathBuf, String) {
+    let dir = tmp(&format!("lock-{tag}"));
+    let _ = std::fs::remove_dir_all(&dir);
+    std::fs::create_dir_all(&dir).unwrap();
+    let proj = dir.join("p.ecproj").to_str().unwrap().to_string();
+    ok_json(&["exec", "project.newFolder", r#"{"name":"start"}"#, "--empty", "--save-as", &proj]);
+    (dir, proj)
+}
+
+/// The lock of the project file `name`, held by this process.
+fn hold_lock(dir: &std::path::Path, name: &str) -> std::fs::File {
+    let lock = std::fs::File::options().create(true).truncate(false).read(true).write(true).open(dir.join(format!(".{name}.lock"))).unwrap();
+    lock.lock().unwrap();
+    lock
+}
+
+/// Start a run that has to wait for a lock this process holds; returns once it says it is waiting.
+fn start_waiting_run(args: &[&str]) -> std::process::Child {
+    let mut child = bin().args(args).stdout(Stdio::null()).stderr(Stdio::piped()).spawn().unwrap();
+    let mut stderr = BufReader::new(child.stderr.take().unwrap());
+    let mut line = String::new();
+    while !line.contains("waiting for another run saving") {
+        line.clear();
+        assert_ne!(stderr.read_line(&mut line).unwrap(), 0, "{args:?} ended without waiting");
+    }
+    assert!(child.try_wait().unwrap().is_none(), "{args:?} did not wait");
+    child
+}
+
+/// Folder names in a saved project.
+fn has_folder(proj: &str, name: &str) -> bool {
+    std::fs::read_to_string(proj).unwrap().contains(&format!("\"{name}\""))
+}
+
+#[test]
+fn parallel_saves_keep_every_edit() {
+    let (dir, proj) = lock_test_project("parallel");
+    let names = |writer: usize| (0..300).map(move |i| format!("edit-{writer}-{i}"));
+    let mut writers: Vec<_> = (0..4)
+        .map(|_| {
+            bin()
+                .args(["run", "--project", &proj, "--commands", "-", "--save"])
+                .stdin(Stdio::piped())
+                .stdout(Stdio::null())
+                .stderr(Stdio::null())
+                .spawn()
+                .unwrap()
+        })
+        .collect();
+    // Each writer reads its whole script before it opens the project, so closing the stdins together
+    // starts all the edits at once, and long scripts keep each read-to-save span over the others.
+    for (n, writer) in writers.iter_mut().enumerate() {
+        let script: String = names(n).map(|name| format!("{}\n", json!({"command": "project.newFolder", "params": {"name": name}}))).collect();
+        writer.stdin.take().unwrap().write_all(script.as_bytes()).unwrap();
+    }
+    for mut writer in writers {
+        assert!(writer.wait().unwrap().success());
+    }
+    let lost: Vec<String> = (0..4).flat_map(names).filter(|name| !has_folder(&proj, name)).collect();
+    assert!(lost.is_empty(), "lost {} edits: {lost:?}", lost.len());
+    let _ = std::fs::remove_dir_all(&dir);
+}
+
+#[test]
+fn a_save_in_place_waits_for_the_lock_then_edits_the_latest_project() {
+    let (dir, proj) = lock_test_project("wait");
+    for save in [&["--save"][..], &["--save-as", proj.as_str()]] {
+        let held = hold_lock(&dir, "p.ecproj");
+        let before = std::fs::read(&proj).unwrap();
+        let params = json!({"name": format!("waited-{}", save.len())}).to_string();
+        let args = [&["exec", "project.newFolder", &params, "--project", &proj][..], save].concat();
+        let mut child = start_waiting_run(&args);
+        assert_eq!(std::fs::read(&proj).unwrap(), before, "{save:?} wrote while another run held the lock");
+        // an edit made meanwhile by the holder must survive: the waiter reads only after the lock is free
+        let held_params = json!({"name": format!("held-{}", save.len())}).to_string();
+        ok_json(&["exec", "project.newFolder", &held_params, "--project", &proj, "--save-as", &format!("{proj}.tmp")]);
+        std::fs::rename(format!("{proj}.tmp"), &proj).unwrap();
+        drop(held);
+        assert!(child.wait().unwrap().success(), "{save:?}");
+        for want in [format!("waited-{}", save.len()), format!("held-{}", save.len())] {
+            assert!(has_folder(&proj, &want), "{save:?} lost {want}");
+        }
+    }
+    let _ = std::fs::remove_dir_all(&dir);
+}
+
+/// Runs through a symlink take the same lock before and after a save, or a run still waiting on
+/// the old one could overwrite a later run's save.
+#[cfg(unix)]
+#[test]
+fn a_symlinked_project_keeps_its_lock_after_a_save() {
+    let (dir, proj) = lock_test_project("symlink");
+    let link = dir.join("link.ecproj");
+    std::os::unix::fs::symlink(&proj, &link).unwrap();
+    for name in [r#"{"name":"first"}"#, r#"{"name":"second"}"#] {
+        let held = hold_lock(&dir, "link.ecproj");
+        let mut child = start_waiting_run(&["exec", "project.newFolder", name, "--project", link.to_str().unwrap(), "--save"]);
+        drop(held);
+        assert!(child.wait().unwrap().success(), "{name}");
+    }
+    let _ = std::fs::remove_dir_all(&dir);
+}
+
+#[test]
+fn runs_that_do_not_save_in_place_ignore_the_lock() {
+    let (dir, proj) = lock_test_project("ignore");
+    let other = dir.join("other.ecproj").to_str().unwrap().to_string();
+    let _held = hold_lock(&dir, "p.ecproj");
+    let params = r#"{"name":"copy-only"}"#;
+    let cases: [&[&str]; 3] = [
+        &["info", "--project", &proj],
+        &["exec", "project.newFolder", params, "--project", &proj],
+        &["exec", "project.newFolder", params, "--project", &proj, "--save-as", &other],
+    ];
+    for args in cases {
+        let mut child = bin().args(args).stdout(Stdio::null()).stderr(Stdio::null()).spawn().unwrap();
+        let deadline = std::time::Instant::now() + std::time::Duration::from_secs(60);
+        let status = loop {
+            if let Some(status) = child.try_wait().unwrap() {
+                break status;
+            }
+            if std::time::Instant::now() > deadline {
+                let _ = child.kill();
+                panic!("{args:?} is waiting for a lock it doesn't need");
+            }
+            std::thread::sleep(std::time::Duration::from_millis(10));
+        };
+        assert!(status.success(), "{args:?}");
+    }
+    assert!(!has_folder(&proj, "copy-only"));
+    assert!(has_folder(&other, "copy-only"));
+    let _ = std::fs::remove_dir_all(&dir);
+}
+
+#[test]
+fn a_missing_project_gets_no_lock_file_and_keeps_its_error() {
+    let (dir, _) = lock_test_project("missing");
+    let gone = dir.join("gone.ecproj").to_str().unwrap().to_string();
+    let other = dir.join("other.ecproj").to_str().unwrap().to_string();
+    // what opening it says, after the `effectcraft-cli <subcommand>: ` prefix
+    let error = |args: &[&str]| {
+        let out = bin().args(args).output().unwrap();
+        assert_eq!(out.status.code(), Some(1), "{args:?}");
+        let stderr = String::from_utf8_lossy(&out.stderr).into_owned();
+        stderr.split_once(": ").map(|(_, e)| e.to_string()).unwrap_or(stderr)
+    };
+    let plain = error(&["info", "--project", &gone]);
+    assert!(plain.contains("gone.ecproj"), "{plain}");
+    // saving in place, saving elsewhere, or not saving: the error is the one opening gives
+    for save in [&["--save"][..], &["--save-as", gone.as_str()], &["--save-as", other.as_str()], &[]] {
+        let args = [&["exec", "project.newFolder", r#"{"name":"copy-only"}"#, "--project", &gone][..], save].concat();
+        assert_eq!(error(&args), plain, "{save:?}");
+    }
+    assert!(!dir.join(".gone.ecproj.lock").exists());
+    assert!(!dir.join("other.ecproj").exists());
+    // a project with --bridge stays a usage error
+    assert_eq!(bin().args(["exec", "project.newFolder", "--bridge", "9877", "--project", &gone, "--save"]).output().unwrap().status.code(), Some(2));
+    let _ = std::fs::remove_dir_all(&dir);
+}
+
 /// `run --args-file` reads its command / params pairs from a file, one per line: a batch longer
 /// than a Windows command line (32 767 characters, os error 206) still runs.
 #[test]
@@ -136,7 +327,14 @@ fn run_reads_arguments_from_a_file() {
 /// Run with `stdin` piped in; returns (exit code, stdout, stderr).
 fn run_with_stdin(args: &[&str], stdin: &str) -> (i32, String, String) {
     let mut child = bin().args(args).stdin(Stdio::piped()).stdout(Stdio::piped()).stderr(Stdio::piped()).spawn().unwrap();
-    child.stdin.take().unwrap().write_all(stdin.as_bytes()).unwrap();
+    // The child may exit before it has read all of standard input (the usage-error cases below
+    // exit with code 2 without draining stdin), so a broken pipe while writing is that early
+    // exit, not a failure of this helper; the assertions on the returned output still hold.
+    let mut stdin_pipe = child.stdin.take().unwrap();
+    if let Err(err) = stdin_pipe.write_all(stdin.as_bytes()) {
+        assert_eq!(err.kind(), std::io::ErrorKind::BrokenPipe, "writing the child's standard input: {err}");
+    }
+    drop(stdin_pipe);
     let out = child.wait_with_output().unwrap();
     (out.status.code().unwrap_or(-1), String::from_utf8_lossy(&out.stdout).into_owned(), String::from_utf8_lossy(&out.stderr).into_owned())
 }