diff --git a/changes/rewatch-overlapping-unwatch.fix.md b/changes/rewatch-overlapping-unwatch.fix.md new file mode 100644 index 0000000000..61454ff211 --- /dev/null +++ b/changes/rewatch-overlapping-unwatch.fix.md @@ -0,0 +1 @@ +- Rewatch: keep watching a source folder on Linux when an overlapping recursive source folder is removed from `rescript.json`. https://github.com/rescript-lang/rescript/pull/8839 diff --git a/rewatch/src/watcher.rs b/rewatch/src/watcher.rs index d27e416a4e..4e58c1dff6 100644 --- a/rewatch/src/watcher.rs +++ b/rewatch/src/watcher.rs @@ -194,7 +194,14 @@ fn register_watches( watch_paths: &[(PathBuf, RecursiveMode)], ) -> Vec { let mut registered = Vec::with_capacity(watch_paths.len()); - for (path, mode) in watch_paths { + // Register non-recursive paths first. notify's inotify backend keeps one + // recursive flag per directory, set by the last registration that reaches + // it, and only watches a folder created later if its parent's flag is + // recursive. A non-recursive path inside a recursive one (an overlapping + // source folder) must not clear the flag that the recursive one set. + let mut ordered: Vec<&(PathBuf, RecursiveMode)> = watch_paths.iter().collect(); + ordered.sort_by_key(|(_, mode)| *mode == RecursiveMode::Recursive); + for (path, mode) in ordered { let mode_str = if *mode == RecursiveMode::Recursive { "recursive" } else { @@ -216,12 +223,32 @@ fn register_watches( registered } -/// Brings the registered watches from `current` to `next`, touching only the paths that changed. +/// Whether `update_watches` registers only the new paths, leaving unchanged watches alone, rather +/// than registering every path again (see `update_watches` for why this differs per platform). +const REGISTER_ONLY_NEW_WATCHES: bool = cfg!(target_os = "macos"); + +/// Brings the registered watches from `current` to `next`, which happens on every full rebuild: +/// after a `rescript.json` change, but also when a source file is added, removed or renamed. +/// +/// Stale watches are unwatched first: those whose path is no longer in `next`, and those whose +/// directory was replaced since it was registered. What happens to the other watches depends on +/// how the platform's watcher backend (from the `notify` crate) treats overlapping watches: +/// +/// - **macOS (FSEvents):** every `watch`/`unwatch` call restarts the event stream, and events that +/// happen during the restart are lost. Each registered path is watched independently, so +/// unwatching one path never affects another. Only the new paths are registered, so a full +/// rebuild that keeps the same paths doesn't restart the stream at all. /// -/// Some backends restart their event stream on every `watch`/`unwatch` call (FSEvents on macOS -/// does), and events that happen during the restart are lost. Leaving unchanged watches alone -/// avoids those gaps entirely in the common case where a full rebuild keeps the same paths. -/// Watches whose directory was replaced since registration are registered again. +/// - **Linux (inotify) and other platforms:** notify keeps one watch per directory, shared by +/// every registered path that reaches it: overlapping source folders, other spellings of a +/// folder (`..`, symlinks) and directory symlinks inside a recursive folder. Unwatching a stale +/// path can therefore also drop a watch that is still needed (#8825). So every path in `next` is +/// registered again after the unwatches. Registering a directory that is already watched only +/// updates its existing watch, so watches that weren't dropped keep reporting events without a +/// gap, and those that were dropped are restored. +/// +/// Windows takes this path too. It has no stable directory identity, so every watch counts as +/// stale there and is unwatched and registered again on each full rebuild. /// /// Returns the watches that are registered afterwards, which excludes paths that failed to register. fn update_watches( @@ -236,6 +263,9 @@ fn update_watches( for watch in stale { let _ = watcher.unwatch(&watch.path); } + if !REGISTER_ONLY_NEW_WATCHES { + return register_watches(watcher, next); + } let added: WatchPaths = next .iter() .filter(|(path, mode)| { diff --git a/rewatch/tests/suite.sh b/rewatch/tests/suite.sh index 5f8d194f7a..84e84e1803 100755 --- a/rewatch/tests/suite.sh +++ b/rewatch/tests/suite.sh @@ -150,6 +150,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/08-watch-overlapping-source-removed.sh && # Lock tests ./lock/01-lock-when-watching.sh && diff --git a/rewatch/tests/watch/08-watch-overlapping-source-removed.sh b/rewatch/tests/watch/08-watch-overlapping-source-removed.sh new file mode 100755 index 0000000000..e8c8066b57 --- /dev/null +++ b/rewatch/tests/watch/08-watch-overlapping-source-removed.sh @@ -0,0 +1,135 @@ +#!/bin/bash +cd $(dirname $0) +source "../utils.sh" +cd ../../testrepo + +bold "Test: Watcher keeps watching a folder when an overlapping source folder is removed" + +error_output=$(rewatch clean 2>&1) +if [ $? -eq 0 ]; +then + success "Repo Cleaned" +else + error "Error Cleaning Repo" + printf "%s\n" "$error_output" >&2 + exit 1 +fi + +DEP01_CONFIG=packages/dep01/rescript.json +RETAINED=packages/dep01/src/retained + +cleanup() { + git restore --worktree -- "$DEP01_CONFIG" + rm -rf "$RETAINED" + rm -f rewatch.log +} + +# src/retained is watched both through the recursive src folder and as a +# source folder of its own +mkdir -p "$RETAINED" +echo 'let value = 1' > "$RETAINED/Retained.res" +node -e " + const fs = require('fs'); + const config = JSON.parse(fs.readFileSync('$DEP01_CONFIG', 'utf8')); + config.sources = [{dir: 'src', subdirs: true}, 'src/retained']; + fs.writeFileSync('$DEP01_CONFIG', JSON.stringify(config, null, 2) + '\n'); +" + +retained_output() { + find "$RETAINED" -name 'Retained.*js' | head -n 1 +} + +rewatch_bg watch > rewatch.log 2>&1 & +success "Watcher Started" + +wait_for_retained_value() { + local value=$1 + local timeout=30 + while [ "$timeout" -gt 0 ]; do + output=$(retained_output) + if [ -n "$output" ] && grep -q "$value" "$output"; then + return 0 + fi + sleep 1 + timeout=$((timeout - 1)) + done + return 1 +} + +if ! wait_for_retained_value "= 1"; then + error "Initial build did not compile the retained folder" + cat rewatch.log + exit_watcher + cleanup + exit 1 +fi +success "Initial build completed" + +# A folder created inside src/retained belongs to the recursive src folder, so +# the watch of src/retained must stay recursive although src/retained is also +# watched on its own +mkdir "$RETAINED/nested" +echo 'let value = 1' > "$RETAINED/nested/Nested.res" +timeout=30 +until [ -n "$(find "$RETAINED/nested" -name 'Nested.*js')" ] || [ "$timeout" -le 0 ]; do + sleep 1 + timeout=$((timeout - 1)) +done +if [ -n "$(find "$RETAINED/nested" -name 'Nested.*js')" ]; then + success "A folder created inside the retained folder is watched" +else + error "A folder created inside the retained folder is not watched" + cat rewatch.log + exit_watcher + cleanup + exit 1 +fi + +completed_builds=$(grep -c "Finished .*compilation" rewatch.log 2>/dev/null || true) + +# Remove the recursive src folder while keeping src itself (non-recursive) +# and src/retained +node -e " + const fs = require('fs'); + const config = JSON.parse(fs.readFileSync('$DEP01_CONFIG', 'utf8')); + config.sources = ['src', 'src/retained']; + fs.writeFileSync('$DEP01_CONFIG', JSON.stringify(config, null, 2) + '\n'); +" +if ! wait_for_pattern_count rewatch.log "Finished .*compilation" \ + $((completed_builds + 1)) 30; then + error "No rebuild after the configuration change" + cat rewatch.log + exit_watcher + cleanup + exit 1 +fi + +echo 'let value = 2' > "$RETAINED/Retained.res" +if wait_for_retained_value "= 2"; then + success "Edit in the retained folder was compiled" +else + error "Edit in the retained folder was not picked up" + cat rewatch.log + exit_watcher + cleanup + exit 1 +fi + +if ! exit_watcher; then + cleanup + exit 1 +fi +cleanup + +# Rebuild to regenerate the outputs removed by `rewatch clean` +rewatch build > /dev/null 2>&1 + +if git diff --exit-code . > /dev/null 2>&1 && [ -z "$(git ls-files --others --exclude-standard .)" ]; +then + success "No leftover changes" +else + error "Leftover changes detected" + git diff . + git ls-files --others --exclude-standard . + exit 1 +fi