From 42b9b7b62cd0664871030f95de70c91357204734 Mon Sep 17 00:00:00 2001 From: Kai M Date: Tue, 6 Dec 2022 09:11:23 +0100 Subject: [PATCH] install: fix issue #3814 (#3950) * install: fix installing one file when using -Dt options * install: fix installing multiple files with -Dt Code was missing the logic to create the target dir when multiple files should be copied and target dir is given by -t option. This simplifies the copy logic also when only one file should be copied to the target dir. * install: fix verbose output when using -D Also adds a unit test to verify the same behaviour as the gnu tools. * install: add more testcases for create leading dir Tests various combinations of "-D" with and w/o "-t" when installing either a single file or multiple files into a non existing directory. install -D file1 file2 (-t) not_existing_dir install -D file1 (-t) not_existing_dir/ Also fixes file formatting, spelling and adds some more test asserts. * install: fix error for nonex. dir path ending on / The install command failed with a different error message than the original GNU install tool. Checking for a trailing slash fixes this. Only works on unix though. * install: add windows support when checking for '/' * install.rs: fix spelling * install.rs: add more tests regarding omitting dir This increases the CI test coverage and also checks for more corner cases to ensure uu_install is compliant with GNU's original. export C=coreutils/target/debug/ rm -rf dir1 no-dir2 dir3 file mkdir -p dir1 dir3 touch file ${C}install dir1/file1 dir1/.. no-dir2 ${C}install dir1/file1 dir1/.. dir3 ${C}install dir1/.. dir3 * install: improve test_install_missing_arguments Also check that install returns the correct error messages, when only a target directory is given via -t and that is is not created (-D option). * install: rework the checks for missing file args This ensures correct (GNU install like) behavior. Tests from the last commit will pass now. --- src/uu/install/src/install.rs | 84 +++++++++------ tests/by-util/test_install.rs | 185 +++++++++++++++++++++++++++++++--- 2 files changed, 227 insertions(+), 42 deletions(-) diff --git a/src/uu/install/src/install.rs b/src/uu/install/src/install.rs index 4cf620e73..db9fe7d3c 100644 --- a/src/uu/install/src/install.rs +++ b/src/uu/install/src/install.rs @@ -27,7 +27,9 @@ use std::fmt::{Debug, Display}; use std::fs; use std::fs::File; use std::os::unix::fs::MetadataExt; -use std::path::{Path, PathBuf}; +#[cfg(unix)] +use std::os::unix::prelude::OsStrExt; +use std::path::{Path, PathBuf, MAIN_SEPARATOR}; use std::process; const DEFAULT_MODE: u32 = 0o755; @@ -480,40 +482,73 @@ fn is_new_file_path(path: &Path) -> bool { || path.parent().unwrap().as_os_str().is_empty()) // In case of a simple file } +/// Test if the path is an existing directory or ends with a trailing separator. +/// +/// Returns true, if one of the conditions above is met; else false. +/// +#[cfg(unix)] +fn is_potential_directory_path(path: &Path) -> bool { + let separator = MAIN_SEPARATOR as u8; + path.as_os_str().as_bytes().last() == Some(&separator) || path.is_dir() +} + +#[cfg(not(unix))] +fn is_potential_directory_path(path: &Path) -> bool { + let path_str = path.to_string_lossy(); + path_str.ends_with(MAIN_SEPARATOR) || path_str.ends_with('/') || path.is_dir() +} + /// Perform an install, given a list of paths and behavior. /// /// Returns a Result type with the Err variant containing the error message. /// fn standard(mut paths: Vec, b: &Behavior) -> UResult<()> { + // first check that paths contains at least one element + if paths.is_empty() { + return Err(UUsageError::new(1, "missing file operand")); + } + + // get the target from either "-t foo" param or from the last given paths argument let target: PathBuf = if let Some(path) = &b.target_dir { path.into() } else { - paths - .pop() - .ok_or_else(|| UUsageError::new(1, "missing file operand"))? - .into() + let last_path: PathBuf = paths.pop().unwrap().into(); + + // paths has to contain more elements + if paths.is_empty() { + return Err(UUsageError::new( + 1, + format!( + "missing destination file operand after '{}'", + last_path.to_str().unwrap() + ), + )); + } + + last_path }; let sources = &paths.iter().map(PathBuf::from).collect::>(); - if sources.len() > 1 || (target.exists() && target.is_dir()) { - copy_files_into_dir(sources, &target, b) - } else { + if b.create_leading { // if -t is used in combination with -D, create whole target because it does not include filename - let to_create: Option<&Path> = if b.target_dir.is_some() && b.create_leading { + let to_create: Option<&Path> = if b.target_dir.is_some() { Some(target.as_path()) - } else { + // if source and target are filenames used in combination with -D, create target's parent + } else if !(sources.len() > 1 || is_potential_directory_path(&target)) { target.parent() + } else { + None }; if let Some(to_create) = to_create { - if !to_create.exists() && b.create_leading { + if !to_create.exists() { if b.verbose { let mut result = PathBuf::new(); // When creating directories with -Dv, show directory creations step by step for part in to_create.components() { result.push(part.as_os_str()); - if !Path::new(part.as_os_str()).is_dir() { + if !result.is_dir() { // Don't display when the directory already exists println!("install: creating directory {}", result.quote()); } @@ -531,25 +566,16 @@ fn standard(mut paths: Vec, b: &Behavior) -> UResult<()> { } } } + } - let source = sources.first().ok_or_else(|| { - UUsageError::new( - 1, - format!( - "missing destination file operand after '{}'", - target.to_str().unwrap() - ), - ) - })?; + if sources.len() > 1 || is_potential_directory_path(&target) { + copy_files_into_dir(sources, &target, b) + } else { + let source = sources.first().unwrap(); - // If the -D flag was passed (target does not include filename), - // we need to add the source name to the target_dir - // because `copy` expects `to` to be a file, not a directory - let target = if target.is_dir() && b.create_leading { - target.join(source) - } else { - target // already includes dest filename - }; + if source.is_dir() { + return Err(InstallError::OmittingDirectory(source.to_path_buf()).into()); + } if target.is_file() || is_new_file_path(&target) { copy(source, &target, b) diff --git a/tests/by-util/test_install.rs b/tests/by-util/test_install.rs index 7598382a6..3543d2628 100644 --- a/tests/by-util/test_install.rs +++ b/tests/by-util/test_install.rs @@ -1,4 +1,4 @@ -// spell-checker:ignore (words) helloworld objdump n'source +// spell-checker:ignore (words) helloworld nodir objdump n'source use crate::common::util::*; use filetime::FileTime; @@ -427,18 +427,42 @@ fn test_install_nested_paths_copy_file() { #[test] fn test_install_failing_omitting_directory() { - let (at, mut ucmd) = at_and_ucmd!(); - let file1 = "source_file"; - let dir1 = "source_dir"; - let dir2 = "target_dir"; + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + let file1 = "file1"; + let dir1 = "dir1"; + let no_dir2 = "no-dir2"; + let dir3 = "dir3"; at.mkdir(dir1); - at.mkdir(dir2); + at.mkdir(dir3); at.touch(file1); - ucmd.arg(dir1) + // GNU install checks for existing target dir first before checking on source params + scene + .ucmd() .arg(file1) - .arg(dir2) + .arg(dir1) + .arg(no_dir2) + .fails() + .stderr_contains("is not a directory"); + + // file1 will be copied before install fails on dir1 + scene + .ucmd() + .arg(file1) + .arg(dir1) + .arg(dir3) + .fails() + .code_is(1) + .stderr_contains("omitting directory"); + assert!(at.file_exists(&format!("{}/{}", dir3, file1))); + + // install also fails, when only one source param is given + scene + .ucmd() + .arg(dir1) + .arg(dir3) .fails() .code_is(1) .stderr_contains("omitting directory"); @@ -668,6 +692,106 @@ fn test_install_creating_leading_dirs() { .arg(at.plus(target)) .succeeds() .no_stderr(); + + assert!(at.file_exists(target)); +} + +#[test] +fn test_install_creating_leading_dirs_verbose() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + let source = "create_leading_test_file"; + let target = "dir1/no-dir2/no-dir3/test_file"; + + at.touch(source); + at.mkdir("dir1"); + + let creating_dir1 = regex::Regex::new("(?m)^install: creating directory.*dir1'$").unwrap(); + let creating_nodir23 = + regex::Regex::new(r"(?m)^install: creating directory.*no-dir[23]'$").unwrap(); + + scene + .ucmd() + .arg("-Dv") + .arg(source) + .arg(at.plus(target)) + .succeeds() + .stdout_matches(&creating_nodir23) + .stdout_does_not_match(&creating_dir1) + .no_stderr(); + + assert!(at.file_exists(target)); +} + +#[test] +fn test_install_creating_leading_dirs_with_single_source_and_target_dir() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + let source1 = "source_file_1"; + let target_dir = "missing_target_dir/"; + + at.touch(source1); + + // installing a single file into a missing directory will fail, when -D is used w/o -t parameter + scene + .ucmd() + .arg("-D") + .arg(source1) + .arg(at.plus(target_dir)) + .fails() + .stderr_contains("missing_target_dir/' is not a directory"); + + assert!(!at.dir_exists(target_dir)); + + scene + .ucmd() + .arg("-D") + .arg(source1) + .arg("-t") + .arg(at.plus(target_dir)) + .succeeds() + .no_stderr(); + + assert!(at.file_exists(&format!("{target_dir}/{source1}"))); +} + +#[test] +fn test_install_creating_leading_dirs_with_multiple_sources_and_target_dir() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + let source1 = "source_file_1"; + let source2 = "source_file_2"; + let target_dir = "missing_target_dir"; + + at.touch(source1); + at.touch(source2); + + // installing multiple files into a missing directory will fail, when -D is used w/o -t parameter + scene + .ucmd() + .arg("-D") + .arg(source1) + .arg(source2) + .arg(at.plus(target_dir)) + .fails() + .stderr_contains("missing_target_dir' is not a directory"); + + assert!(!at.dir_exists(target_dir)); + + scene + .ucmd() + .arg("-D") + .arg(source1) + .arg(source2) + .arg("-t") + .arg(at.plus(target_dir)) + .succeeds() + .no_stderr(); + + assert!(at.dir_exists(target_dir)); } #[test] @@ -1125,11 +1249,25 @@ fn test_install_backup_off() { #[test] fn test_install_missing_arguments() { let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + let no_target_dir = "no-target_dir"; scene .ucmd() .fails() - .stderr_contains("install: missing file operand"); + .code_is(1) + .stderr_contains("install: missing file operand") + .stderr_contains("install --help' for more information."); + + scene + .ucmd() + .arg("-D") + .arg(format!("-t {}", no_target_dir)) + .fails() + .stderr_contains("install: missing file operand") + .stderr_contains("install --help' for more information."); + assert!(!at.dir_exists(no_target_dir)); } #[test] @@ -1138,12 +1276,33 @@ fn test_install_missing_destination() { let at = &scene.fixtures; let file_1 = "source_file1"; + let dir_1 = "source_dir1"; at.touch(file_1); - scene.ucmd().arg(file_1).fails().stderr_contains(format!( - "install: missing destination file operand after '{}'", - file_1 - )); + at.mkdir(dir_1); + + // will fail and also print some info on correct usage + scene + .ucmd() + .arg(file_1) + .fails() + .stderr_contains(format!( + "install: missing destination file operand after '{}'", + file_1 + )) + .stderr_contains("install --help' for more information."); + + // GNU's install will check for correct num of arguments and then fail + // and it does not recognize, that the source is not a file but a directory. + scene + .ucmd() + .arg(dir_1) + .fails() + .stderr_contains(format!( + "install: missing destination file operand after '{}'", + dir_1 + )) + .stderr_contains("install --help' for more information."); } #[test]