diff --git a/rewatch/src/build.rs b/rewatch/src/build.rs index f59fa165339..f85a911e12a 100644 --- a/rewatch/src/build.rs +++ b/rewatch/src/build.rs @@ -14,6 +14,7 @@ use crate::build::compile::{mark_modules_with_deleted_deps_dirty, mark_modules_w use crate::build::compiler_info::{CompilerCheckResult, verify_compiler_info, write_compiler_info}; use crate::helpers::emojis::*; use crate::helpers::{self}; +use crate::lock::{LockKind, drop_lock, get_lock_or_exit}; use crate::project_context::ProjectContext; use crate::sourcedirs; use anyhow::{Context, Result, anyhow}; @@ -146,7 +147,13 @@ pub fn initialize_build( return Err(anyhow!("Failed to validate package dependencies")); } - let mut build_state = BuildCommandState::new(project_context, packages, compiler, warn_error); + let mut build_state = BuildCommandState::new( + path.to_path_buf(), + project_context, + packages, + compiler, + warn_error, + ); packages::parse_packages(&mut build_state)?; let compile_assets_state = read_compile_state::read(&mut build_state)?; @@ -238,6 +245,10 @@ pub fn incremental_build( create_sourcedirs: bool, plain_output: bool, ) -> Result<(), IncrementalBuildError> { + let build_folder = build_state.root_folder.to_string_lossy().to_string(); + + let _lock = get_lock_or_exit(LockKind::Build, &build_folder); + logs::initialize(&build_state.packages); let num_dirty_modules = build_state.modules.values().filter(|m| is_dirty(m)).count() as u64; let pb = if !plain_output && show_progress { @@ -281,6 +292,8 @@ pub fn incremental_build( } eprintln!("{}", &err); + let _lock = drop_lock(LockKind::Build, &build_folder); + return Err(IncrementalBuildError { kind: IncrementalBuildErrorKind::SourceFileParseError, plain_output, @@ -343,9 +356,13 @@ pub fn incremental_build( || pb.inc(1), |size| pb.set_length(size), ) - .map_err(|e| IncrementalBuildError { - kind: IncrementalBuildErrorKind::CompileError(Some(e.to_string())), - plain_output, + .map_err(|e| { + let _lock = drop_lock(LockKind::Build, &build_folder); + + IncrementalBuildError { + kind: IncrementalBuildErrorKind::CompileError(Some(e.to_string())), + plain_output, + } })?; let compile_duration = start_compiling.elapsed(); @@ -379,6 +396,9 @@ pub fn incremental_build( if helpers::contains_ascii_characters(&compile_errors) { eprintln!("{}", &compile_errors); } + + let _lock = drop_lock(LockKind::Build, &build_folder); + Err(IncrementalBuildError { kind: IncrementalBuildErrorKind::CompileError(None), plain_output, @@ -409,6 +429,7 @@ pub fn incremental_build( // Write per-package compiler metadata to `lib/bs/compiler-info.json` (idempotent) write_compiler_info(build_state); + let _lock = drop_lock(LockKind::Build, &build_folder); Ok(()) } } diff --git a/rewatch/src/build/build_types.rs b/rewatch/src/build/build_types.rs index e80444daaf1..75bed1f10c3 100644 --- a/rewatch/src/build/build_types.rs +++ b/rewatch/src/build/build_types.rs @@ -122,6 +122,7 @@ pub struct BuildState { /// - This prevents the "code smell" of optional fields that are None for some commands #[derive(Debug)] pub struct BuildCommandState { + pub root_folder: PathBuf, pub build_state: BuildState, // Command-line --warn-error flag override (takes precedence over rescript.json config) pub warn_error_override: Option, @@ -171,12 +172,14 @@ impl BuildState { impl BuildCommandState { pub fn new( + root_folder: PathBuf, project_context: ProjectContext, packages: AHashMap, compiler: CompilerInfo, warn_error_override: Option, ) -> Self { Self { + root_folder, build_state: BuildState::new(project_context, packages, compiler), warn_error_override, } diff --git a/rewatch/src/lock.rs b/rewatch/src/lock.rs index 7d5528053b9..60f0222f827 100644 --- a/rewatch/src/lock.rs +++ b/rewatch/src/lock.rs @@ -1,20 +1,41 @@ +use anyhow::Result; +use notify::{Config, Event, EventKind, RecommendedWatcher, RecursiveMode, Watcher}; use std::fs; use std::fs::File; use std::io::Write; use std::path::Path; use std::process; +use std::sync::Arc; +use std::time::{Duration, SystemTime}; use sysinfo::{PidExt, ProcessExt, System, SystemExt}; +use crate::queue::FifoQueue; +use crate::queue::*; + /* This locking mechanism is meant to never be deleted. Instead, it stores the PID of the process * that's running, when trying to aquire a lock, it checks wether that process is still running. If * not, it rewrites the lockfile to have its own PID instead. */ -pub static LOCKFILE: &str = "rescript.lock"; +pub enum AwaitLockError { + Watcher(notify::Error), + Timeout(String), +} + +impl std::fmt::Display for AwaitLockError { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + let msg = match self { + AwaitLockError::Watcher(error) => format!("Error starting file watcher {}", error), + AwaitLockError::Timeout(path) => format!("Timeout awaiting lockfile {}", path), + }; + write!(f, "{msg}") + } +} pub enum Error { Locked(u32), + AwaitingLockFile(AwaitLockError), ParsingLockfile(std::num::ParseIntError), - ReadingLockfile(std::io::Error), + ReadingLockfile(LockKind, std::io::Error), WritingLockfile(std::io::Error), ProjectFolderMissing(std::path::PathBuf), } @@ -28,14 +49,20 @@ impl std::fmt::Display for Error { Error::ParsingLockfile(e) => format!( "Could not parse lockfile: \n {e} \n (try removing it and running the command again)" ), - Error::ReadingLockfile(e) => { - format!("Could not read lockfile: \n {e} \n (try removing it and running the command again)") + Error::ReadingLockfile(kind, e) => { + format!( + "Could not read lockfile: {}, \n {e} \n (try removing it and running the command again)", + kind.file_name() + ) } Error::WritingLockfile(e) => format!("Could not write lockfile: \n {e}"), Error::ProjectFolderMissing(path) => format!( "Could not write lockfile because the specified project folder does not exist: {}", path.to_string_lossy() ), + Error::AwaitingLockFile(await_lock_error) => { + format!("Error awaiting lockfile: {await_lock_error}") + } }; write!(f, "{msg}") } @@ -72,28 +99,101 @@ fn pid_matches_current_process(to_check_pid: u32) -> bool { }) } -pub fn get(folder: &str) -> Lock { +#[derive(Clone, Copy)] +pub enum LockKind { + Watch, + Build, +} + +impl LockKind { + pub fn file_name(&self) -> String { + String::from(match self { + LockKind::Watch => "watch.lock", + LockKind::Build => "build.lock", + }) + } +} + +pub const TIMEOUT_SECONDS: u64 = 60; + +pub fn await_lock_deletion(location: &Path, kind: LockKind) -> Result<(), Error> { + let now = SystemTime::now(); + let lock_path = location.join(kind.file_name()); + let queue = Arc::new(FifoQueue::>::new()); + let producer = queue.clone(); + + let mut watcher = RecommendedWatcher::new(move |res| producer.push(res), Config::default()) + .map_err(|e| Error::AwaitingLockFile(AwaitLockError::Watcher(e)))?; + + watcher + .watch(location, RecursiveMode::NonRecursive) + .map_err(|e| Error::AwaitingLockFile(AwaitLockError::Watcher(e)))?; + + loop { + if !lock_path.exists() { + return Ok(()); + } + + while !queue.is_empty() { + match queue.pop() { + Ok(Event { + kind: EventKind::Remove(_), + paths, + .. + }) if paths + .iter() + .any(|path| path == &lock_path || path.ends_with(kind.file_name())) => + { + return Ok(()); + } + Ok(_) | Err(_) => (), + } + } + + match now.elapsed() { + Ok(elapsed) if elapsed < Duration::from_secs(TIMEOUT_SECONDS) => { + std::thread::sleep(Duration::from_millis(50)); + } + Ok(_) | Err(_) => { + return Err(Error::AwaitingLockFile(AwaitLockError::Timeout( + lock_path.to_string_lossy().to_string(), + ))); + } + } + } +} + +pub fn get(kind: LockKind, folder: &str) -> Lock { let project_folder = Path::new(folder); if !project_folder.exists() { return Lock::Error(Error::ProjectFolderMissing(project_folder.to_path_buf())); } let lib_dir = project_folder.join("lib"); - let location = lib_dir.join(LOCKFILE); + let location = lib_dir.join(kind.file_name()); let pid = process::id(); // When a lockfile already exists we parse its PID: if the process is still alive we refuse to // proceed, otherwise we will overwrite the stale lock with our own PID. - match fs::read_to_string(&location) { - Ok(contents) => match contents.parse::() { - Ok(parsed_pid) if pid_matches_current_process(parsed_pid) => { - return Lock::Error(Error::Locked(parsed_pid)); - } - Ok(_) => (), - Err(e) => return Lock::Error(Error::ParsingLockfile(e)), - }, - Err(e) if e.kind() == std::io::ErrorKind::NotFound => (), - Err(e) => return Lock::Error(Error::ReadingLockfile(e)), + loop { + match fs::read_to_string(&location) { + Ok(contents) => match contents.parse::() { + Ok(parsed_pid) if pid_matches_current_process(parsed_pid) => match kind { + LockKind::Build => { + println!("Waiting for other build to finish..."); + match await_lock_deletion(&lib_dir, kind) { + Ok(_) => continue, + Err(_) => return Lock::Error(Error::Locked(parsed_pid)), + }; + } + LockKind::Watch => return Lock::Error(Error::Locked(parsed_pid)), + }, + Ok(_) => break, + Err(e) => return Lock::Error(Error::ParsingLockfile(e)), + }, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => break, + Err(e) => return Lock::Error(Error::ReadingLockfile(kind, e)), + } } if let Err(e) = fs::create_dir_all(&lib_dir) { @@ -110,10 +210,37 @@ pub fn get(folder: &str) -> Lock { } } +pub fn get_lock_or_exit(kind: LockKind, folder: &str) -> Lock { + match get(kind, folder) { + Lock::Error(error) => { + eprintln!("Could not start Rescript build: {error}"); + std::process::exit(1); + } + + acquired_lock => acquired_lock, + } +} + +pub fn drop_lock(kind: LockKind, folder: &str) -> Result<()> { + let project_folder = Path::new(folder); + if !project_folder.exists() { + return Ok(()); + } + + let lib_dir = project_folder.join("lib"); + let location = lib_dir.join(kind.file_name()); + + fs::remove_file(&location)?; + + Ok(()) +} + #[cfg(test)] mod tests { use super::*; use std::fs; + use std::thread; + use std::time::Duration; use tempfile::TempDir; #[test] @@ -121,7 +248,10 @@ mod tests { let temp_dir = TempDir::new().expect("temp dir should be created"); let missing_folder = temp_dir.path().join("missing_project"); - match get(missing_folder.to_str().expect("path should be valid")) { + match get( + LockKind::Watch, + missing_folder.to_str().expect("path should be valid"), + ) { Lock::Error(Error::ProjectFolderMissing(path)) => { assert_eq!(path, missing_folder); } @@ -140,7 +270,10 @@ mod tests { let project_folder = temp_dir.path().join("project"); fs::create_dir(&project_folder).expect("project folder should be created"); - match get(project_folder.to_str().expect("path should be valid")) { + match get( + LockKind::Watch, + project_folder.to_str().expect("path should be valid"), + ) { Lock::Aquired(_) => {} _ => panic!("expected lock to be acquired"), } @@ -150,7 +283,10 @@ mod tests { "lib directory should be created" ); assert!( - project_folder.join("lib").join(LOCKFILE).exists(), + project_folder + .join("lib") + .join(LockKind::Watch.file_name()) + .exists(), "lockfile should be created" ); } @@ -161,11 +297,89 @@ mod tests { let project_folder = temp_dir.path().join("project"); let lib_dir = project_folder.join("lib"); fs::create_dir_all(&lib_dir).expect("lib directory should be created"); - fs::write(lib_dir.join(LOCKFILE), "1").expect("lockfile should be written"); + fs::write(lib_dir.join(LockKind::Watch.file_name()), "1").expect("lockfile should be written"); - match get(project_folder.to_str().expect("path should be valid")) { + match get( + LockKind::Watch, + project_folder.to_str().expect("path should be valid"), + ) { Lock::Aquired(_) => {} _ => panic!("expected stale lock from unrelated process to be ignored"), } } + + #[test] + fn returns_locked_for_active_watch_lock() { + let temp_dir = TempDir::new().expect("temp dir should be created"); + let project_folder = temp_dir.path().join("project"); + let lib_dir = project_folder.join("lib"); + let current_pid = process::id(); + + fs::create_dir_all(&lib_dir).expect("lib directory should be created"); + fs::write(lib_dir.join(LockKind::Watch.file_name()), current_pid.to_string()) + .expect("lockfile should be written"); + + match get( + LockKind::Watch, + project_folder.to_str().expect("path should be valid"), + ) { + Lock::Error(Error::Locked(locked_pid)) => assert_eq!(locked_pid, current_pid), + _ => panic!("expected watch lock to block a second active watcher"), + } + } + + #[test] + fn waits_for_active_build_lock_to_be_removed() { + let temp_dir = TempDir::new().expect("temp dir should be created"); + let project_folder = temp_dir.path().join("project"); + let lib_dir = project_folder.join("lib"); + let build_lock_path = lib_dir.join(LockKind::Build.file_name()); + let current_pid = process::id(); + + fs::create_dir_all(&lib_dir).expect("lib directory should be created"); + fs::write(&build_lock_path, current_pid.to_string()).expect("lockfile should be written"); + + let build_lock_path_for_thread = build_lock_path.clone(); + let remover = thread::spawn(move || { + thread::sleep(Duration::from_millis(250)); + fs::remove_file(build_lock_path_for_thread).expect("lockfile should be removed"); + }); + + match get( + LockKind::Build, + project_folder.to_str().expect("path should be valid"), + ) { + Lock::Aquired(acquired_pid) => assert_eq!(acquired_pid, current_pid), + _ => panic!("expected build lock to wait for removal and then be acquired"), + } + + remover.join().expect("lockfile remover thread should complete"); + + assert_eq!( + fs::read_to_string(build_lock_path).expect("build lock should be rewritten"), + current_pid.to_string() + ); + } + + #[test] + fn drop_lock_removes_existing_build_lock() { + let temp_dir = TempDir::new().expect("temp dir should be created"); + let project_folder = temp_dir.path().join("project"); + let lib_dir = project_folder.join("lib"); + let build_lock_path = lib_dir.join(LockKind::Build.file_name()); + + fs::create_dir_all(&lib_dir).expect("lib directory should be created"); + fs::write(&build_lock_path, process::id().to_string()).expect("lockfile should be written"); + + drop_lock( + LockKind::Build, + project_folder.to_str().expect("path should be valid"), + ) + .expect("build lock should be removed"); + + assert!( + !build_lock_path.exists(), + "drop_lock should remove the build lock file" + ); + } } diff --git a/rewatch/src/main.rs b/rewatch/src/main.rs index c27c21e9596..10be746d04a 100644 --- a/rewatch/src/main.rs +++ b/rewatch/src/main.rs @@ -3,7 +3,11 @@ use console::Term; use log::LevelFilter; use std::{io::Write, path::Path}; -use rescript::{build, cli, cmd, format, lock, watcher}; +use rescript::{ + build, cli, cmd, format, + lock::{LockKind, drop_lock, get_lock_or_exit}, + watcher, +}; fn main() -> Result<()> { let cli = cli::parse_with_default().unwrap_or_else(|err| err.exit()); @@ -42,8 +46,6 @@ fn main() -> Result<()> { std::process::exit(0); } cli::Command::Build(build_args) => { - let _lock = get_lock(&build_args.folder); - match build::build( &build_args.filter, Path::new(&build_args.folder as &str), @@ -66,7 +68,7 @@ fn main() -> Result<()> { }; } cli::Command::Watch(watch_args) => { - let _lock = get_lock(&watch_args.folder); + let _lock = get_lock_or_exit(LockKind::Watch, &watch_args.folder); match watcher::start( &watch_args.filter, @@ -85,20 +87,13 @@ fn main() -> Result<()> { } } cli::Command::Clean { folder } => { - let _lock = get_lock(&folder); - build::clean::clean(Path::new(&folder as &str), show_progress, plain_output) - } - cli::Command::Format { stdin, check, files } => format::format(stdin, check, files), - } -} + let _lock = get_lock_or_exit(LockKind::Build, &folder); + let result = build::clean::clean(Path::new(&folder as &str), show_progress, plain_output); + let _lock = drop_lock(LockKind::Build, &folder); -fn get_lock(folder: &str) -> lock::Lock { - match lock::get(folder) { - lock::Lock::Error(error) => { - eprintln!("Could not start ReScript build: {error}"); - std::process::exit(1); + result } - acquired_lock => acquired_lock, + cli::Command::Format { stdin, check, files } => format::format(stdin, check, files), } } diff --git a/rewatch/src/watcher.rs b/rewatch/src/watcher.rs index 9a0ccf1a2d4..8a37cdaf618 100644 --- a/rewatch/src/watcher.rs +++ b/rewatch/src/watcher.rs @@ -6,7 +6,7 @@ use crate::config; use crate::helpers; use crate::helpers::StrippedVerbatimPath; use crate::helpers::emojis::*; -use crate::lock::LOCKFILE; +use crate::lock::LockKind; use crate::queue::FifoQueue; use crate::queue::*; use anyhow::{Context, Result}; @@ -109,7 +109,7 @@ fn compute_watch_paths(build_state: &BuildCommandState, root: &Path) -> Vec<(Pat } } - // Watch the lib/ directory for the lockfile (rescript.lock lives in lib/) + // Watch the lib/ directory for the watcher lockfile (watch.lock lives in lib/) let lib_dir = root.join("lib"); if lib_dir.exists() { insert(lib_dir, RecursiveMode::NonRecursive); @@ -202,8 +202,11 @@ async fn async_watch( } for event in events { - // if there is a file named rescript.lock in the events path, we can quit the watcher - if event.paths.iter().any(|path| path.ends_with(LOCKFILE)) + // If watch.lock is removed, we can quit the watcher. + if event + .paths + .iter() + .any(|path| path.ends_with(LockKind::Watch.file_name())) && let EventKind::Remove(_) = event.kind { if show_progress { diff --git a/rewatch/tests/experimental/02-experimental-features-parse-error.sh b/rewatch/tests/experimental/02-experimental-features-parse-error.sh index fd55f2dc0c4..b9b35864edb 100755 --- a/rewatch/tests/experimental/02-experimental-features-parse-error.sh +++ b/rewatch/tests/experimental/02-experimental-features-parse-error.sh @@ -18,7 +18,7 @@ out=$(rewatch build 2>&1) status=$? mv rescript.json.bak rescript.json -rm -f lib/rescript.lock +clear_locks if [ $status -eq 0 ]; then error "Expected build to fail for experimental-features list input" diff --git a/rewatch/tests/experimental/03-watch-invalid-experimental.sh b/rewatch/tests/experimental/03-watch-invalid-experimental.sh index 79557f26990..7f4f1f0f0d1 100755 --- a/rewatch/tests/experimental/03-watch-invalid-experimental.sh +++ b/rewatch/tests/experimental/03-watch-invalid-experimental.sh @@ -18,7 +18,7 @@ out=$(rewatch watch 2>&1) status=$? mv rescript.json.bak rescript.json -rm -f lib/rescript.lock +clear_locks if [ $status -eq 0 ]; then error "Expected watch to fail for invalid experimental-features list" diff --git a/rewatch/tests/lock/01-lock-when-watching.sh b/rewatch/tests/lock/01-lock-when-watching.sh index 9a4c46efe57..c97706f4815 100755 --- a/rewatch/tests/lock/01-lock-when-watching.sh +++ b/rewatch/tests/lock/01-lock-when-watching.sh @@ -3,9 +3,14 @@ cd $(dirname $0) source "../utils.sh" cd ../../testrepo -bold "Test: It should lock - when watching" +bold "Test: watch.lock blocks watch and stays separate from build.lock" -sleep 1 +cleanup() { + exit_watcher + rm -f rewatch.log second-watch.log +} + +trap cleanup EXIT error_output=$(rewatch clean 2>&1) if [ $? -eq 0 ]; @@ -17,37 +22,73 @@ else exit 1 fi +clear_locks + rewatch_bg watch > /dev/null 2>&1 & success "Watcher Started" -sleep 2 +if ! wait_for_file "lib/watch.lock" 20; then + error "watch.lock was not created" + exit 1 +fi + +success "watch.lock created" -if rewatch build 2>&1 | grep 'Could not start ReScript build:' &> /dev/null; +if rewatch watch > second-watch.log 2>&1; then - success "Lock is correctly set" - exit_watcher + error "Second watcher should have been blocked by watch.lock" + cat second-watch.log + exit 1 +elif grep 'Could not start Rescript build:' second-watch.log > /dev/null; +then + success "Second watcher is blocked by watch.lock" else - error "Not setting lock correctly" - exit_watcher + error "Second watcher failed without the expected lock message" + cat second-watch.log exit 1 fi -sleep 1 +build_output=$(rewatch build 2>&1) +build_status=$? -touch tmp.txt -rewatch_bg watch > tmp.txt 2>&1 & -success "Watcher Started" +if [ $build_status -ne 0 ]; +then + error "Build should succeed while watch.lock is active" + printf "%s\n" "$build_output" + exit 1 +fi -sleep 2 +success "Build succeeds while watcher is running" -if cat tmp.txt | grep 'Could not start ReScript build:' &> /dev/null; +if wait_for_file_gone "lib/build.lock" 10; then - error "Lock not removed correctly" - exit_watcher + success "build.lock is removed after build finishes" +else + error "build.lock was not removed after build" exit 1 +fi + +if [ -f lib/watch.lock ]; +then + success "watch.lock remains while watcher is running" else - success "Lock removed correctly" - exit_watcher + error "watch.lock disappeared while watcher was still running" + exit 1 +fi + +exit_watcher + +if ! wait_for_file_gone "lib/watch.lock" 10; then + error "watch.lock was not removed" + exit 1 fi -rm tmp.txt +rewatch_bg watch > rewatch.log 2>&1 & + +if wait_for_file "lib/watch.lock" 20; then + success "Watcher can start again after watch.lock is removed" +else + error "Watcher did not restart after watch.lock removal" + cat rewatch.log + exit 1 +fi diff --git a/rewatch/tests/utils.sh b/rewatch/tests/utils.sh index 9834da3d605..0955c2681db 100644 --- a/rewatch/tests/utils.sh +++ b/rewatch/tests/utils.sh @@ -52,7 +52,11 @@ replace() { } exit_watcher() { - rm -f lib/rescript.lock + rm -f lib/watch.lock +} + +clear_locks() { + rm -f lib/watch.lock lib/build.lock } wait_for_file() { diff --git a/rewatch/tests/watch/04-watch-config-change.sh b/rewatch/tests/watch/04-watch-config-change.sh index 8e2d3b7aecb..519f77f5e82 100755 --- a/rewatch/tests/watch/04-watch-config-change.sh +++ b/rewatch/tests/watch/04-watch-config-change.sh @@ -52,7 +52,7 @@ else fi # Verify the watcher is still running (didn't crash on config change) -if [ -f lib/rescript.lock ]; then +if [ -f lib/watch.lock ]; then success "Watcher still running after config change" else error "Watcher crashed after config change"