diff --git a/changes/watch-rebuild-recovery.fix.md b/changes/watch-rebuild-recovery.fix.md new file mode 100644 index 0000000000..89152cd15c --- /dev/null +++ b/changes/watch-rebuild-recovery.fix.md @@ -0,0 +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 diff --git a/rewatch-ocaml/README.md b/rewatch-ocaml/README.md index f79281cd0e..c1a625100f 100644 --- a/rewatch-ocaml/README.md +++ b/rewatch-ocaml/README.md @@ -104,6 +104,8 @@ 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. +- 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 972d94c1ac..e9c6dad799 100755 --- a/rewatch-ocaml/tests/check_command_validation.sh +++ b/rewatch-ocaml/tests/check_command_validation.sh @@ -1193,6 +1193,11 @@ fi 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' 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 build package tree for 'no-config' at path '$work/configless-dependency'. Error:" @@ -1224,55 +1229,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" diff --git a/rewatch-ocaml/tests/config_acceptance_cases.tsv b/rewatch-ocaml/tests/config_acceptance_cases.tsv index f9753331bf..d9c4420dd5 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 e5ce179dd7..733d66bccf 100644 --- a/rewatch/src/build.rs +++ b/rewatch/src/build.rs @@ -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/packages.rs b/rewatch/src/build/packages.rs index 5ef4a417cd..73c97302ac 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 dependency context for CLI exit codes and recovery watches after config errors. +#[derive(Debug)] +pub struct DependencyError { + pub package_path: Option, + package_name: String, + root_path: PathBuf, +} + +impl fmt::Display for DependencyError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!( + f, + "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() + ) + } +} + /// 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,16 @@ 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 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(|| 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); @@ -388,18 +385,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 +555,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 +575,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) { @@ -847,7 +839,15 @@ pub fn make( 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).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 4314767163..d677c2a92d 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/main.rs b/rewatch/src/main.rs index 5380c92661..92d40c525f 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 4e58c1dff6..50cb779a2c 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,20 @@ 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::() + && let Some(package_path) = &error.package_path + && !next.iter().any(|(path, _)| path == package_path) + { + next.push((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 +435,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 +493,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 +534,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 +574,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 +634,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 +646,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 +669,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 +683,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 +958,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 703304ac97..05e12d053f 100755 --- a/rewatch/tests/suite.sh +++ b/rewatch/tests/suite.sh @@ -104,6 +104,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..9cbf472b00 --- /dev/null +++ b/rewatch/tests/watch/09-watch-recovers-from-invalid-config.sh @@ -0,0 +1,92 @@ +#!/bin/bash +set -e +cd "$(dirname "$0")" +source "../utils.sh" + +bold "Test: Watcher recovers from invalid JSON and JSX config changes" + +test_dir=$(pwd) +fixture=$(mktemp -d) +cleanup() { + cd "$fixture" || return 1 + if ! exit_watcher; then + return 1 + fi + if [ -n "${watch_pid:-}" ]; then + kill "$watch_pid" 2>/dev/null || true + 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 >&2 + exit 1 +} + +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 + +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" + +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" + + # 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 "$(cat lib/watch.lock)" || fail_test "Watcher exited after the config error" + + 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 + +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 new file mode 100755 index 0000000000..78701f618a --- /dev/null +++ b/rewatch/tests/watch/10-watch-recovers-new-dependency-config.sh @@ -0,0 +1,76 @@ +#!/bin/bash +set -e +cd "$(dirname "$0")" +source "../utils.sh" + +bold "Test: Watcher recovers when only a newly added dependency config is fixed" + +test_dir=$(pwd) +fixture=$(mktemp -d) +cleanup() { + cd "$fixture" || return 1 + if ! exit_watcher; then + return 1 + fi + if [ -n "${watch_pid:-}" ]; then + kill "$watch_pid" 2>/dev/null || true + 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 >&2 + exit 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 + +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" +completed=$(grep -c "Finished compilation" rewatch.log || true) + +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" + +# 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" + +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 d98217346b..092fdbef32 100644 --- a/tests/build_tests/weird_deps/input.js +++ b/tests/build_tests/weird_deps/input.js @@ -6,7 +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.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 d98217346b..724b7a1981 100644 --- a/tests/build_tests/weird_devdeps/input.js +++ b/tests/build_tests/weird_devdeps/input.js @@ -6,7 +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.match(out.stderr, /at path '.*weird_devdeps'/i); await execClean();