diff --git a/changes/watch-rebuild-recovery.fix.md b/changes/watch-rebuild-recovery.fix.md new file mode 100644 index 0000000000..9ea4c0fede --- /dev/null +++ b/changes/watch-rebuild-recovery.fix.md @@ -0,0 +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-ocaml/README.md b/rewatch-ocaml/README.md index af62cd8677..044fad6f45 100644 --- a/rewatch-ocaml/README.md +++ b/rewatch-ocaml/README.md @@ -105,6 +105,10 @@ 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 existing `node_modules` of an ancestor directory, where the dependency lookup finds it. diff --git a/rewatch-ocaml/tests/check_command_validation.sh b/rewatch-ocaml/tests/check_command_validation.sh index d4a0b5a7eb..0d7e1b745a 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 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 + 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 } # Rust needs RESCRIPT_BSC_EXE to exist; OCaml builds and formats with the @@ -1190,14 +1190,16 @@ if ! grep -F \ cat "$work/ocaml.out" "$work/ocaml.err" >&2 exit 1 fi -run_case build-missing-dependency exit2 exit2 build "$work/missing-dependency" +run_case build-missing-dependency reject exit2 build "$work/missing-dependency" require_both_errors_contain build-missing-dependency \ - "Could not build package tree reading dependency 'absent' at path '$work/missing-dependency'. Error:" -run_case build-configless-dependency exit2 exit2 build "$work/configless-dependency" + "Could not build package tree reading dependency 'absent'" +run_case build-configless-dependency reject 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 exit2 exit2 build "$work/malformed-dependency" +run_case build-malformed-dependency reject 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' @@ -1224,55 +1226,46 @@ if ! grep -F "$duplicate_warning" "$work/rust.err.norm" >/dev/null || \ exit 1 fi -"$rust" watch "$work/watch-config-rust" \ - >"$work/watch-rust.out" 2>"$work/watch-rust.err" & -rust_watch_pid=$! -background_pids="$background_pids $rust_watch_pid" -wait_for_file "$work/watch-config-rust/src/A.js" -wait_for_text "$work/watch-config-rust/lib/ocaml/.compiler.log" "#Done(" -printf '{ invalid json\n' >"$work/watch-config-rust/rescript.json" -wait_for_text "$work/watch-rust.err" "Could not initialize build" -wait_for_exit "$rust_watch_pid" -set +e -wait "$rust_watch_pid" -rust_watch_status=$? -set -e -if [ "$rust_watch_status" -ne 101 ]; then - printf 'watch-invalid-config-rebuild: expected Rust panic exit 101, got %s\n' \ - "$rust_watch_status" >&2 - cat "$work/watch-rust.out" "$work/watch-rust.err" >&2 - exit 1 -fi - -"$ocaml" watch "$work/watch-config-ocaml" \ - >"$work/watch-ocaml.out" 2>"$work/watch-ocaml.err" & -ocaml_watch_pid=$! -background_pids="$background_pids $ocaml_watch_pid" -wait_for_file "$work/watch-config-ocaml/src/A.js" -wait_for_text "$work/watch-config-ocaml/lib/ocaml/.compiler.log" "#Done(" -printf '{ invalid json\n' >"$work/watch-config-ocaml/rescript.json" -wait_for_text "$work/watch-ocaml.err" "invalid JSON" -if ! kill -0 "$ocaml_watch_pid" 2>/dev/null; then - echo "OCaml watcher exited after a recoverable config error" >&2 - cat "$work/watch-ocaml.out" "$work/watch-ocaml.err" >&2 - exit 1 -fi -printf '{"name":"watch-config","sources":["src"],"dependencies":["definitely-missing-dep"]}\n' \ - >"$work/watch-config-ocaml/rescript.json" -wait_for_text "$work/watch-ocaml.err" "definitely-missing-dep" -if ! kill -0 "$ocaml_watch_pid" 2>/dev/null; then - echo "OCaml watcher exited after a recoverable dependency error" >&2 - cat "$work/watch-ocaml.out" "$work/watch-ocaml.err" >&2 - exit 1 -fi -printf '{"name":"watch-config","sources":["src"],"package-specs":{"module":"esmodule","in-source":true,"suffix":".mjs"}}\n' \ - >"$work/watch-config-ocaml/rescript.json" -wait_for_file "$work/watch-config-ocaml/src/A.mjs" -wait_for_file "$work/watch-config-ocaml/lib/bs/build.ninja" -if ! terminate_and_wait "$ocaml_watch_pid" "recoverable-config watcher"; then - cat "$work/watch-ocaml.out" "$work/watch-ocaml.err" >&2 - exit 1 -fi +for implementation in rust ocaml; do + if [ "$implementation" = rust ]; then + executable=$rust + config_error="Could not initialize build" + else + executable=$ocaml + config_error="invalid JSON" + fi + watch_project="$work/watch-config-$implementation" + watch_out="$work/watch-$implementation.out" + watch_err="$work/watch-$implementation.err" + "$executable" watch "$watch_project" >"$watch_out" 2>"$watch_err" & + watch_pid=$! + background_pids="$background_pids $watch_pid" + wait_for_file "$watch_project/src/A.js" + wait_for_text "$watch_project/lib/ocaml/.compiler.log" "#Done(" + printf '{ invalid json\n' >"$watch_project/rescript.json" + wait_for_text "$watch_err" "$config_error" + if ! kill -0 "$watch_pid" 2>/dev/null; then + echo "$implementation watcher exited after a recoverable config error" >&2 + cat "$watch_out" "$watch_err" >&2 + exit 1 + fi + printf '{"name":"watch-config","sources":["src"],"dependencies":["definitely-missing-dep"]}\n' \ + >"$watch_project/rescript.json" + wait_for_text "$watch_err" "definitely-missing-dep" + if ! kill -0 "$watch_pid" 2>/dev/null; then + echo "$implementation watcher exited after a recoverable dependency error" >&2 + cat "$watch_out" "$watch_err" >&2 + exit 1 + fi + printf '{"name":"watch-config","sources":["src"],"package-specs":{"module":"esmodule","in-source":true,"suffix":".mjs"}}\n' \ + >"$watch_project/rescript.json" + wait_for_file "$watch_project/src/A.mjs" + wait_for_file "$watch_project/lib/bs/build.ninja" + if ! terminate_and_wait "$watch_pid" "$implementation recoverable-config watcher"; then + cat "$watch_out" "$watch_err" >&2 + exit 1 + fi +done checked=$((checked + 1)) retained_graph_marker="$work/watch-retained-graph/after-build.log" @@ -1529,9 +1522,9 @@ if ! cmp -s "$work/quiet-watch-rust.err.norm" \ fi checked=$((checked + 1)) -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_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_both clean "$work/clean-duplicate-{impl}" if [ "$(classify "$rust_status")" != reject ] || [ "$ocaml_status" -ne 0 ] || \ [ ! -e "$work/clean-duplicate-rust/src/one/A.js" ] || \ @@ -1549,8 +1542,8 @@ set +e "$rust" watch "$work/missing-dependency" >"$work/rust.out" 2>"$work/rust.err" rust_status=$? set -e -if [ "$rust_status" -ne 2 ]; then - echo "watch-missing-dependency: expected Rust exit 2, got $rust_status" >&2 +if [ "$rust_status" -ne 1 ]; then + echo "watch-missing-dependency: expected Rust exit 1, got $rust_status" >&2 cat "$work/rust.out" "$work/rust.err" >&2 exit 1 fi @@ -1581,11 +1574,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 exit2 exit2 format + format-missing-dependency reject exit2 format run_case --cwd "$work/configless-dependency" \ - format-configless-dependency exit2 exit2 format + format-configless-dependency reject exit2 format run_case --cwd "$work/malformed-dependency" \ - format-malformed-dependency exit2 exit2 format + format-malformed-dependency reject 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/src/build.rs b/rewatch/src/build.rs index e5ce179dd7..d01073b01e 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, 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 4fdc3e1faa..0fd7bb1a3f 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 5ef4a417cd..a9520df619 100644 --- a/rewatch/src/build/packages.rs +++ b/rewatch/src/build/packages.rs @@ -8,12 +8,12 @@ use crate::helpers::StrippedVerbatimPath; use crate::helpers::emojis::*; use crate::project_context::{MonoRepoContext, ProjectContext}; use ahash::{AHashMap, AHashSet}; -use anyhow::{Result, anyhow}; -use console::style; +use anyhow::{Context, Result, anyhow}; 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 +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. +#[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 @@ -293,10 +317,9 @@ fn read_dependencies( registered_dependencies_set: &mut AHashSet, project_context: &ProjectContext, package_config: &Config, - 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 +334,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 +368,15 @@ 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}", - ); - - 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) - } - } - } - }; + .map(|package_name| -> Result { + 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, + })?; let is_local_dep = is_local_dependency(project_context, package_name, &canonical_path); @@ -388,18 +384,18 @@ fn read_dependencies( &mut registered_dependencies_set.to_owned(), project_context, &config, - 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() } @@ -558,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(); @@ -582,10 +574,9 @@ fn read_packages( &mut registered_dependencies_set, project_context, &project_context.current_config, - show_progress, /* is local dep */ true, prod, - )); + )?); for d in dependencies.iter() { if !map.contains_key(&d.name) { @@ -843,11 +834,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 eed70f0812..ff0b897fba 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 4e58c1dff6..317878a6f1 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, @@ -279,6 +298,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 { @@ -402,6 +434,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..."); @@ -459,32 +492,30 @@ 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, event.kind) { + match (needs_compile_type, source_compile_type) { ( 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); 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); if let Ok(canonicalized_path_buf) = path_buf .canonicalize() .map(StrippedVerbatimPath::to_stripped_verbatim_path) @@ -502,33 +533,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(_) => (), } @@ -541,10 +573,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, _) => (), @@ -604,7 +633,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, @@ -616,7 +645,8 @@ async fn async_watch( features.clone(), SourceMapCommand::Watch, ) - .expect("Could not initialize build"); + .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. @@ -638,10 +668,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, @@ -652,7 +682,23 @@ async fn async_watch( result, ); } - Err(_) => { + Ok(Err(_)) => { + if should_clear_screen(clear_screen, show_progress, plain_output) { + print_build_failed_footer(); + } + } + 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(); } @@ -911,6 +957,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, 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, 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 84e84e1803..b28e2c0dad 100755 --- a/rewatch/tests/suite.sh +++ b/rewatch/tests/suite.sh @@ -151,6 +151,8 @@ fi ./watch/06-watch-missing-source-folder.sh && ./watch/07-changed-interface-after-failed-implementation.sh && ./watch/08-watch-overlapping-source-removed.sh && +./watch/09-watch-recovers-from-invalid-config.sh && +./watch/10-watch-recovers-new-dependency-config.sh && # Lock tests ./lock/01-lock-when-watching.sh && diff --git a/rewatch/tests/watch/09-watch-recovers-from-invalid-config.sh b/rewatch/tests/watch/09-watch-recovers-from-invalid-config.sh new file mode 100755 index 0000000000..b9dfa5062f --- /dev/null +++ b/rewatch/tests/watch/09-watch-recovers-from-invalid-config.sh @@ -0,0 +1,129 @@ +#!/bin/bash +cd $(dirname $0) +source "../utils.sh" +cd ../../testrepo + +bold "Test: Watcher recovers from an invalid config change" + +stop_watcher() { + if ! exit_watcher; then + exit 1 + 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|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" + 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 + +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 + stop_watcher + mv rescript.json.watch-recovery.bak rescript.json + git restore --worktree -- src/Test.res + exit 1 +fi + +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 + +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 + +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 diff --git a/rewatch/tests/watch/10-watch-recovers-new-dependency-config.sh b/rewatch/tests/watch/10-watch-recovers-new-dependency-config.sh new file mode 100755 index 0000000000..f2654a6e9e --- /dev/null +++ b/rewatch/tests/watch/10-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\|Could not build package tree for.*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 diff --git a/tests/build_tests/weird_deps/input.js b/tests/build_tests/weird_deps/input.js index d98217346b..145846d0a2 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 d98217346b..145846d0a2 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();