From 3377441a3deb7fb4b3b1ceeaacccaabfb10d5f70 Mon Sep 17 00:00:00 2001 From: Florian Hammerschmidt Date: Fri, 9 Oct 2026 14:53:50 +0200 Subject: [PATCH 1/8] Keep watcher running after full rebuild initialization errors Signed-off-by: Florian Hammerschmidt --- CHANGELOG.md | 1 + rewatch/src/watcher.rs | 16 ++- rewatch/tests/suite.sh | 1 + .../07-watch-recovers-from-invalid-config.sh | 103 ++++++++++++++++++ 4 files changed, 116 insertions(+), 5 deletions(-) create mode 100755 rewatch/tests/watch/07-watch-recovers-from-invalid-config.sh diff --git a/CHANGELOG.md b/CHANGELOG.md index 4a9390cd84b..483281dac95 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,7 @@ #### :bug: Bug fix +- Keep watch mode running after full rebuild initialization fails so it can recover after a later valid change. https://github.com/rescript-lang/rescript/pull/8641 - Fix compiler crashes when wrapping values in `Some` or passing optional arguments whose types come from a transitive dependency. https://github.com/rescript-lang/rescript/pull/8719 - Fix unused-pattern warnings for variant spreads reported without a location. https://github.com/rescript-lang/rescript/pull/8744 - Keep `@warning("-4")` on `if` expressions when formatting; the formatter dropped it. https://github.com/rescript-lang/rescript/pull/8741 diff --git a/rewatch/src/watcher.rs b/rewatch/src/watcher.rs index 9984d6908b4..a34489b7e97 100644 --- a/rewatch/src/watcher.rs +++ b/rewatch/src/watcher.rs @@ -569,7 +569,7 @@ async fn async_watch( let timing_total = Instant::now(); // Reinitialization runs cleanup for previous build artifacts, so full rebuilds need // the same build lock boundary as regular `rescript build`. - let result = build::with_build_lock(path, || { + let result = build::with_build_lock(path, || -> Result<_> { let mut next_build_state = build::initialize_build( None, filter, @@ -581,7 +581,7 @@ async fn async_watch( features.clone(), SourceMapCommand::Watch, ) - .expect("Could not initialize build"); + .context("Could not initialize build")?; // Full rebuilds can be triggered by editor atomic saves that surface as rename events. // Preserve warnings and blocked dirty modules when fresh state replaces the previous one. @@ -603,10 +603,10 @@ async fn async_watch( plain_output, ); build::write_build_ninja(&build_state); - result + Ok(result) }); match result { - Ok(result) => { + Ok(Ok(result)) => { finish_successful_watch_compile( after_build.clone(), timing_total, @@ -617,7 +617,13 @@ async fn async_watch( result, ); } - Err(_) => { + Ok(Err(_)) => { + if should_clear_screen(clear_screen, show_progress, plain_output) { + print_build_failed_footer(); + } + } + Err(error) => { + eprintln!("{error:#}"); if should_clear_screen(clear_screen, show_progress, plain_output) { print_build_failed_footer(); } diff --git a/rewatch/tests/suite.sh b/rewatch/tests/suite.sh index 3209eae41d8..0efc9e26469 100755 --- a/rewatch/tests/suite.sh +++ b/rewatch/tests/suite.sh @@ -145,6 +145,7 @@ fi ./watch/05-watch-ignores-non-source.sh && ./watch/06-watch-missing-source-folder.sh && ./watch/07-changed-interface-after-failed-implementation.sh && +./watch/07-watch-recovers-from-invalid-config.sh && # Lock tests ./lock/01-lock-when-watching.sh && diff --git a/rewatch/tests/watch/07-watch-recovers-from-invalid-config.sh b/rewatch/tests/watch/07-watch-recovers-from-invalid-config.sh new file mode 100755 index 00000000000..775e54e0be1 --- /dev/null +++ b/rewatch/tests/watch/07-watch-recovers-from-invalid-config.sh @@ -0,0 +1,103 @@ +#!/bin/bash +cd $(dirname $0) +source "../utils.sh" +cd ../../testrepo + +bold "Test: Watcher recovers from an invalid config change" + +wait_for_pattern() { + local file="$1" pattern="$2" timeout="${3:-30}" + while [ "$timeout" -gt 0 ]; do + grep -q "$pattern" "$file" 2> /dev/null && return 0 + sleep 1 + timeout=$((timeout - 1)) + done + return 1 +} + +wait_for_pattern_count() { + local file="$1" pattern="$2" expected="$3" timeout="${4:-30}" + while [ "$timeout" -gt 0 ]; do + local count + count=$(grep -c "$pattern" "$file" 2> /dev/null || true) + [ "$count" -ge "$expected" ] && return 0 + sleep 1 + timeout=$((timeout - 1)) + done + return 1 +} + +rewatch clean > /dev/null 2>&1 +cp rescript.json rescript.json.watch-recovery.bak +rewatch_bg watch > rewatch.log 2>&1 & + +if ! wait_for_file "./src/Test.mjs" 20 || ! wait_for_file "lib/watch.lock" 20; then + error "Initial watch build did not complete" + cat rewatch.log + exit_watcher + mv rescript.json.watch-recovery.bak rescript.json + exit 1 +fi + +if ! wait_for_pattern rewatch.log "Finished initial compilation" 60; then + error "Initial watch build did not finish" + cat rewatch.log + exit_watcher + mv rescript.json.watch-recovery.bak rescript.json + exit 1 +fi + +initial_completed=$(grep -c "Finished compilation" rewatch.log 2> /dev/null || true) +printf '{"name":' > rescript.json.watch-recovery.next +mv rescript.json.watch-recovery.next rescript.json + +if ! wait_for_pattern rewatch.log "Could not initialize build" 30; then + error "Watcher did not report the invalid config" + cat rewatch.log + exit_watcher + mv rescript.json.watch-recovery.bak rescript.json + exit 1 +fi + +node -e ' + const fs = require("fs"); + const config = JSON.parse(fs.readFileSync(process.argv[1], "utf8")); + config.suffix = ".recovered.mjs"; + fs.writeFileSync(process.argv[2], `${JSON.stringify(config, null, 2)}\n`); +' rescript.json.watch-recovery.bak rescript.json.watch-recovery.next +mv rescript.json.watch-recovery.next rescript.json + +expected_completed=$((initial_completed + 1)) +if ! wait_for_pattern_count rewatch.log "Finished compilation" "$expected_completed" 30; then + error "Watcher did not recover after restoring a valid config" + cat rewatch.log + exit_watcher + mv rescript.json.watch-recovery.bak rescript.json + exit 1 +fi + +echo '// watch-recovery-test' >> src/Test.res +if ! wait_for_file "./src/Test.recovered.mjs" 20; then + error "Recovered watcher did not compile with the restored config" + cat rewatch.log + exit_watcher + mv rescript.json.watch-recovery.bak rescript.json + git checkout -- src/Test.res + exit 1 +fi + +exit_watcher +rewatch clean > /dev/null 2>&1 +mv rescript.json.watch-recovery.bak rescript.json +git checkout -- src/Test.res +rm -f rewatch.log +rewatch build > /dev/null 2>&1 + +if git diff --exit-code . > /dev/null 2>&1 && [ -z "$(git ls-files --others --exclude-standard .)" ]; then + success "Watcher recovered from invalid config" +else + error "Watcher recovery test left repository changes" + git diff . + git ls-files --others --exclude-standard . + exit 1 +fi From f83cd4ffd86edb9d53890bdd86255bdc4534c3c7 Mon Sep 17 00:00:00 2001 From: Florian Hammerschmidt Date: Fri, 9 Oct 2026 15:55:07 +0200 Subject: [PATCH 2/8] Retry watcher initialization and watch invalid dependencies Signed-off-by: Florian Hammerschmidt --- CHANGELOG.md | 2 +- rewatch/src/build/packages.rs | 94 ++++----- rewatch/src/watcher.rs | 186 ++++++++++++++---- rewatch/tests/suite.sh | 1 + .../07-watch-recovers-from-invalid-config.sh | 78 ++++---- ...08-watch-recovers-new-dependency-config.sh | 83 ++++++++ 6 files changed, 329 insertions(+), 115 deletions(-) create mode 100755 rewatch/tests/watch/08-watch-recovers-new-dependency-config.sh diff --git a/CHANGELOG.md b/CHANGELOG.md index 483281dac95..9efec79b99f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,7 +23,7 @@ #### :bug: Bug fix -- Keep watch mode running after full rebuild initialization fails so it can recover after a later valid change. https://github.com/rescript-lang/rescript/pull/8641 +- Keep watch mode running after full rebuild initialization fails, retry initialization on source edits, and watch newly added dependencies for configuration fixes. https://github.com/rescript-lang/rescript/pull/8641 - Fix compiler crashes when wrapping values in `Some` or passing optional arguments whose types come from a transitive dependency. https://github.com/rescript-lang/rescript/pull/8719 - Fix unused-pattern warnings for variant spreads reported without a location. https://github.com/rescript-lang/rescript/pull/8744 - Keep `@warning("-4")` on `if` expressions when formatting; the formatter dropped it. https://github.com/rescript-lang/rescript/pull/8741 diff --git a/rewatch/src/build/packages.rs b/rewatch/src/build/packages.rs index 7a820b9b557..c544d738de0 100644 --- a/rewatch/src/build/packages.rs +++ b/rewatch/src/build/packages.rs @@ -8,12 +8,13 @@ use crate::helpers::StrippedVerbatimPath; use crate::helpers::emojis::*; use crate::project_context::{MonoRepoContext, ProjectContext}; use ahash::{AHashMap, AHashSet}; -use anyhow::{Result, anyhow}; +use anyhow::{Context, Result, anyhow}; use console::style; use log::debug; use rayon::prelude::*; use std::collections::hash_map::Entry; use std::error; +use std::fmt; use std::fs::{self}; use std::hash::{Hash, Hasher}; use std::path::{Path, PathBuf}; @@ -282,6 +283,30 @@ pub fn read_dependency( Ok(canonical_path) } +/// Retain the resolved directory even when its config cannot be read, so watch mode can +/// observe a correction to a dependency that was absent from the previous build state. +#[derive(Debug)] +pub struct DependencyConfigError { + pub package_path: PathBuf, + source: anyhow::Error, +} + +impl fmt::Display for DependencyConfigError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!( + f, + "Could not read dependency config at {}", + self.package_path.display() + ) + } +} + +impl error::Error for DependencyConfigError { + fn source(&self) -> Option<&(dyn error::Error + 'static)> { + Some(self.source.as_ref()) + } +} + /// Given a config, recursively finds all dependencies. /// 1. It starts with registering dependencies and /// prevents the operation for the ones which are already @@ -296,7 +321,7 @@ fn read_dependencies( show_progress: bool, is_local_dep: bool, prod: bool, -) -> Vec { +) -> Result> { let mut dependencies: Vec = package_config.get_dependency_names(); // Concatenate dev dependencies if is_local_dep is true and not in prod mode @@ -311,17 +336,14 @@ fn read_dependencies( // Package already registered - check for duplicate (different path) // Re-resolve from current package and from root to compare paths if let Ok(current_path) = read_dependency(package_name, package_config, project_context) - && let Ok(chosen_path) = read_dependency(package_name, &project_context.current_config, project_context) + && let Ok(chosen_path) = + read_dependency(package_name, &project_context.current_config, project_context) && current_path != chosen_path { // Different paths - this is a duplicate let root_path = project_context.get_root_path(); - let chosen_relative = chosen_path - .strip_prefix(root_path) - .unwrap_or(&chosen_path); - let duplicate_relative = current_path - .strip_prefix(root_path) - .unwrap_or(¤t_path); + let chosen_relative = chosen_path.strip_prefix(root_path).unwrap_or(&chosen_path); + let duplicate_relative = current_path.strip_prefix(root_path).unwrap_or(¤t_path); let current_package_path = package_config .path .parent() @@ -348,39 +370,22 @@ fn read_dependencies( .collect::>() // Read all config files in parallel instead of blocking .par_iter() - .map(|package_name| { - let (config, canonical_path) = - match read_dependency(package_name, package_config, project_context) { - Err(error) => { - if show_progress { - println!( - "{} {} Error building package tree. {}", - style("[1/2]").bold().dim(), - CROSS, - error - ); - } - - let parent_path_str = project_context.get_root_path().to_string_lossy(); - log::error!( - "Could not build package tree reading dependency '{package_name}' at path '{parent_path_str}'. Error: {error}", + .map(|package_name| -> Result { + let canonical_path = + read_dependency(package_name, package_config, project_context).inspect_err(|error| { + if show_progress { + println!( + "{} {} Error building package tree. {}", + style("[1/2]").bold().dim(), + CROSS, + error ); - - std::process::exit(2) - } - Ok(canonical_path) => { - match read_config(&canonical_path) { - Ok(config) => (config, canonical_path), - Err(error) => { - let parent_path_str = project_context.get_root_path().to_string_lossy(); - log::error!( - "Could not build package tree for '{package_name}' at path '{parent_path_str}'. Error: {error}", - ); - std::process::exit(2) - } - } } - }; + })?; + let config = read_config(&canonical_path).map_err(|source| DependencyConfigError { + package_path: canonical_path.clone(), + source, + })?; let is_local_dep = is_local_dependency(project_context, package_name, &canonical_path); @@ -391,15 +396,16 @@ fn read_dependencies( show_progress, is_local_dep, prod, - ); + ) + .with_context(|| format!("Could not read dependencies of '{package_name}'"))?; - Dependency { + Ok(Dependency { name: package_name.to_owned(), config, path: canonical_path, dependencies, is_local_dep, - } + }) }) .collect() } @@ -585,7 +591,7 @@ fn read_packages( show_progress, /* is local dep */ true, prod, - )); + )?); for d in dependencies.iter() { if !map.contains_key(&d.name) { diff --git a/rewatch/src/watcher.rs b/rewatch/src/watcher.rs index a34489b7e97..f8b5c83ed7e 100644 --- a/rewatch/src/watcher.rs +++ b/rewatch/src/watcher.rs @@ -25,6 +25,25 @@ enum CompileType { None, } +fn compile_type_for_source_event(kind: EventKind, initialization_failed: bool) -> CompileType { + match kind { + EventKind::Remove(_) + | EventKind::Any + | EventKind::Create(_) + | EventKind::Modify(ModifyKind::Name(_)) => CompileType::Full, + EventKind::Modify(ModifyKind::Data(_) | ModifyKind::Any) => { + if initialization_failed { + CompileType::Full + } else { + CompileType::Incremental + } + } + EventKind::Access(_) + | EventKind::Other + | EventKind::Modify(ModifyKind::Metadata(_) | ModifyKind::Other) => CompileType::None, + } +} + type WatchPaths = Vec<(PathBuf, RecursiveMode)>; type StartupBuildResult = ( BuildCommandState, @@ -249,6 +268,19 @@ fn update_watches( kept } +fn watch_paths_after_initialization_error(current: &[RegisteredWatch], error: &anyhow::Error) -> WatchPaths { + let mut next: WatchPaths = current + .iter() + .map(|watch| (watch.path.clone(), watch.mode)) + .collect(); + if let Some(error) = error.downcast_ref::() + && !next.iter().any(|(path, _)| *path == error.package_path) + { + next.push((error.package_path.clone(), RecursiveMode::NonRecursive)); + } + next +} + fn carry_forward_compile_state(previous: &BuildCommandState, next: &mut BuildCommandState) { for (module_name, next_module) in next.build_state.modules.iter_mut() { let Some(previous_module) = previous.build_state.modules.get(module_name) else { @@ -362,6 +394,7 @@ async fn async_watch( ) -> Result<()> { let mut build_state = initial_build_state; let mut needs_compile_type = CompileType::None; + let mut initialization_failed = false; loop { if ctrlc_pressed.load(Ordering::SeqCst) { cleanup_before_watch_exit(path, &build_state, show_progress, "\nExiting..."); @@ -425,31 +458,35 @@ async fn async_watch( for path in paths { let path_buf = path.to_path_buf(); - match (needs_compile_type, event.kind) { + match ( + needs_compile_type, + compile_type_for_source_event(event.kind, initialization_failed), + ) { ( CompileType::Incremental | CompileType::None, - // when we have a name change, create or remove event we need to do a full compile - EventKind::Remove(_) - | EventKind::Any - | EventKind::Create(_) - | EventKind::Modify(ModifyKind::Name(_)), + // Structural changes and edits after failed initialization need a full scan. + CompileType::Full, ) => { // if we are going to do a full compile, we don't need to bother marking // files dirty because we do a full scan anyway - log::debug!("received {:?} while needs_compile_type was {needs_compile_type:?} -> full compile", event.kind); + log::debug!( + "received {:?} while needs_compile_type was {needs_compile_type:?} -> full compile", + event.kind + ); needs_compile_type = CompileType::Full; } ( CompileType::None | CompileType::Incremental, - // when we have a data change event, we can do an incremental compile - EventKind::Modify(ModifyKind::Data(_)) | - // windows sends ModifyKind::Any on file content changes - EventKind::Modify(ModifyKind::Any), + // Content edits can compile incrementally while initialization is valid. + CompileType::Incremental, ) => { // if we are going to compile incrementally, we need to mark the exact files // dirty - log::debug!("received {:?} while needs_compile_type was {needs_compile_type:?} -> incremental compile", event.kind); + log::debug!( + "received {:?} while needs_compile_type was {needs_compile_type:?} -> incremental compile", + event.kind + ); if let Ok(canonicalized_path_buf) = path_buf .canonicalize() .map(StrippedVerbatimPath::to_stripped_verbatim_path) @@ -467,33 +504,34 @@ async fn async_watch( if let Some(module) = build_state.build_state.modules.get_mut(&module_name) { match module.source_type { SourceType::SourceFile(ref mut source_file) => { - let canonicalized_implementation_file = - package.path.join(&source_file.implementation.path); - if canonicalized_path_buf == canonicalized_implementation_file { - if let Ok(modified) = - canonicalized_path_buf.metadata().and_then(|x| x.modified()) - { - source_file.implementation.last_modified = modified; - }; - source_file.implementation.parse_dirty = true; - break; - } - - // mark the interface file dirty - if let Some(ref mut interface) = source_file.interface { - let canonicalized_interface_file = - package.path.join(&interface.path); - if canonicalized_path_buf == canonicalized_interface_file { + let canonicalized_implementation_file = + package.path.join(&source_file.implementation.path); + if canonicalized_path_buf == canonicalized_implementation_file { if let Ok(modified) = canonicalized_path_buf .metadata() .and_then(|x| x.modified()) { - interface.last_modified = modified; - } - interface.parse_dirty = true; + source_file.implementation.last_modified = modified; + }; + source_file.implementation.parse_dirty = true; break; } - } + + // mark the interface file dirty + if let Some(ref mut interface) = source_file.interface { + let canonicalized_interface_file = + package.path.join(&interface.path); + if canonicalized_path_buf == canonicalized_interface_file { + if let Ok(modified) = canonicalized_path_buf + .metadata() + .and_then(|x| x.modified()) + { + interface.last_modified = modified; + } + interface.parse_dirty = true; + break; + } + } } SourceType::MlMap(_) => (), } @@ -506,10 +544,7 @@ async fn async_watch( ( CompileType::None | CompileType::Incremental, // these are not relevant events for compilation - EventKind::Access(_) - | EventKind::Other - | EventKind::Modify(ModifyKind::Metadata(_)) - | EventKind::Modify(ModifyKind::Other), + CompileType::None, ) => (), // if we already need a full compile, we don't need to check for other events (CompileType::Full, _) => (), @@ -582,6 +617,7 @@ async fn async_watch( SourceMapCommand::Watch, ) .context("Could not initialize build")?; + initialization_failed = false; // Full rebuilds can be triggered by editor atomic saves that surface as rename events. // Preserve warnings and blocked dirty modules when fresh state replaces the previous one. @@ -623,6 +659,16 @@ async fn async_watch( } } Err(error) => { + // Keep source edits on the full initialization path until the current + // configuration can be loaded. Never compile against the stale state. + initialization_failed = true; + let recovery_watch_paths = + watch_paths_after_initialization_error(¤t_watches, &error); + current_watches = update_watches( + watcher, + std::mem::take(&mut current_watches), + &recovery_watch_paths, + ); eprintln!("{error:#}"); if should_clear_screen(clear_screen, show_progress, plain_output) { print_build_failed_footer(); @@ -887,6 +933,72 @@ mod tests { } } + #[test] + fn source_edits_retry_failed_initialization() { + for kind in [ + EventKind::Modify(ModifyKind::Data(notify::event::DataChange::Content)), + EventKind::Modify(ModifyKind::Any), + ] { + assert_eq!( + compile_type_for_source_event(kind, false), + CompileType::Incremental + ); + assert_eq!(compile_type_for_source_event(kind, true), CompileType::Full); + } + + let metadata = EventKind::Modify(ModifyKind::Metadata(notify::event::MetadataKind::Any)); + assert_eq!(compile_type_for_source_event(metadata, true), CompileType::None); + } + + #[test] + fn watches_new_dependency_after_config_read_fails() { + let temp = tempfile::TempDir::new().unwrap(); + let root = temp.path().canonicalize().unwrap().to_stripped_verbatim_path(); + let dependency = root.join("node_modules/parent/node_modules/new-dependency"); + std::fs::create_dir_all(&dependency).unwrap(); + std::fs::write( + root.join("rescript.json"), + r#"{"name":"root","dependencies":["parent"]}"#, + ) + .unwrap(); + std::fs::write( + root.join("node_modules/parent/rescript.json"), + r#"{"name":"parent","dependencies":["new-dependency"]}"#, + ) + .unwrap(); + std::fs::write(dependency.join("rescript.json"), r#"{"name":"#).unwrap(); + + let context = ProjectContext::new(&root).unwrap(); + let error = build::packages::make(&None, &context, false, false, None) + .unwrap_err() + .context("Could not initialize build"); + assert!(format!("{error:#}").contains("Failed to parse rescript.json")); + + let mut current = vec![RegisteredWatch { + path: root.clone(), + mode: RecursiveMode::NonRecursive, + identity: dir_identity(&root), + }]; + let paths = watch_paths_after_initialization_error(¤t, &error); + assert_eq!(paths.len(), 2); + assert!(paths.contains(&(root, RecursiveMode::NonRecursive))); + assert!(paths.contains(&(dependency.clone(), RecursiveMode::NonRecursive))); + + // A config error in an already watched source directory must retain its recursive mode. + current.push(RegisteredWatch { + path: dependency.clone(), + mode: RecursiveMode::Recursive, + identity: dir_identity(&dependency), + }); + let paths = watch_paths_after_initialization_error(¤t, &error); + assert_eq!(paths.len(), 2); + assert!(paths.contains(&(dependency.clone(), RecursiveMode::Recursive))); + + std::fs::write(dependency.join("rescript.json"), r#"{"name":"new-dependency"}"#).unwrap(); + let recovered = build::packages::make(&None, &context, false, false, None).unwrap(); + assert!(recovered.contains_key("new-dependency")); + } + #[test] fn clears_screen_only_for_interactive_rebuilds() { assert!(should_clear_screen(true, true, false)); diff --git a/rewatch/tests/suite.sh b/rewatch/tests/suite.sh index 0efc9e26469..245c9dbafc9 100755 --- a/rewatch/tests/suite.sh +++ b/rewatch/tests/suite.sh @@ -146,6 +146,7 @@ fi ./watch/06-watch-missing-source-folder.sh && ./watch/07-changed-interface-after-failed-implementation.sh && ./watch/07-watch-recovers-from-invalid-config.sh && +./watch/08-watch-recovers-new-dependency-config.sh && # Lock tests ./lock/01-lock-when-watching.sh && diff --git a/rewatch/tests/watch/07-watch-recovers-from-invalid-config.sh b/rewatch/tests/watch/07-watch-recovers-from-invalid-config.sh index 775e54e0be1..2fd76136830 100755 --- a/rewatch/tests/watch/07-watch-recovers-from-invalid-config.sh +++ b/rewatch/tests/watch/07-watch-recovers-from-invalid-config.sh @@ -5,36 +5,23 @@ cd ../../testrepo bold "Test: Watcher recovers from an invalid config change" -wait_for_pattern() { - local file="$1" pattern="$2" timeout="${3:-30}" - while [ "$timeout" -gt 0 ]; do - grep -q "$pattern" "$file" 2> /dev/null && return 0 - sleep 1 - timeout=$((timeout - 1)) - done - return 1 +stop_watcher() { + if ! exit_watcher; then + exit 1 + fi } -wait_for_pattern_count() { - local file="$1" pattern="$2" expected="$3" timeout="${4:-30}" - while [ "$timeout" -gt 0 ]; do - local count - count=$(grep -c "$pattern" "$file" 2> /dev/null || true) - [ "$count" -ge "$expected" ] && return 0 - sleep 1 - timeout=$((timeout - 1)) - done - return 1 -} - -rewatch clean > /dev/null 2>&1 +if ! rewatch clean > /dev/null 2>&1; then + error "Could not clean before watcher recovery test" + exit 1 +fi cp rescript.json rescript.json.watch-recovery.bak rewatch_bg watch > rewatch.log 2>&1 & if ! wait_for_file "./src/Test.mjs" 20 || ! wait_for_file "lib/watch.lock" 20; then error "Initial watch build did not complete" cat rewatch.log - exit_watcher + stop_watcher mv rescript.json.watch-recovery.bak rescript.json exit 1 fi @@ -42,7 +29,7 @@ fi if ! wait_for_pattern rewatch.log "Finished initial compilation" 60; then error "Initial watch build did not finish" cat rewatch.log - exit_watcher + stop_watcher mv rescript.json.watch-recovery.bak rescript.json exit 1 fi @@ -54,8 +41,27 @@ mv rescript.json.watch-recovery.next rescript.json if ! wait_for_pattern rewatch.log "Could not initialize build" 30; then error "Watcher did not report the invalid config" cat rewatch.log - exit_watcher + stop_watcher + mv rescript.json.watch-recovery.bak rescript.json + exit 1 +fi + +# A content-only edit must retry initialization instead of compiling with the old config. +initial_errors=$(grep -c "Could not initialize build" rewatch.log 2> /dev/null || true) +echo 'let watchRecoveryMarker = "watch-recovery-marker"' >> src/Test.res +if ! wait_for_pattern_count rewatch.log "Could not initialize build" "$((initial_errors + 1))" 30; then + error "Source edit did not retry failed initialization" + cat rewatch.log + stop_watcher + mv rescript.json.watch-recovery.bak rescript.json + git restore --worktree -- src/Test.res + exit 1 +fi +if grep -q "watch-recovery-marker" src/Test.mjs 2> /dev/null; then + error "Watcher compiled the source using stale configuration" + stop_watcher mv rescript.json.watch-recovery.bak rescript.json + git restore --worktree -- src/Test.res exit 1 fi @@ -71,27 +77,33 @@ expected_completed=$((initial_completed + 1)) if ! wait_for_pattern_count rewatch.log "Finished compilation" "$expected_completed" 30; then error "Watcher did not recover after restoring a valid config" cat rewatch.log - exit_watcher + stop_watcher mv rescript.json.watch-recovery.bak rescript.json + git restore --worktree -- src/Test.res exit 1 fi -echo '// watch-recovery-test' >> src/Test.res -if ! wait_for_file "./src/Test.recovered.mjs" 20; then +if ! wait_for_pattern "./src/Test.recovered.mjs" "watch-recovery-marker" 20; then error "Recovered watcher did not compile with the restored config" cat rewatch.log - exit_watcher + stop_watcher mv rescript.json.watch-recovery.bak rescript.json - git checkout -- src/Test.res + git restore --worktree -- src/Test.res exit 1 fi -exit_watcher -rewatch clean > /dev/null 2>&1 +stop_watcher +if ! rewatch clean > /dev/null 2>&1; then + error "Could not clean after watcher recovery test" + exit 1 +fi mv rescript.json.watch-recovery.bak rescript.json -git checkout -- src/Test.res +git restore --worktree -- src/Test.res rm -f rewatch.log -rewatch build > /dev/null 2>&1 +if ! rewatch build > /dev/null 2>&1; then + error "Could not rebuild after watcher recovery test" + exit 1 +fi if git diff --exit-code . > /dev/null 2>&1 && [ -z "$(git ls-files --others --exclude-standard .)" ]; then success "Watcher recovered from invalid config" diff --git a/rewatch/tests/watch/08-watch-recovers-new-dependency-config.sh b/rewatch/tests/watch/08-watch-recovers-new-dependency-config.sh new file mode 100755 index 00000000000..84532e28efe --- /dev/null +++ b/rewatch/tests/watch/08-watch-recovers-new-dependency-config.sh @@ -0,0 +1,83 @@ +#!/bin/bash +cd $(dirname $0) +source "../utils.sh" +cd ../../testrepo + +bold "Test: Watcher recovers when only a newly added dependency config is fixed" + +dependency_config=packages/standalone/rescript.json + +stop_and_restore() { + if ! exit_watcher; then + return 1 + fi + mv rescript.json.watch-dependency.bak rescript.json + mv "$dependency_config.watch-dependency.bak" "$dependency_config" + rm -f rewatch.log + if ! rewatch build > /dev/null 2>&1; then + error "Could not rebuild testrepo after dependency recovery test" + return 1 + fi + # The root no longer depends on standalone; restore its artifacts with its own build. + if ! (cd packages/standalone && rewatch build > /dev/null 2>&1); then + error "Could not rebuild standalone after dependency recovery test" + return 1 + fi +} + +fail_test() { + error "$1" + cat rewatch.log + if ! stop_and_restore; then + exit 1 + fi + exit 1 +} + +if ! rewatch clean > /dev/null 2>&1; then + error "Could not clean before dependency recovery test" + exit 1 +fi +cp rescript.json rescript.json.watch-dependency.bak +cp "$dependency_config" "$dependency_config.watch-dependency.bak" +rewatch_bg watch > rewatch.log 2>&1 & + +if ! wait_for_file "lib/watch.lock" 20 || ! wait_for_pattern rewatch.log "Finished initial compilation" 60; then + fail_test "Initial watch build did not finish" +fi +initial_completed=$(grep -c "Finished compilation" rewatch.log 2> /dev/null || true) + +# Standalone is installed but absent from the root's original ReScript dependency graph. +printf '{"name":' > "$dependency_config.watch-dependency.next" +mv "$dependency_config.watch-dependency.next" "$dependency_config" +node -e ' + const fs = require("fs"); + const config = JSON.parse(fs.readFileSync(process.argv[1], "utf8")); + config.dependencies.push("@testrepo/standalone"); + fs.writeFileSync(process.argv[2], `${JSON.stringify(config, null, 2)}\n`); +' rescript.json rescript.json.watch-dependency.next +mv rescript.json.watch-dependency.next rescript.json + +if ! wait_for_pattern rewatch.log "Could not read dependency config.*standalone" 30; then + fail_test "Watcher did not report the newly added dependency's invalid config" +fi + +# Only fix the new dependency. Its directory must have a recovery watch by this point. +cp "$dependency_config.watch-dependency.bak" "$dependency_config.watch-dependency.next" +mv "$dependency_config.watch-dependency.next" "$dependency_config" +if ! wait_for_pattern_count rewatch.log "Finished compilation" "$((initial_completed + 1))" 30; then + fail_test "Watcher did not recover after fixing only the new dependency's config" +fi + +if ! stop_and_restore; then + exit 1 +fi + +if git diff --exit-code . > /dev/null 2>&1 && [ -z "$(git ls-files --others --exclude-standard .)" ]; then + success "Watcher recovered after the new dependency config was fixed" +else + error "Dependency recovery test left repository changes" + git diff . + git ls-files --others --exclude-standard . + exit 1 +fi From b7ff3d53c2fdae9b948e867a416dfb1333126879 Mon Sep 17 00:00:00 2001 From: Florian Hammerschmidt Date: Mon, 12 Oct 2026 22:53:10 +0200 Subject: [PATCH 3/8] Restore dependency resolution error context Signed-off-by: Florian Hammerschmidt --- rewatch/src/build/packages.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/rewatch/src/build/packages.rs b/rewatch/src/build/packages.rs index c544d738de0..4e266a3ccf4 100644 --- a/rewatch/src/build/packages.rs +++ b/rewatch/src/build/packages.rs @@ -371,8 +371,8 @@ fn read_dependencies( // Read all config files in parallel instead of blocking .par_iter() .map(|package_name| -> Result { - let canonical_path = - read_dependency(package_name, package_config, project_context).inspect_err(|error| { + let canonical_path = read_dependency(package_name, package_config, project_context) + .inspect_err(|error| { if show_progress { println!( "{} {} Error building package tree. {}", @@ -381,6 +381,9 @@ fn read_dependencies( error ); } + }) + .with_context(|| { + format!("Could not build package tree reading dependency '{package_name}'") })?; let config = read_config(&canonical_path).map_err(|source| DependencyConfigError { package_path: canonical_path.clone(), From ff35b9a9045622b8f38f8a44ec119dcd9bd7e6a0 Mon Sep 17 00:00:00 2001 From: Florian Hammerschmidt Date: Tue, 13 Oct 2026 14:13:35 +0200 Subject: [PATCH 4/8] Address watch recovery documentation and output nits Signed-off-by: Florian Hammerschmidt --- changes/watch-rebuild-recovery.fix.md | 1 + rewatch/src/build.rs | 2 +- rewatch/src/build/clean.rs | 2 +- rewatch/src/build/packages.rs | 23 ++----------------- rewatch/src/format.rs | 2 +- rewatch/src/watcher.rs | 4 ++-- rewatch/tests/suite.sh | 4 ++-- ... 08-watch-recovers-from-invalid-config.sh} | 0 ...9-watch-recovers-new-dependency-config.sh} | 0 tests/build_tests/weird_deps/input.js | 1 + tests/build_tests/weird_devdeps/input.js | 1 + 11 files changed, 12 insertions(+), 28 deletions(-) rename rewatch/tests/watch/{07-watch-recovers-from-invalid-config.sh => 08-watch-recovers-from-invalid-config.sh} (100%) rename rewatch/tests/watch/{08-watch-recovers-new-dependency-config.sh => 09-watch-recovers-new-dependency-config.sh} (100%) diff --git a/changes/watch-rebuild-recovery.fix.md b/changes/watch-rebuild-recovery.fix.md index 89152cd15cb..9ea4c0feded 100644 --- a/changes/watch-rebuild-recovery.fix.md +++ b/changes/watch-rebuild-recovery.fix.md @@ -1 +1,2 @@ - Keep watch mode running after full rebuild initialization fails, retry initialization on source edits, and watch newly added dependencies for configuration fixes. https://github.com/rescript-lang/rescript/pull/8641 +- Dependency resolution and configuration errors now return exit code `1` instead of `2` from `rescript build`, `clean`, and `format`. https://github.com/rescript-lang/rescript/pull/8641 diff --git a/rewatch/src/build.rs b/rewatch/src/build.rs index 1db5dbc218a..4dcc23bc0bb 100644 --- a/rewatch/src/build.rs +++ b/rewatch/src/build.rs @@ -186,7 +186,7 @@ pub fn initialize_build( let compiler = get_compiler_info(&project_context)?; let timing_clean_start = Instant::now(); - let packages = packages::make(filter, &project_context, show_progress, prod, features.as_ref())?; + let packages = packages::make(filter, &project_context, prod, features.as_ref())?; let source_map_args = project_context .get_root_config() diff --git a/rewatch/src/build/clean.rs b/rewatch/src/build/clean.rs index 9661f2911d2..67591925394 100644 --- a/rewatch/src/build/clean.rs +++ b/rewatch/src/build/clean.rs @@ -346,7 +346,7 @@ pub fn clean(path: &Path, show_progress: bool, plain_output: bool, prod: bool) - // `clean` always acts on the full set of source directories regardless of which features are // active. We explicitly pass `None` so every tagged source folder is included and its // artifacts can be removed, even for features the user hasn't enabled for this build. - let packages = packages::make(&None, &project_context, show_progress, prod, None)?; + let packages = packages::make(&None, &project_context, prod, None)?; let timing_clean_compiler_assets = Instant::now(); if !plain_output && show_progress { diff --git a/rewatch/src/build/packages.rs b/rewatch/src/build/packages.rs index 4e266a3ccf4..7f71c145c58 100644 --- a/rewatch/src/build/packages.rs +++ b/rewatch/src/build/packages.rs @@ -9,7 +9,6 @@ use crate::helpers::emojis::*; use crate::project_context::{MonoRepoContext, ProjectContext}; use ahash::{AHashMap, AHashSet}; use anyhow::{Context, Result, anyhow}; -use console::style; use log::debug; use rayon::prelude::*; use std::collections::hash_map::Entry; @@ -318,7 +317,6 @@ fn read_dependencies( registered_dependencies_set: &mut AHashSet, project_context: &ProjectContext, package_config: &Config, - show_progress: bool, is_local_dep: bool, prod: bool, ) -> Result> { @@ -372,16 +370,6 @@ fn read_dependencies( .par_iter() .map(|package_name| -> Result { let canonical_path = read_dependency(package_name, package_config, project_context) - .inspect_err(|error| { - if show_progress { - println!( - "{} {} Error building package tree. {}", - style("[1/2]").bold().dim(), - CROSS, - error - ); - } - }) .with_context(|| { format!("Could not build package tree reading dependency '{package_name}'") })?; @@ -396,7 +384,6 @@ fn read_dependencies( &mut registered_dependencies_set.to_owned(), project_context, &config, - show_progress, is_local_dep, prod, ) @@ -567,11 +554,7 @@ This inconsistency will cause issues with package resolution.\n", }) } -fn read_packages( - project_context: &ProjectContext, - show_progress: bool, - prod: bool, -) -> Result> { +fn read_packages(project_context: &ProjectContext, prod: bool) -> Result> { // Store all packages and completely deduplicate them let mut map: AHashMap = AHashMap::new(); @@ -591,7 +574,6 @@ fn read_packages( &mut registered_dependencies_set, project_context, &project_context.current_config, - show_progress, /* is local dep */ true, prod, )?); @@ -854,11 +836,10 @@ fn compute_active_features( pub fn make( filter: &Option, project_context: &ProjectContext, - show_progress: bool, prod: bool, cli_features: Option<&Vec>, ) -> Result> { - let mut map = read_packages(project_context, show_progress, prod)?; + let mut map = read_packages(project_context, prod)?; let active_features = compute_active_features(&map, cli_features, prod)?; diff --git a/rewatch/src/format.rs b/rewatch/src/format.rs index eed70f0812f..ff0b897fba0 100644 --- a/rewatch/src/format.rs +++ b/rewatch/src/format.rs @@ -39,7 +39,7 @@ fn get_files_in_scope() -> Result> { let project_context = project_context::ProjectContext::new(¤t_dir)?; // Format walks all source files regardless of feature selection. - let packages = packages::make(&None, &project_context, false, false, None)?; + let packages = packages::make(&None, &project_context, false, None)?; let mut files: Vec = Vec::new(); let packages_to_format = project_context.get_scoped_local_packages(); diff --git a/rewatch/src/watcher.rs b/rewatch/src/watcher.rs index d7e2ec3dc23..00d5d34cefd 100644 --- a/rewatch/src/watcher.rs +++ b/rewatch/src/watcher.rs @@ -969,7 +969,7 @@ mod tests { std::fs::write(dependency.join("rescript.json"), r#"{"name":"#).unwrap(); let context = ProjectContext::new(&root).unwrap(); - let error = build::packages::make(&None, &context, false, false, None) + let error = build::packages::make(&None, &context, false, None) .unwrap_err() .context("Could not initialize build"); assert!(format!("{error:#}").contains("Failed to parse rescript.json")); @@ -995,7 +995,7 @@ mod tests { assert!(paths.contains(&(dependency.clone(), RecursiveMode::Recursive))); std::fs::write(dependency.join("rescript.json"), r#"{"name":"new-dependency"}"#).unwrap(); - let recovered = build::packages::make(&None, &context, false, false, None).unwrap(); + let recovered = build::packages::make(&None, &context, false, None).unwrap(); assert!(recovered.contains_key("new-dependency")); } diff --git a/rewatch/tests/suite.sh b/rewatch/tests/suite.sh index 245c9dbafc9..43467d69315 100755 --- a/rewatch/tests/suite.sh +++ b/rewatch/tests/suite.sh @@ -145,8 +145,8 @@ fi ./watch/05-watch-ignores-non-source.sh && ./watch/06-watch-missing-source-folder.sh && ./watch/07-changed-interface-after-failed-implementation.sh && -./watch/07-watch-recovers-from-invalid-config.sh && -./watch/08-watch-recovers-new-dependency-config.sh && +./watch/08-watch-recovers-from-invalid-config.sh && +./watch/09-watch-recovers-new-dependency-config.sh && # Lock tests ./lock/01-lock-when-watching.sh && diff --git a/rewatch/tests/watch/07-watch-recovers-from-invalid-config.sh b/rewatch/tests/watch/08-watch-recovers-from-invalid-config.sh similarity index 100% rename from rewatch/tests/watch/07-watch-recovers-from-invalid-config.sh rename to rewatch/tests/watch/08-watch-recovers-from-invalid-config.sh diff --git a/rewatch/tests/watch/08-watch-recovers-new-dependency-config.sh b/rewatch/tests/watch/09-watch-recovers-new-dependency-config.sh similarity index 100% rename from rewatch/tests/watch/08-watch-recovers-new-dependency-config.sh rename to rewatch/tests/watch/09-watch-recovers-new-dependency-config.sh diff --git a/tests/build_tests/weird_deps/input.js b/tests/build_tests/weird_deps/input.js index d98217346bb..145846d0a21 100644 --- a/tests/build_tests/weird_deps/input.js +++ b/tests/build_tests/weird_deps/input.js @@ -9,4 +9,5 @@ const out = await execBuild(); assert.notEqual(out.status, 0); assert.match(out.stderr, /could not build package tree/i); assert.match(out.stderr, /dependency 'weird'/i); +assert.doesNotMatch(out.stdout, /error building package tree/i); await execClean(); diff --git a/tests/build_tests/weird_devdeps/input.js b/tests/build_tests/weird_devdeps/input.js index d98217346bb..145846d0a21 100644 --- a/tests/build_tests/weird_devdeps/input.js +++ b/tests/build_tests/weird_devdeps/input.js @@ -9,4 +9,5 @@ const out = await execBuild(); assert.notEqual(out.status, 0); assert.match(out.stderr, /could not build package tree/i); assert.match(out.stderr, /dependency 'weird'/i); +assert.doesNotMatch(out.stdout, /error building package tree/i); await execClean(); From fef2886a0a06f3dc3f773960b39f4906919d273d Mon Sep 17 00:00:00 2001 From: Florian Hammerschmidt Date: Wed, 14 Oct 2026 21:54:11 +0200 Subject: [PATCH 5/8] Tie watcher recovery regression to the source edit Log the source event path and its requested compile type once, then wait for the edited source to request full initialization and for an error after that event. Delayed config failures can no longer satisfy the recovery assertion before the edit is processed. Signed-off-by: Florian Hammerschmidt --- rewatch/src/watcher.rs | 18 ++++++------------ .../08-watch-recovers-from-invalid-config.sh | 18 +++++++++++++++--- 2 files changed, 21 insertions(+), 15 deletions(-) diff --git a/rewatch/src/watcher.rs b/rewatch/src/watcher.rs index 00d5d34cefd..602d648e05b 100644 --- a/rewatch/src/watcher.rs +++ b/rewatch/src/watcher.rs @@ -462,11 +462,13 @@ async fn async_watch( .filter(|path| matches_filter(path, filter)); for path in paths { let path_buf = path.to_path_buf(); + let source_compile_type = compile_type_for_source_event(event.kind, initialization_failed); + log::debug!( + "received {:?} for {path:?} -> {source_compile_type:?} (pending: {needs_compile_type:?})", + event.kind + ); - match ( - needs_compile_type, - compile_type_for_source_event(event.kind, initialization_failed), - ) { + match (needs_compile_type, source_compile_type) { ( CompileType::Incremental | CompileType::None, // Structural changes and edits after failed initialization need a full scan. @@ -474,10 +476,6 @@ async fn async_watch( ) => { // if we are going to do a full compile, we don't need to bother marking // files dirty because we do a full scan anyway - log::debug!( - "received {:?} while needs_compile_type was {needs_compile_type:?} -> full compile", - event.kind - ); needs_compile_type = CompileType::Full; } @@ -488,10 +486,6 @@ async fn async_watch( ) => { // if we are going to compile incrementally, we need to mark the exact files // dirty - log::debug!( - "received {:?} while needs_compile_type was {needs_compile_type:?} -> incremental compile", - event.kind - ); if let Ok(canonicalized_path_buf) = path_buf .canonicalize() .map(StrippedVerbatimPath::to_stripped_verbatim_path) diff --git a/rewatch/tests/watch/08-watch-recovers-from-invalid-config.sh b/rewatch/tests/watch/08-watch-recovers-from-invalid-config.sh index 2fd76136830..94ce1d6ac92 100755 --- a/rewatch/tests/watch/08-watch-recovers-from-invalid-config.sh +++ b/rewatch/tests/watch/08-watch-recovers-from-invalid-config.sh @@ -11,12 +11,25 @@ stop_watcher() { fi } +wait_for_source_retry() { + local marker='received Modify.*Test[.]res.* -> Full' + wait_for_pattern rewatch.log "$marker" 30 || return 1 + # Require an initialization failure after the source event, not another config event before it. + local preceding_errors + preceding_errors=$(awk -v marker="$marker" ' + $0 ~ marker {exit} + /Could not initialize build/ {count++} + END {print count + 0} + ' rewatch.log) + wait_for_pattern_count rewatch.log "Could not initialize build" "$((preceding_errors + 1))" 30 +} + if ! rewatch clean > /dev/null 2>&1; then error "Could not clean before watcher recovery test" exit 1 fi cp rescript.json rescript.json.watch-recovery.bak -rewatch_bg watch > rewatch.log 2>&1 & +rewatch_bg -vv watch > rewatch.log 2>&1 & if ! wait_for_file "./src/Test.mjs" 20 || ! wait_for_file "lib/watch.lock" 20; then error "Initial watch build did not complete" @@ -47,9 +60,8 @@ if ! wait_for_pattern rewatch.log "Could not initialize build" 30; then fi # A content-only edit must retry initialization instead of compiling with the old config. -initial_errors=$(grep -c "Could not initialize build" rewatch.log 2> /dev/null || true) echo 'let watchRecoveryMarker = "watch-recovery-marker"' >> src/Test.res -if ! wait_for_pattern_count rewatch.log "Could not initialize build" "$((initial_errors + 1))" 30; then +if ! wait_for_source_retry; then error "Source edit did not retry failed initialization" cat rewatch.log stop_watcher From 300110cb9b7d82b1a8185c267817943c1b6fa06f Mon Sep 17 00:00:00 2001 From: Florian Hammerschmidt Date: Tue, 20 Oct 2026 15:55:26 +0200 Subject: [PATCH 6/8] Recover from invalid JSX and preserve dependency error behavior Reject unsupported JSX versions while reading configuration, and release the build lock through a Drop guard on every return or unwind. Retain dependency failures as typed error context so the CLI preserves exit code 2, package and root-path diagnostics, and the package-tree progress message. Review: Both recovery tests use temporary projects. The invalid-config test records every emitted output across invalid JSON and JSX configurations and recovery, with the same assertions for Rust and OCaml. Disabling Rust's source retry makes it detect stale output without depending on debug-log wording. Validation: Native compiler/CLI and Rust builds, 150 Rust unit tests, all 42 build fixtures, 297 configuration parity cases, 28 focused dependency-error checks, Clippy with warnings denied, formatting, changelog, and shell syntax checks pass. Both recovery scripts pass for both CLIs using isolated polling builds; native filesystem events are unavailable locally and remain for CI. Area: rewatch tests docs Output: unchanged; no generated JavaScript changes Tests: ~rewatch/src/build.rs ~rewatch/src/config.rs ~rewatch/tests/watch/09-watch-recovers-from-invalid-config.sh ~rewatch/tests/watch/10-watch-recovers-new-dependency-config.sh ~rewatch-ocaml/tests/check_command_validation.sh ~rewatch-ocaml/tests/config_acceptance_cases.tsv ~tests/build_tests/weird_deps/input.js ~tests/build_tests/weird_devdeps/input.js PR: #8641 Signed-off-by: Florian Hammerschmidt --- changes/watch-rebuild-recovery.fix.md | 1 - rewatch-ocaml/README.md | 2 - .../tests/check_command_validation.sh | 57 +++--- .../tests/config_acceptance_cases.tsv | 2 +- rewatch/src/build.rs | 28 ++- rewatch/src/build/clean.rs | 2 +- rewatch/src/build/packages.rs | 54 +++-- rewatch/src/config.rs | 17 +- rewatch/src/format.rs | 2 +- rewatch/src/main.rs | 6 +- rewatch/src/watcher.rs | 11 +- .../09-watch-recovers-from-invalid-config.sh | 193 +++++++----------- ...10-watch-recovers-new-dependency-config.sh | 117 +++++------ tests/build_tests/weird_deps/input.js | 4 +- tests/build_tests/weird_devdeps/input.js | 4 +- 15 files changed, 253 insertions(+), 247 deletions(-) diff --git a/changes/watch-rebuild-recovery.fix.md b/changes/watch-rebuild-recovery.fix.md index 9ea4c0feded..89152cd15cb 100644 --- a/changes/watch-rebuild-recovery.fix.md +++ b/changes/watch-rebuild-recovery.fix.md @@ -1,2 +1 @@ - Keep watch mode running after full rebuild initialization fails, retry initialization on source edits, and watch newly added dependencies for configuration fixes. https://github.com/rescript-lang/rescript/pull/8641 -- Dependency resolution and configuration errors now return exit code `1` instead of `2` from `rescript build`, `clean`, and `format`. https://github.com/rescript-lang/rescript/pull/8641 diff --git a/rewatch-ocaml/README.md b/rewatch-ocaml/README.md index 044fad6f459..481b0fe8f61 100644 --- a/rewatch-ocaml/README.md +++ b/rewatch-ocaml/README.md @@ -105,8 +105,6 @@ These are intentional: own process group. - `format -v` and `compiler-args -v` write their logs to stderr, so stdout remains formatted source or JSON. Rust writes them to stdout. -- Dependency resolution failures and invalid dependency configurations from - `build`, `clean`, and `format` return exit code `2` here and `1` in Rust. - While the root configuration is invalid, only configuration changes trigger recovery attempts here. Rust also retries initialization on watched source edits. - Watch mode also notices a missing dependency being installed into an diff --git a/rewatch-ocaml/tests/check_command_validation.sh b/rewatch-ocaml/tests/check_command_validation.sh index 0d7e1b745aa..92933718408 100755 --- a/rewatch-ocaml/tests/check_command_validation.sh +++ b/rewatch-ocaml/tests/check_command_validation.sh @@ -518,19 +518,19 @@ require_same_output() { require_both_errors_contain() { local name=$1 - local implementation fragment native_fragment native_short_fragment - for implementation in rust ocaml; do - if [ "$implementation" = rust ]; then fragment=$2; else fragment=${3:-$2}; fi - native_fragment=${fragment/"$work"/"$native_work"} - native_short_fragment=${fragment/"$work"/"$native_short_work"} - if ! { grep -F "$fragment" "$work/$implementation.err" >/dev/null || \ - tr '\\' '/' <"$work/$implementation.err" | grep -F "$native_fragment" >/dev/null || \ - tr '\\' '/' <"$work/$implementation.err" | grep -F "$native_short_fragment" >/dev/null; }; then - printf '%s: expected %s error to contain %s\n' "$name" "$implementation" "$fragment" >&2 - dump_both - exit 1 - fi - done + local fragment=$2 + local native_fragment=${fragment/"$work"/"$native_work"} + local native_short_fragment=${fragment/"$work"/"$native_short_work"} + if ! { grep -F "$fragment" "$work/rust.err" >/dev/null || \ + tr '\\' '/' <"$work/rust.err" | grep -F "$native_fragment" >/dev/null || \ + tr '\\' '/' <"$work/rust.err" | grep -F "$native_short_fragment" >/dev/null; } || \ + ! { grep -F "$fragment" "$work/ocaml.err" >/dev/null || \ + tr '\\' '/' <"$work/ocaml.err" | grep -F "$native_fragment" >/dev/null || \ + tr '\\' '/' <"$work/ocaml.err" | grep -F "$native_short_fragment" >/dev/null; }; then + printf '%s: expected both errors to contain %s\n' "$name" "$fragment" >&2 + dump_both + exit 1 + fi } # Rust needs RESCRIPT_BSC_EXE to exist; OCaml builds and formats with the @@ -1190,16 +1190,19 @@ if ! grep -F \ cat "$work/ocaml.out" "$work/ocaml.err" >&2 exit 1 fi -run_case build-missing-dependency reject exit2 build "$work/missing-dependency" +run_case build-missing-dependency exit2 exit2 build "$work/missing-dependency" require_both_errors_contain build-missing-dependency \ - "Could not build package tree reading dependency 'absent'" -run_case build-configless-dependency reject exit2 build "$work/configless-dependency" + "Could not build package tree reading dependency 'absent' at path '$work/missing-dependency'. Error:" +if ! grep -F 'Error building package tree.' "$work/rust.out" >/dev/null; then + echo "Missing dependency did not report package-tree progress" >&2 + dump_both + exit 1 +fi +run_case build-configless-dependency exit2 exit2 build "$work/configless-dependency" require_both_errors_contain build-configless-dependency \ - "Could not read dependency config at $work/configless-dependency/node_modules/no-config" \ "Could not build package tree for 'no-config' at path '$work/configless-dependency'. Error:" -run_case build-malformed-dependency reject exit2 build "$work/malformed-dependency" +run_case build-malformed-dependency exit2 exit2 build "$work/malformed-dependency" require_both_errors_contain build-malformed-dependency \ - "Could not read dependency config at $work/malformed-dependency/node_modules/bad-config" \ "Could not build package tree for 'bad-config' at path '$work/malformed-dependency'. Error:" run_case build-duplicate-dependency accept accept build "$work/duplicate-dependency" duplicate_warning='Duplicated package: shared ./node_modules/shared (chosen) vs ./node_modules/a/node_modules/shared in ./node_modules/a' @@ -1522,9 +1525,9 @@ if ! cmp -s "$work/quiet-watch-rust.err.norm" \ fi checked=$((checked + 1)) -run_case clean-missing-dependency reject exit2 clean "$work/missing-dependency" -run_case clean-configless-dependency reject exit2 clean "$work/configless-dependency" -run_case clean-malformed-dependency reject exit2 clean "$work/malformed-dependency" +run_case clean-missing-dependency exit2 exit2 clean "$work/missing-dependency" +run_case clean-configless-dependency exit2 exit2 clean "$work/configless-dependency" +run_case clean-malformed-dependency exit2 exit2 clean "$work/malformed-dependency" run_both clean "$work/clean-duplicate-{impl}" if [ "$(classify "$rust_status")" != reject ] || [ "$ocaml_status" -ne 0 ] || \ [ ! -e "$work/clean-duplicate-rust/src/one/A.js" ] || \ @@ -1542,8 +1545,8 @@ set +e "$rust" watch "$work/missing-dependency" >"$work/rust.out" 2>"$work/rust.err" rust_status=$? set -e -if [ "$rust_status" -ne 1 ]; then - echo "watch-missing-dependency: expected Rust exit 1, got $rust_status" >&2 +if [ "$rust_status" -ne 2 ]; then + echo "watch-missing-dependency: expected Rust exit 2, got $rust_status" >&2 cat "$work/rust.out" "$work/rust.err" >&2 exit 1 fi @@ -1574,11 +1577,11 @@ printf '{"name":"missing-dependency","sources":["src"],"dependencies":["absent"] >"$work/missing-dependency/rescript.json" checked=$((checked + 1)) run_case --cwd "$work/missing-dependency" \ - format-missing-dependency reject exit2 format + format-missing-dependency exit2 exit2 format run_case --cwd "$work/configless-dependency" \ - format-configless-dependency reject exit2 format + format-configless-dependency exit2 exit2 format run_case --cwd "$work/malformed-dependency" \ - format-malformed-dependency reject exit2 format + format-malformed-dependency exit2 exit2 format if [ -e "$work/missing-dependency/lib/build.lock" ] || \ [ -e "$work/missing-dependency/lib/watch.lock" ]; then echo "OCaml dependency failures left a build or watch lock behind" >&2 diff --git a/rewatch-ocaml/tests/config_acceptance_cases.tsv b/rewatch-ocaml/tests/config_acceptance_cases.tsv index f9753331bf3..d9c4420dd52 100644 --- a/rewatch-ocaml/tests/config_acceptance_cases.tsv +++ b/rewatch-ocaml/tests/config_acceptance_cases.tsv @@ -124,7 +124,7 @@ jsx preserve-false accept {"name":"config-shape","jsx":{"preserve":false}} jsx preserve-true accept {"name":"config-shape","jsx":{"preserve":true}} jsx unknown-field accept {"name":"config-shape","jsx":{"future":true}} jsx boolean-outer reject {"name":"config-shape","jsx":true} -jsx version-three panic/reject {"name":"config-shape","jsx":{"version":3}} +jsx version-three reject {"name":"config-shape","jsx":{"version":3}} jsx string-version reject {"name":"config-shape","jsx":{"version":"4"}} jsx boolean-module reject {"name":"config-shape","jsx":{"module":true}} jsx unknown-mode accept {"name":"config-shape","jsx":{"mode":"modern"}} diff --git a/rewatch/src/build.rs b/rewatch/src/build.rs index d01073b01e5..733d66bccf7 100644 --- a/rewatch/src/build.rs +++ b/rewatch/src/build.rs @@ -189,7 +189,7 @@ pub fn initialize_build( let compiler = get_compiler_info(&project_context)?; let timing_clean_start = Instant::now(); - let packages = packages::make(filter, &project_context, prod, features.as_ref())?; + let packages = packages::make(filter, &project_context, show_progress, prod, features.as_ref())?; let source_map_args = project_context .get_root_config() @@ -292,11 +292,17 @@ impl fmt::Display for IncrementalBuildError { } pub fn with_build_lock(path: &Path, f: impl FnOnce() -> T) -> T { + struct BuildLock<'a>(&'a str); + impl Drop for BuildLock<'_> { + fn drop(&mut self) { + let _ = drop_lock(LockKind::Build, self.0); + } + } + let build_folder = path.to_string_lossy().to_string(); - let _lock = get_lock_or_exit(LockKind::Build, &build_folder); - let result = f(); - let _lock = drop_lock(LockKind::Build, &build_folder); - result + get_lock_or_exit(LockKind::Build, &build_folder); + let _lock = BuildLock(&build_folder); + f() } #[instrument(name = "build.incremental", skip_all, fields(module_count = build_state.modules.len()))] @@ -733,6 +739,18 @@ mod tests { ); } + #[test] + fn with_build_lock_drops_lock_after_panic() { + let temp_dir = TempDir::new().unwrap(); + let project_folder = temp_dir.path(); + let result = std::panic::catch_unwind(|| { + with_build_lock(project_folder, || panic!("failed")); + }); + + assert!(result.is_err()); + assert!(!project_folder.join("lib/build.lock").exists()); + } + #[test] fn build_waits_for_lock_before_initializing() { let temp_dir = TempDir::new().expect("temp dir should be created"); diff --git a/rewatch/src/build/clean.rs b/rewatch/src/build/clean.rs index 0fd7bb1a3f3..4fdc3e1faaa 100644 --- a/rewatch/src/build/clean.rs +++ b/rewatch/src/build/clean.rs @@ -346,7 +346,7 @@ pub fn clean(path: &Path, show_progress: bool, plain_output: bool, prod: bool) - // `clean` always acts on the full set of source directories regardless of which features are // active. We explicitly pass `None` so every tagged source folder is included and its // artifacts can be removed, even for features the user hasn't enabled for this build. - let packages = packages::make(&None, &project_context, prod, None)?; + let packages = packages::make(&None, &project_context, show_progress, prod, None)?; let timing_clean_compiler_assets = Instant::now(); if !plain_output && show_progress { diff --git a/rewatch/src/build/packages.rs b/rewatch/src/build/packages.rs index a9520df6194..73c97302ac2 100644 --- a/rewatch/src/build/packages.rs +++ b/rewatch/src/build/packages.rs @@ -282,30 +282,30 @@ pub fn read_dependency( Ok(canonical_path) } -/// Retain the resolved directory even when its config cannot be read, so watch mode can -/// observe a correction to a dependency that was absent from the previous build state. +/// Retain dependency context for CLI exit codes and recovery watches after config errors. #[derive(Debug)] -pub struct DependencyConfigError { - pub package_path: PathBuf, - source: anyhow::Error, +pub struct DependencyError { + pub package_path: Option, + package_name: String, + root_path: PathBuf, } -impl fmt::Display for DependencyConfigError { +impl fmt::Display for DependencyError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!( f, - "Could not read dependency config at {}", - self.package_path.display() + "Could not build package tree {} '{}' at path '{}'. Error", + if self.package_path.is_some() { + "for" + } else { + "reading dependency" + }, + self.package_name, + self.root_path.display() ) } } -impl error::Error for DependencyConfigError { - fn source(&self) -> Option<&(dyn error::Error + 'static)> { - Some(self.source.as_ref()) - } -} - /// Given a config, recursively finds all dependencies. /// 1. It starts with registering dependencies and /// prevents the operation for the ones which are already @@ -369,14 +369,15 @@ fn read_dependencies( // Read all config files in parallel instead of blocking .par_iter() .map(|package_name| -> Result { + let dependency_error = |package_path| DependencyError { + package_path, + package_name: package_name.clone(), + root_path: project_context.get_root_path().to_path_buf(), + }; let canonical_path = read_dependency(package_name, package_config, project_context) - .with_context(|| { - format!("Could not build package tree reading dependency '{package_name}'") - })?; - let config = read_config(&canonical_path).map_err(|source| DependencyConfigError { - package_path: canonical_path.clone(), - source, - })?; + .with_context(|| dependency_error(None))?; + let config = read_config(&canonical_path) + .with_context(|| dependency_error(Some(canonical_path.clone())))?; let is_local_dep = is_local_dependency(project_context, package_name, &canonical_path); @@ -834,10 +835,19 @@ fn compute_active_features( pub fn make( filter: &Option, project_context: &ProjectContext, + show_progress: bool, prod: bool, cli_features: Option<&Vec>, ) -> Result> { - let mut map = read_packages(project_context, prod)?; + let mut map = read_packages(project_context, prod).inspect_err(|_| { + if show_progress { + println!( + "{} {} Error building package tree.", + console::style("[1/2]").bold().dim(), + CROSS + ); + } + })?; let active_features = compute_active_features(&map, cli_features, prod)?; diff --git a/rewatch/src/config.rs b/rewatch/src/config.rs index 43147671635..d677c2a92d9 100644 --- a/rewatch/src/config.rs +++ b/rewatch/src/config.rs @@ -778,6 +778,14 @@ impl Config { config.handle_deprecations()?; config.unknown_fields = unknown_fields; + if let Some(version) = config.jsx.as_ref().and_then(|jsx| jsx.version) + && version != 4 + { + return Err(anyhow!( + "JSX version {version} is unsupported; jsx.version must be 4" + )); + } + Ok(config) } @@ -828,10 +836,9 @@ impl Config { pub fn get_jsx_args(&self) -> Vec { match self.jsx.to_owned() { Some(jsx) => match jsx.version { - Some(version) if version == 4 => { + Some(version) => { vec!["-bs-jsx".to_string(), version.to_string()] } - Some(version) => panic!("JSX version {version} is unsupported"), None => vec![], }, None => vec![], @@ -1635,6 +1642,12 @@ pub mod tests { assert!(config.get_gentype_args(&[], None, &[]).is_empty()); } + #[test] + fn test_unsupported_jsx_version_is_a_config_error() { + let error = Config::new_from_json_string(r#"{"name":"test","jsx":{"version":3}}"#).unwrap_err(); + assert!(error.to_string().contains("JSX version 3 is unsupported")); + } + #[test] fn test_other_jsx_module() { let json = r#" diff --git a/rewatch/src/format.rs b/rewatch/src/format.rs index ff0b897fba0..eed70f0812f 100644 --- a/rewatch/src/format.rs +++ b/rewatch/src/format.rs @@ -39,7 +39,7 @@ fn get_files_in_scope() -> Result> { let project_context = project_context::ProjectContext::new(¤t_dir)?; // Format walks all source files regardless of feature selection. - let packages = packages::make(&None, &project_context, false, None)?; + let packages = packages::make(&None, &project_context, false, false, None)?; let mut files: Vec = Vec::new(); let packages_to_format = project_context.get_scoped_local_packages(); diff --git a/rewatch/src/main.rs b/rewatch/src/main.rs index 5380c926610..92d40c525f3 100644 --- a/rewatch/src/main.rs +++ b/rewatch/src/main.rs @@ -121,7 +121,11 @@ fn exit_code(result: Result<()>) -> i32 { Ok(()) => 0, Err(e) => { eprintln!("{:#}", e); - 1 + if e.is::() { + 2 + } else { + 1 + } } } } diff --git a/rewatch/src/watcher.rs b/rewatch/src/watcher.rs index 317878a6f18..50cb779a2c2 100644 --- a/rewatch/src/watcher.rs +++ b/rewatch/src/watcher.rs @@ -303,10 +303,11 @@ fn watch_paths_after_initialization_error(current: &[RegisteredWatch], error: &a .iter() .map(|watch| (watch.path.clone(), watch.mode)) .collect(); - if let Some(error) = error.downcast_ref::() - && !next.iter().any(|(path, _)| *path == error.package_path) + if let Some(error) = error.downcast_ref::() + && let Some(package_path) = &error.package_path + && !next.iter().any(|(path, _)| path == package_path) { - next.push((error.package_path.clone(), RecursiveMode::NonRecursive)); + next.push((package_path.clone(), RecursiveMode::NonRecursive)); } next } @@ -993,7 +994,7 @@ mod tests { std::fs::write(dependency.join("rescript.json"), r#"{"name":"#).unwrap(); let context = ProjectContext::new(&root).unwrap(); - let error = build::packages::make(&None, &context, false, None) + let error = build::packages::make(&None, &context, false, false, None) .unwrap_err() .context("Could not initialize build"); assert!(format!("{error:#}").contains("Failed to parse rescript.json")); @@ -1019,7 +1020,7 @@ mod tests { assert!(paths.contains(&(dependency.clone(), RecursiveMode::Recursive))); std::fs::write(dependency.join("rescript.json"), r#"{"name":"new-dependency"}"#).unwrap(); - let recovered = build::packages::make(&None, &context, false, None).unwrap(); + let recovered = build::packages::make(&None, &context, false, false, None).unwrap(); assert!(recovered.contains_key("new-dependency")); } diff --git a/rewatch/tests/watch/09-watch-recovers-from-invalid-config.sh b/rewatch/tests/watch/09-watch-recovers-from-invalid-config.sh index b9dfa5062f2..d01079eca37 100755 --- a/rewatch/tests/watch/09-watch-recovers-from-invalid-config.sh +++ b/rewatch/tests/watch/09-watch-recovers-from-invalid-config.sh @@ -1,129 +1,94 @@ #!/bin/bash -cd $(dirname $0) +set -e +cd "$(dirname "$0")" source "../utils.sh" -cd ../../testrepo -bold "Test: Watcher recovers from an invalid config change" +bold "Test: Watcher recovers from invalid JSON and JSX config changes" -stop_watcher() { +test_dir=$(pwd) +fixture=$(mktemp -d) +cleanup() { + cd "$fixture" || return 1 if ! exit_watcher; then - exit 1 + return 1 fi + if [ -n "${watch_pid:-}" ]; then + if process_is_running "$watch_pid"; then + kill "$watch_pid" + fi + wait "$watch_pid" 2>/dev/null || true + fi + cd "$test_dir" + rm -rf "$fixture" } +trap cleanup EXIT +cd "$fixture" -wait_for_source_retry() { - local marker='received Modify.*Test[.]res.* -> Full' - wait_for_pattern rewatch.log "$marker" 30 || return 1 - # Require an initialization failure after the source event, not another config event before it. - local preceding_errors - preceding_errors=$(awk -v marker="$marker" ' - $0 ~ marker {exit} - /Could not initialize build|invalid JSON/ {count++} - END {print count + 0} - ' rewatch.log) - wait_for_pattern_count rewatch.log 'Could not initialize build\|invalid JSON' "$((preceding_errors + 1))" 30 -} - -if ! rewatch clean > /dev/null 2>&1; then - error "Could not clean before watcher recovery test" +fail_test() { + error "$1" + cat rewatch.log >&2 exit 1 -fi -cp rescript.json rescript.json.watch-recovery.bak -rewatch_bg -vv watch > rewatch.log 2>&1 & - -if ! wait_for_file "./src/Test.mjs" 20 || ! wait_for_file "lib/watch.lock" 20; then - error "Initial watch build did not complete" - cat rewatch.log - stop_watcher - mv rescript.json.watch-recovery.bak rescript.json - exit 1 -fi - -if ! wait_for_pattern rewatch.log "Finished initial compilation" 60; then - error "Initial watch build did not finish" - cat rewatch.log - stop_watcher - mv rescript.json.watch-recovery.bak rescript.json - exit 1 -fi - -initial_completed=$(grep -c "Finished compilation" rewatch.log 2> /dev/null || true) -printf '{"name":' > rescript.json.watch-recovery.next -mv rescript.json.watch-recovery.next rescript.json - -if ! wait_for_pattern rewatch.log 'Could not initialize build\|invalid JSON' 30; then - error "Watcher did not report the invalid config" - cat rewatch.log - stop_watcher - mv rescript.json.watch-recovery.bak rescript.json - exit 1 -fi +} -# A content-only edit must retry initialization instead of compiling with the old config. -echo 'let watchRecoveryMarker = "watch-recovery-marker"' >> src/Test.res -# OCaml watches only configuration while the root config is invalid. -# Rust additionally guarantees a source-triggered initialization retry. -if ! rewatch --help | grep -q '^NAME$' && ! wait_for_source_retry; then - error "Source edit did not retry failed initialization" - cat rewatch.log - stop_watcher - mv rescript.json.watch-recovery.bak rescript.json - git restore --worktree -- src/Test.res - exit 1 -fi -if grep -q "watch-recovery-marker" src/Test.mjs 2> /dev/null; then - error "Watcher compiled the source using stale configuration" - stop_watcher - mv rescript.json.watch-recovery.bak rescript.json - git restore --worktree -- src/Test.res - exit 1 -fi +mkdir src +printf 'let value = 0\n' > src/Test.res +cat > rescript.json <<'JSON' +{ + "name": "watch-recovery", + "sources": ["src"], + "package-specs": {"module": "esmodule", "in-source": true}, + "suffix": ".mjs", + "js-post-build": {"cmd": "node record-output.cjs"} +} +JSON +cp rescript.json valid.json +cat > record-output.cjs <<'JS' +const fs = require("fs"); +const path = require("path"); +fs.appendFileSync("outputs.log", path.basename(process.argv[2]) + "\n"); +JS -node -e ' - const fs = require("fs"); - const config = JSON.parse(fs.readFileSync(process.argv[1], "utf8")); - config.suffix = ".recovered.mjs"; - fs.writeFileSync(process.argv[2], `${JSON.stringify(config, null, 2)}\n`); -' rescript.json.watch-recovery.bak rescript.json.watch-recovery.next -mv rescript.json.watch-recovery.next rescript.json +rewatch watch > rewatch.log 2>&1 & +watch_pid=$! +wait_for_pattern rewatch.log "Finished initial compilation" 60 || + fail_test "Initial watch build did not finish" -expected_completed=$((initial_completed + 1)) -if ! wait_for_pattern_count rewatch.log "Finished compilation" "$expected_completed" 30; then - error "Watcher did not recover after restoring a valid config" - cat rewatch.log - stop_watcher - mv rescript.json.watch-recovery.bak rescript.json - git restore --worktree -- src/Test.res - exit 1 -fi +previous_output='Test.mjs' +iteration=0 +for invalid_config in '{"name":' '{"name":"watch-recovery","jsx":{"version":3}}'; do + iteration=$((iteration + 1)) + completed=$(grep -c "Finished compilation" rewatch.log || true) + errors=$(grep -c 'Could not initialize build\|invalid JSON\|jsx.version' rewatch.log || true) + printf '%s\n' "$invalid_config" > rescript.next + mv rescript.next rescript.json + wait_for_pattern_count rewatch.log 'Could not initialize build\|invalid JSON\|jsx.version' "$((errors + 1))" 30 || + fail_test "Watcher did not report the invalid config" -if ! wait_for_pattern "./src/Test.recovered.mjs" "watch-recovery-marker" 20; then - error "Recovered watcher did not compile with the restored config" - cat rewatch.log - stop_watcher - mv rescript.json.watch-recovery.bak rescript.json - git restore --worktree -- src/Test.res - exit 1 -fi + # Record emitted files across recovery, even if a later cleanup removes them. + # Both CLIs must leave the old output untouched while the config is invalid. + : > outputs.log + printf 'let marker%s = "watch-recovery-marker-%s"\n' "$iteration" "$iteration" >> src/Test.res + if wait_for_pattern outputs.log "$previous_output" 5; then + fail_test "Watcher compiled the source using stale configuration" + fi + process_is_running "$watch_pid" || fail_test "Watcher exited after the config error" -stop_watcher -if ! rewatch clean > /dev/null 2>&1; then - error "Could not clean after watcher recovery test" - exit 1 -fi -mv rescript.json.watch-recovery.bak rescript.json -git restore --worktree -- src/Test.res -rm -f rewatch.log -if ! rewatch build > /dev/null 2>&1; then - error "Could not rebuild after watcher recovery test" - exit 1 -fi + node - "$iteration" <<'JS' +const fs = require("fs"); +const config = JSON.parse(fs.readFileSync("valid.json", "utf8")); +config.suffix = `.recovered${process.argv[2]}.mjs`; +fs.writeFileSync("rescript.next", JSON.stringify(config)); +JS + mv rescript.next rescript.json + wait_for_pattern_count rewatch.log "Finished compilation" "$((completed + 1))" 30 || + fail_test "Watcher did not recover after restoring a valid config" + wait_for_pattern "src/Test.recovered$iteration.mjs" "watch-recovery-marker-$iteration" 20 || + fail_test "Recovered watcher did not compile the edited source with the new suffix" + if grep -Fq "$previous_output" outputs.log; then + fail_test "Watcher emitted stale output before recovering" + fi + wait_for_file_gone lib/build.lock 20 || fail_test "Recovery left the build lock behind" + previous_output="Test.recovered$iteration.mjs" +done -if git diff --exit-code . > /dev/null 2>&1 && [ -z "$(git ls-files --others --exclude-standard .)" ]; then - success "Watcher recovered from invalid config" -else - error "Watcher recovery test left repository changes" - git diff . - git ls-files --others --exclude-standard . - exit 1 -fi +success "Both config errors recover without compiling against stale configuration" diff --git a/rewatch/tests/watch/10-watch-recovers-new-dependency-config.sh b/rewatch/tests/watch/10-watch-recovers-new-dependency-config.sh index f2654a6e9ed..7e12b4df97b 100755 --- a/rewatch/tests/watch/10-watch-recovers-new-dependency-config.sh +++ b/rewatch/tests/watch/10-watch-recovers-new-dependency-config.sh @@ -1,83 +1,78 @@ #!/bin/bash -cd $(dirname $0) +set -e +cd "$(dirname "$0")" source "../utils.sh" -cd ../../testrepo bold "Test: Watcher recovers when only a newly added dependency config is fixed" -dependency_config=packages/standalone/rescript.json - -stop_and_restore() { +test_dir=$(pwd) +fixture=$(mktemp -d) +cleanup() { + cd "$fixture" || return 1 if ! exit_watcher; then return 1 fi - mv rescript.json.watch-dependency.bak rescript.json - mv "$dependency_config.watch-dependency.bak" "$dependency_config" - rm -f rewatch.log - if ! rewatch build > /dev/null 2>&1; then - error "Could not rebuild testrepo after dependency recovery test" - return 1 - fi - # The root no longer depends on standalone; restore its artifacts with its own build. - if ! (cd packages/standalone && rewatch build > /dev/null 2>&1); then - error "Could not rebuild standalone after dependency recovery test" - return 1 + if [ -n "${watch_pid:-}" ]; then + if process_is_running "$watch_pid"; then + kill "$watch_pid" + fi + wait "$watch_pid" 2>/dev/null || true fi + cd "$test_dir" + rm -rf "$fixture" } +trap cleanup EXIT +cd "$fixture" fail_test() { error "$1" - cat rewatch.log - if ! stop_and_restore; then - exit 1 - fi + cat rewatch.log >&2 exit 1 } -if ! rewatch clean > /dev/null 2>&1; then - error "Could not clean before dependency recovery test" - exit 1 -fi -cp rescript.json rescript.json.watch-dependency.bak -cp "$dependency_config" "$dependency_config.watch-dependency.bak" -rewatch_bg watch > rewatch.log 2>&1 & +node <<'JS' +const fs = require("fs"); +const path = require("path"); +const config = name => ({ + name, + sources: ["src"], + "package-specs": {module: "esmodule", "in-source": true}, + suffix: ".mjs", +}); +fs.mkdirSync("src"); +fs.writeFileSync("src/Main.res", "let value = 0\n"); +fs.writeFileSync("rescript.json", JSON.stringify(config("watch-dependency"))); +fs.mkdirSync("packages/new-dependency/src", {recursive: true}); +fs.writeFileSync("packages/new-dependency/src/Dep.res", "let value = 1\n"); +fs.writeFileSync("packages/new-dependency/package.json", JSON.stringify({name: "new-dependency", version: "1.0.0"})); +fs.writeFileSync("valid-dependency.json", JSON.stringify(config("new-dependency"))); +fs.writeFileSync("packages/new-dependency/rescript.json", '{"name":'); +fs.mkdirSync("node_modules"); +fs.symlinkSync(path.resolve("packages/new-dependency"), "node_modules/new-dependency", "junction"); +JS -if ! wait_for_file "lib/watch.lock" 20 || ! wait_for_pattern rewatch.log "Finished initial compilation" 60; then +rewatch watch > rewatch.log 2>&1 & +watch_pid=$! +wait_for_pattern rewatch.log "Finished initial compilation" 60 || fail_test "Initial watch build did not finish" -fi -initial_completed=$(grep -c "Finished compilation" rewatch.log 2> /dev/null || true) - -# Standalone is installed but absent from the root's original ReScript dependency graph. -printf '{"name":' > "$dependency_config.watch-dependency.next" -mv "$dependency_config.watch-dependency.next" "$dependency_config" -node -e ' - const fs = require("fs"); - const config = JSON.parse(fs.readFileSync(process.argv[1], "utf8")); - config.dependencies.push("@testrepo/standalone"); - fs.writeFileSync(process.argv[2], `${JSON.stringify(config, null, 2)}\n`); -' rescript.json rescript.json.watch-dependency.next -mv rescript.json.watch-dependency.next rescript.json +completed=$(grep -c "Finished compilation" rewatch.log || true) -if ! wait_for_pattern rewatch.log 'Could not read dependency config.*standalone\|Could not build package tree for.*standalone' 30; then +node <<'JS' +const fs = require("fs"); +const config = JSON.parse(fs.readFileSync("rescript.json", "utf8")); +config.dependencies = ["new-dependency"]; +fs.writeFileSync("rescript.next", JSON.stringify(config)); +JS +mv rescript.next rescript.json +wait_for_pattern rewatch.log "Could not build package tree for 'new-dependency'" 30 || fail_test "Watcher did not report the newly added dependency's invalid config" -fi -# Only fix the new dependency. Its directory must have a recovery watch by this point. -cp "$dependency_config.watch-dependency.bak" "$dependency_config.watch-dependency.next" -mv "$dependency_config.watch-dependency.next" "$dependency_config" -if ! wait_for_pattern_count rewatch.log "Finished compilation" "$((initial_completed + 1))" 30; then - fail_test "Watcher did not recover after fixing only the new dependency's config" -fi +# Only fix the dependency: its directory must have a recovery watch by now. +cp valid-dependency.json packages/new-dependency/rescript.next +mv packages/new-dependency/rescript.next packages/new-dependency/rescript.json +wait_for_pattern_count rewatch.log "Finished compilation" "$((completed + 1))" 30 || + fail_test "Watcher did not recover after fixing only the dependency config" +wait_for_file packages/new-dependency/src/Dep.mjs 20 || + fail_test "Recovered watcher did not compile the new dependency" -if ! stop_and_restore; then - exit 1 -fi - -if git diff --exit-code . > /dev/null 2>&1 && [ -z "$(git ls-files --others --exclude-standard .)" ]; then - success "Watcher recovered after the new dependency config was fixed" -else - error "Dependency recovery test left repository changes" - git diff . - git ls-files --others --exclude-standard . - exit 1 -fi +success "Watcher recovered after only the new dependency config was fixed" diff --git a/tests/build_tests/weird_deps/input.js b/tests/build_tests/weird_deps/input.js index 145846d0a21..092fdbef323 100644 --- a/tests/build_tests/weird_deps/input.js +++ b/tests/build_tests/weird_deps/input.js @@ -6,8 +6,8 @@ import { setup } from "#dev/process"; const { execBuild, execClean } = setup(import.meta.dirname); const out = await execBuild(); -assert.notEqual(out.status, 0); +assert.equal(out.status, 2); assert.match(out.stderr, /could not build package tree/i); assert.match(out.stderr, /dependency 'weird'/i); -assert.doesNotMatch(out.stdout, /error building package tree/i); +assert.match(out.stderr, /at path '.*weird_deps'/i); await execClean(); diff --git a/tests/build_tests/weird_devdeps/input.js b/tests/build_tests/weird_devdeps/input.js index 145846d0a21..724b7a19811 100644 --- a/tests/build_tests/weird_devdeps/input.js +++ b/tests/build_tests/weird_devdeps/input.js @@ -6,8 +6,8 @@ import { setup } from "#dev/process"; const { execBuild, execClean } = setup(import.meta.dirname); const out = await execBuild(); -assert.notEqual(out.status, 0); +assert.equal(out.status, 2); assert.match(out.stderr, /could not build package tree/i); assert.match(out.stderr, /dependency 'weird'/i); -assert.doesNotMatch(out.stdout, /error building package tree/i); +assert.match(out.stderr, /at path '.*weird_devdeps'/i); await execClean(); From fd08da7ed2f836da4f80527852362ee646a8bd47 Mon Sep 17 00:00:00 2001 From: Florian Hammerschmidt Date: Tue, 20 Oct 2026 16:40:42 +0200 Subject: [PATCH 7/8] Tolerate watcher exit during recovery-test cleanup The Windows recovery assertions pass, but cleanup can fail when the background launcher exits between the process check and kill. Match the existing watcher test's best-effort launcher cleanup in both new recovery scripts. Shutdown errors from exit_watcher still fail before cleanup removes the fixture. Validation: The native toolchain builds. A focused probe reproduces the old failure in both scripts and verifies the fix for an already-exited launcher, a running launcher, and a failed watcher shutdown. Shell syntax and whitespace checks pass. Signed-off-by: Florian Hammerschmidt --- rewatch/tests/watch/09-watch-recovers-from-invalid-config.sh | 2 +- rewatch/tests/watch/10-watch-recovers-new-dependency-config.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/rewatch/tests/watch/09-watch-recovers-from-invalid-config.sh b/rewatch/tests/watch/09-watch-recovers-from-invalid-config.sh index d01079eca37..15aaaaf317a 100755 --- a/rewatch/tests/watch/09-watch-recovers-from-invalid-config.sh +++ b/rewatch/tests/watch/09-watch-recovers-from-invalid-config.sh @@ -14,7 +14,7 @@ cleanup() { fi if [ -n "${watch_pid:-}" ]; then if process_is_running "$watch_pid"; then - kill "$watch_pid" + kill "$watch_pid" 2>/dev/null || true fi wait "$watch_pid" 2>/dev/null || true fi diff --git a/rewatch/tests/watch/10-watch-recovers-new-dependency-config.sh b/rewatch/tests/watch/10-watch-recovers-new-dependency-config.sh index 7e12b4df97b..5018984cb1a 100755 --- a/rewatch/tests/watch/10-watch-recovers-new-dependency-config.sh +++ b/rewatch/tests/watch/10-watch-recovers-new-dependency-config.sh @@ -14,7 +14,7 @@ cleanup() { fi if [ -n "${watch_pid:-}" ]; then if process_is_running "$watch_pid"; then - kill "$watch_pid" + kill "$watch_pid" 2>/dev/null || true fi wait "$watch_pid" 2>/dev/null || true fi From 0e3814bf5edc8bfac174791045b3c6d6d3cc7913 Mon Sep 17 00:00:00 2001 From: Florian Hammerschmidt Date: Tue, 20 Oct 2026 17:01:18 +0200 Subject: [PATCH 8/8] Check the native watcher PID in recovery tests Git Bash's background-process ID is not the native Windows process ID expected by process_is_running. Read the native PID from watch.lock for the recovery test's liveness assertion. Let Bash kill and reap its own launcher without passing that launcher ID through the native-process check. Validation: The native toolchain builds. A focused probe reproduces the false failure with different Bash and native IDs, accepts the live native watcher, and rejects an exited watcher. Both cleanup routines pass the already-exited and running-launcher probes and preserve shutdown failures. Shell syntax and whitespace checks pass. Signed-off-by: Florian Hammerschmidt --- .../tests/watch/09-watch-recovers-from-invalid-config.sh | 6 ++---- .../tests/watch/10-watch-recovers-new-dependency-config.sh | 4 +--- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/rewatch/tests/watch/09-watch-recovers-from-invalid-config.sh b/rewatch/tests/watch/09-watch-recovers-from-invalid-config.sh index 15aaaaf317a..9cbf472b000 100755 --- a/rewatch/tests/watch/09-watch-recovers-from-invalid-config.sh +++ b/rewatch/tests/watch/09-watch-recovers-from-invalid-config.sh @@ -13,9 +13,7 @@ cleanup() { return 1 fi if [ -n "${watch_pid:-}" ]; then - if process_is_running "$watch_pid"; then - kill "$watch_pid" 2>/dev/null || true - fi + kill "$watch_pid" 2>/dev/null || true wait "$watch_pid" 2>/dev/null || true fi cd "$test_dir" @@ -71,7 +69,7 @@ for invalid_config in '{"name":' '{"name":"watch-recovery","jsx":{"version":3}}' if wait_for_pattern outputs.log "$previous_output" 5; then fail_test "Watcher compiled the source using stale configuration" fi - process_is_running "$watch_pid" || fail_test "Watcher exited after the config error" + process_is_running "$(cat lib/watch.lock)" || fail_test "Watcher exited after the config error" node - "$iteration" <<'JS' const fs = require("fs"); diff --git a/rewatch/tests/watch/10-watch-recovers-new-dependency-config.sh b/rewatch/tests/watch/10-watch-recovers-new-dependency-config.sh index 5018984cb1a..78701f618af 100755 --- a/rewatch/tests/watch/10-watch-recovers-new-dependency-config.sh +++ b/rewatch/tests/watch/10-watch-recovers-new-dependency-config.sh @@ -13,9 +13,7 @@ cleanup() { return 1 fi if [ -n "${watch_pid:-}" ]; then - if process_is_running "$watch_pid"; then - kill "$watch_pid" 2>/dev/null || true - fi + kill "$watch_pid" 2>/dev/null || true wait "$watch_pid" 2>/dev/null || true fi cd "$test_dir"