From 6a7b6ccdbe2a048102b2998a29899d26e648622d Mon Sep 17 00:00:00 2001 From: Jan Scheer Date: Mon, 23 May 2022 00:51:02 +0200 Subject: [PATCH] tail: add test_follow_name_move_retry * add fixes to pass test_follow_name_move_retry * fix test_follow_name_remove * bump notify to 5.0.0-pre.15 * adjust PollWatcher::with_delay -> PollWatcher::with_config --- Cargo.lock | 5 +- src/uu/tail/Cargo.toml | 4 +- src/uu/tail/src/platform/unix.rs | 6 +-- src/uu/tail/src/tail.rs | 66 ++++++++++++----------- tests/by-util/test_tail.rs | 91 +++++++++++++++++++++++++++++--- 5 files changed, 127 insertions(+), 45 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f3892fa3b..a6d7a8163 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1221,8 +1221,9 @@ dependencies = [ [[package]] name = "notify" -version = "5.0.0-pre.14" -source = "git+https://github.com/notify-rs/notify#8399e4195b31f6f188109363292ed220226146f4" +version = "5.0.0-pre.15" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "553f9844ad0b0824605c20fb55a661679782680410abfb1a8144c2e7e437e7a7" dependencies = [ "bitflags", "crossbeam-channel", diff --git a/src/uu/tail/Cargo.toml b/src/uu/tail/Cargo.toml index d4d9c5d5e..000a1d697 100644 --- a/src/uu/tail/Cargo.toml +++ b/src/uu/tail/Cargo.toml @@ -18,8 +18,8 @@ path = "src/tail.rs" [dependencies] clap = { version = "3.1", features = ["wrap_help", "cargo"] } libc = "0.2.126" -# notify = { version = "5.0.0-pre.14", features=["macos_kqueue"]} -notify = { git = "https://github.com/notify-rs/notify", features=["macos_kqueue"]} +notify = { version = "5.0.0-pre.15", features=["macos_kqueue"]} +# notify = { git = "https://github.com/notify-rs/notify", features=["macos_kqueue"]} uucore = { version=">=0.0.11", package="uucore", path="../../uucore", features=["ringbuffer", "lines"] } [target.'cfg(windows)'.dependencies] diff --git a/src/uu/tail/src/platform/unix.rs b/src/uu/tail/src/platform/unix.rs index 8d1af2a3d..489323c3d 100644 --- a/src/uu/tail/src/platform/unix.rs +++ b/src/uu/tail/src/platform/unix.rs @@ -8,8 +8,8 @@ * file that was distributed with this source code. */ -// spell-checker:ignore (ToDO) errno EPERM ENOSYS -// spell-checker:ignore (options) GETFD +// spell-checker:ignore (ToDO) stdlib +// spell-checker:ignore (options) GETFD EPERM ENOSYS use std::io::{stdin, Error}; @@ -67,7 +67,7 @@ pub fn stdin_is_pipe_or_fifo() -> bool { // FIXME: Detect a closed file descriptor, e.g.: `tail <&-` pub fn stdin_is_bad_fd() -> bool { let fd = stdin().as_raw_fd(); - // this is never `true`, even with `<&-` because stdlib is reopening fds as /dev/null + // this is never `true`, even with `<&-` because Rust's stdlib is reopening fds as /dev/null // see also: https://github.com/uutils/coreutils/issues/2873 // (gnu/tests/tail-2/follow-stdin.sh fails because of this) unsafe { libc::fcntl(fd, libc::F_GETFD) == -1 && get_errno() == libc::EBADF } diff --git a/src/uu/tail/src/tail.rs b/src/uu/tail/src/tail.rs index fcb5d4975..98604c57a 100644 --- a/src/uu/tail/src/tail.rs +++ b/src/uu/tail/src/tail.rs @@ -445,7 +445,11 @@ fn uu_tail(mut settings: Settings) -> UResult<()> { } } - // dbg!(files.map.is_empty(), files.files_remaining(), files.only_stdin_remaining()); + // dbg!( + // files.map.is_empty(), + // files.files_remaining(), + // files.only_stdin_remaining() + // ); // for k in files.map.keys() { // dbg!(k); // } @@ -650,7 +654,11 @@ fn follow(files: &mut FileHandling, settings: &mut Settings) -> UResult<()> { let mut watcher: Box; if settings.use_polling || RecommendedWatcher::kind() == WatcherKind::PollWatcher { - watcher = Box::new(notify::PollWatcher::with_delay(tx, settings.sleep_sec).unwrap()); + let config = notify::poll::PollWatcherConfig { + poll_interval: settings.sleep_sec, + ..Default::default() + }; + watcher = Box::new(notify::PollWatcher::with_config(tx, config).unwrap()); } else { let tx_clone = tx.clone(); match notify::RecommendedWatcher::new(tx) { @@ -664,9 +672,11 @@ fn follow(files: &mut FileHandling, settings: &mut Settings) -> UResult<()> { text::BACKEND ); settings.return_code = 1; - watcher = Box::new( - notify::PollWatcher::with_delay(tx_clone, settings.sleep_sec).unwrap(), - ); + let config = notify::poll::PollWatcherConfig { + poll_interval: settings.sleep_sec, + ..Default::default() + }; + watcher = Box::new(notify::PollWatcher::with_config(tx_clone, config).unwrap()); } Err(e) => panic!("called `Result::unwrap()` on an `Err` value: {:?}", &e), }; @@ -679,6 +689,8 @@ fn follow(files: &mut FileHandling, settings: &mut Settings) -> UResult<()> { let mut orphans = Vec::new(); for path in files.map.keys() { if path.is_tailable() { + // TODO: [2021-10; jhscheer] also add `file` to please inotify-rotate-resourced.sh + // because it is looking for 2x inotify_add_watch and 1x inotify_rm_watch let path = get_path(path, settings); watcher .watch(&path.canonicalize().unwrap(), RecursiveMode::NonRecursive) @@ -686,7 +698,6 @@ fn follow(files: &mut FileHandling, settings: &mut Settings) -> UResult<()> { } else if settings.follow.is_some() && settings.retry { if path.is_orphan() { orphans.push(path.to_path_buf()); - // TODO: [2021-10; jhscheer] add test for this } else { let parent = path.parent().unwrap(); watcher @@ -715,7 +726,6 @@ fn follow(files: &mut FileHandling, settings: &mut Settings) -> UResult<()> { if new_path.exists() { let display_name = files.map.get(new_path).unwrap().display_name.to_path_buf(); if new_path.is_file() && files.map.get(new_path).unwrap().metadata.is_none() { - // TODO: [2021-10; jhscheer] add test for this show_error!("{} has appeared; following new file", display_name.quote()); if let Ok(new_path_canonical) = new_path.canonicalize() { files.update_metadata(&new_path_canonical, None); @@ -775,31 +785,29 @@ fn follow(files: &mut FileHandling, settings: &mut Settings) -> UResult<()> { } match rx_result { Ok(Ok(event)) => { - // eprintln!("=={:=>3}===========================", _event_counter); + // eprintln!("=={:=>3}=====================dbg===", _event_counter); // dbg!(&event); // dbg!(files.map.keys()); - // eprintln!("=={:=>3}===========================", _event_counter); + // eprintln!("=={:=>3}=====================dbg===", _event_counter); handle_event(&event, files, settings, &mut watcher, &mut orphans); } Ok(Err(notify::Error { kind: notify::ErrorKind::Io(ref e), paths, })) if e.kind() == std::io::ErrorKind::NotFound => { - // TODO: [2021-10; jhscheer] is this case still needed ? if let Some(event_path) = paths.first() { if files.map.contains_key(event_path) { - // TODO: [2021-10; jhscheer] handle this case for --follow=name --retry let _ = watcher.unwatch(event_path); - // TODO: [2021-10; jhscheer] add test for this - show_error!( - "{}: {}", - files.map.get(event_path).unwrap().display_name.display(), - text::NO_SUCH_FILE - ); - if !files.files_remaining() && !settings.retry { - // TODO: [2021-10; jhscheer] add test for this - crash!(1, "{}", text::NO_FILES_REMAINING); - } + // TODO: [2021-10; jhscheer] still needed? if yes, add test for this: + // show_error!( + // "{}: {}", + // files.map.get(event_path).unwrap().display_name.display(), + // text::NO_SUCH_FILE + // ); + // TODO: [2021-10; jhscheer] still needed? if yes, add test for this: + // if !files.files_remaining() && !settings.retry { + // crash!(1, "{}", text::NO_FILES_REMAINING); + // } } } } @@ -944,7 +952,9 @@ fn handle_event( } } } - EventKind::Remove(RemoveKind::File) | EventKind::Remove(RemoveKind::Any) => { + EventKind::Remove(RemoveKind::File) | EventKind::Remove(RemoveKind::Any) + // | EventKind::Modify(ModifyKind::Name(RenameMode::Any)) + | EventKind::Modify(ModifyKind::Name(RenameMode::From)) => { if settings.follow == Some(FollowMode::Name) { if settings.retry { if let Some(old_md) = &files.map.get(event_path).unwrap().metadata { @@ -979,7 +989,7 @@ fn handle_event( ); } else { show_error!("{}: {}", display_name.display(), text::NO_SUCH_FILE); - if !files.files_remaining() { + if !files.files_remaining() && settings.use_polling { crash!(1, "{}", text::NO_FILES_REMAINING); } } @@ -989,12 +999,9 @@ fn handle_event( files.map.remove(event_path).unwrap(); } } - EventKind::Modify(ModifyKind::Name(RenameMode::Any)) - | EventKind::Modify(ModifyKind::Name(RenameMode::From)) => { - if settings.follow == Some(FollowMode::Name) { - show_error!("{}: {}", display_name.display(), text::NO_SUCH_FILE); - } - } + // EventKind::Modify(ModifyKind::Name(RenameMode::Any)) + // | EventKind::Modify(ModifyKind::Name(RenameMode::From)) => { + // } EventKind::Modify(ModifyKind::Name(RenameMode::Both)) => { // NOTE: For `tail -f a`, keep tracking additions to b after `mv a b` // (gnu/tests/tail-2/descriptor-vs-rename.sh) @@ -1437,7 +1444,6 @@ pub fn stdin_is_pipe_or_fifo() -> bool { } #[cfg(windows)] { - use winapi_util; winapi_util::file::typ(winapi_util::HandleRef::stdin()) .map(|t| t.is_disk() || t.is_pipe()) .unwrap_or(false) diff --git a/tests/by-util/test_tail.rs b/tests/by-util/test_tail.rs index e2ddfb146..a24e22ef1 100644 --- a/tests/by-util/test_tail.rs +++ b/tests/by-util/test_tail.rs @@ -36,6 +36,7 @@ static FOLLOW_NAME_SHORT_EXP: &str = "follow_name_short.expected"; static FOLLOW_NAME_EXP: &str = "follow_name.expected"; #[test] +#[cfg(all(unix, not(target_os = "android")))] // FIXME: fix this test for Android fn test_stdin_default() { new_ucmd!() .pipe_in_fixture(FOOBAR_TXT) @@ -45,6 +46,7 @@ fn test_stdin_default() { } #[test] +#[cfg(all(unix, not(target_os = "android")))] // FIXME: fix this test for Android fn test_stdin_explicit() { new_ucmd!() .pipe_in_fixture(FOOBAR_TXT) @@ -55,7 +57,7 @@ fn test_stdin_explicit() { } #[test] -#[cfg(target_os = "linux")] +#[cfg(all(unix, not(target_os = "android")))] // FIXME: fix this test for Android fn test_stdin_redirect_file() { // $ echo foo > f @@ -389,6 +391,7 @@ fn test_follow_name_multiple() { } #[test] +#[cfg(all(unix, not(target_os = "android")))] // FIXME: fix this test for Android fn test_follow_stdin_pipe() { new_ucmd!() .arg("-f") @@ -493,6 +496,7 @@ fn test_bytes_single() { } #[test] +#[cfg(all(unix, not(target_os = "android")))] // FIXME: fix this test for Android fn test_bytes_stdin() { new_ucmd!() .pipe_in_fixture(FOOBAR_TXT) @@ -741,6 +745,7 @@ fn test_sleep_interval() { /// Test for reading all but the first NUM bytes: `tail -c +3`. #[test] +#[cfg(all(unix, not(target_os = "android")))] // FIXME: fix this test for Android fn test_positive_bytes() { new_ucmd!() .args(&["-c", "+3"]) @@ -751,6 +756,7 @@ fn test_positive_bytes() { /// Test for reading all bytes, specified by `tail -c +0`. #[test] +#[cfg(all(unix, not(target_os = "android")))] // FIXME: fix this test for Android fn test_positive_zero_bytes() { new_ucmd!() .args(&["-c", "+0"]) @@ -761,6 +767,7 @@ fn test_positive_zero_bytes() { /// Test for reading all but the first NUM lines: `tail -n +3`. #[test] +#[cfg(all(unix, not(target_os = "android")))] // FIXME: fix this test for Android fn test_positive_lines() { new_ucmd!() .args(&["-n", "+3"]) @@ -802,6 +809,7 @@ once /// Test for reading all but the first NUM lines: `tail -3`. #[test] +#[cfg(all(unix, not(target_os = "android")))] // FIXME: fix this test for Android fn test_obsolete_syntax_positive_lines() { new_ucmd!() .args(&["-3"]) @@ -812,6 +820,7 @@ fn test_obsolete_syntax_positive_lines() { /// Test for reading all but the first NUM lines: `tail -n -10`. #[test] +#[cfg(all(unix, not(target_os = "android")))] // FIXME: fix this test for Android fn test_small_file() { new_ucmd!() .args(&["-n -10"]) @@ -822,6 +831,7 @@ fn test_small_file() { /// Test for reading all but the first NUM lines: `tail -10`. #[test] +#[cfg(all(unix, not(target_os = "android")))] // FIXME: fix this test for Android fn test_obsolete_syntax_small_file() { new_ucmd!() .args(&["-10"]) @@ -832,6 +842,7 @@ fn test_obsolete_syntax_small_file() { /// Test for reading all lines, specified by `tail -n +0`. #[test] +#[cfg(all(unix, not(target_os = "android")))] // FIXME: fix this test for Android fn test_positive_zero_lines() { new_ucmd!() .args(&["-n", "+0"]) @@ -841,6 +852,7 @@ fn test_positive_zero_lines() { } #[test] +#[cfg(all(unix, not(target_os = "android")))] // FIXME: fix this test for Android fn test_invalid_num() { new_ucmd!() .args(&["-c", "1024R", "emptyfile.txt"]) @@ -878,6 +890,7 @@ fn test_invalid_num() { } #[test] +#[cfg(all(unix, not(target_os = "android")))] // FIXME: fix this test for Android fn test_num_with_undocumented_sign_bytes() { // tail: '-' is not documented (8.32 man pages) // head: '+' is not documented (8.32 man pages) @@ -1425,15 +1438,21 @@ fn test_follow_name_remove() { at.copy(source, source_copy); let expected_stdout = at.read(FOLLOW_NAME_SHORT_EXP); - let expected_stderr = format!( - "{}: {}: No such file or directory\n{0}: no files remaining\n", - ts.util_name, source_copy - ); + let expected_stderr = [ + format!( + "{}: {}: No such file or directory\n{0}: no files remaining\n", + ts.util_name, source_copy + ), + format!( + "{}: {}: No such file or directory\n", + ts.util_name, source_copy + ), + ]; let delay = 2000; let mut args = vec!["--follow=name", source_copy, "--use-polling"]; - for _ in 0..2 { + for i in 0..2 { at.copy(source, source_copy); let mut p = ts.ucmd().set_stdin(Stdio::null()).args(&args).run_no_wait(); @@ -1445,7 +1464,7 @@ fn test_follow_name_remove() { let (buf_stdout, buf_stderr) = take_stdout_stderr(&mut p); assert_eq!(buf_stdout, expected_stdout); - assert_eq!(buf_stderr, expected_stderr); + assert_eq!(buf_stderr, expected_stderr[i]); args.pop(); } @@ -1597,7 +1616,7 @@ fn test_follow_name_move_create() { } #[test] -#[cfg(unix)] +#[cfg(all(unix, not(target_os = "android")))] // FIXME: fix this test for Android fn test_follow_name_move() { // This test triggers a move event while `tail --follow=name logfile` is running. // ((sleep 2 && mv logfile backup &)>/dev/null 2>&1 &) ; tail --follow=name logfile @@ -1638,6 +1657,59 @@ fn test_follow_name_move() { } } +#[test] +#[cfg(all(unix, not(target_os = "android")))] // FIXME: fix this test for Android +fn test_follow_name_move_retry() { + // Similar to test_follow_name_move but with `--retry` (`-F`) + // This test triggers two move/rename events while `tail --follow=name --retry logfile` is running. + + let ts = TestScenario::new(util_name!()); + let at = &ts.fixtures; + + let source = FOLLOW_NAME_TXT; + let backup = "backup"; + + let expected_stderr = format!( + "{0}: '{1}' has become inaccessible: No such file or directory\n\ + {0}: '{1}' has appeared; following new file\n", + ts.util_name, source + ); + let expected_stdout = "tailed\nnew content\n"; + + let mut args = vec!["--follow=name", "--retry", source, "--use-polling"]; + + #[allow(clippy::needless_range_loop)] + for _ in 0..2 { + at.touch(source); + let mut p = ts.ucmd().set_stdin(Stdio::null()).args(&args).run_no_wait(); + + sleep(Duration::from_millis(1000)); + at.append(source, "tailed\n"); + + sleep(Duration::from_millis(2000)); + // with --follow=name, tail should stop monitoring the renamed file + at.rename(source, backup); + sleep(Duration::from_millis(4000)); + + // overwrite backup while it's not monitored + at.truncate(backup, "new content\n"); + sleep(Duration::from_millis(500)); + // move back, tail should pick this up and print new content + at.rename(backup, source); + sleep(Duration::from_millis(4000)); + + p.kill().unwrap(); + + let (buf_stdout, buf_stderr) = take_stdout_stderr(&mut p); + dbg!(&buf_stdout, &buf_stderr); + assert_eq!(buf_stdout, expected_stdout); + assert_eq!(buf_stderr, expected_stderr); + + at.remove(source); + args.pop(); + } +} + #[test] #[cfg(unix)] fn test_follow_inotify_only_regular() { @@ -1685,11 +1757,13 @@ fn test_no_such_file() { } #[test] +#[cfg(all(unix, not(target_os = "android")))] // FIXME: fix this test for Android fn test_no_trailing_newline() { new_ucmd!().pipe_in("x").succeeds().stdout_only("x"); } #[test] +#[cfg(all(unix, not(target_os = "android")))] // FIXME: fix this test for Android fn test_lines_zero_terminated() { new_ucmd!() .args(&["-z", "-n", "2"]) @@ -1704,6 +1778,7 @@ fn test_lines_zero_terminated() { } #[test] +#[cfg(all(unix, not(target_os = "android")))] // FIXME: fix this test for Android fn test_presume_input_pipe_default() { new_ucmd!() .arg("---presume-input-pipe")