diff --git a/src/calibrate/args.rs b/src/calibrate/args.rs index 9b3fb98f..a92da3c0 100644 --- a/src/calibrate/args.rs +++ b/src/calibrate/args.rs @@ -53,7 +53,7 @@ lazy_static::lazy_static! { static ref DI_CALIBRATE_OUTPUT_HELP: String = format!("Paths to the calibration output files. Supported calibrated visibility outputs: {}. Supported calibration solution formats: {}. Default: {}", *VIS_OUTPUT_EXTENSIONS, *CAL_SOLUTION_EXTENSIONS, DEFAULT_OUTPUT_SOLUTIONS_FILENAME); - static ref MODEL_FILENAME_HELP: String = format!("The path to the file where the generated sky-model visibilities are written. If this argument isn't supplied, then no file is written. Supported formats: {}", *VIS_OUTPUT_EXTENSIONS); + static ref MODEL_FILENAME_HELP: String = format!("The path to the file where the generated sky-model visibilities are written. If this argument isn't supplied, then no file is written. Supported formats: uvfits"); static ref SOURCE_LIST_TYPE_HELP: String = format!("The type of sky-model source list. Valid types are: {}. If not specified, all types are attempted", *mwa_hyperdrive_srclist::SOURCE_LIST_TYPES_COMMA_SEPARATED); diff --git a/src/calibrate/di/mod.rs b/src/calibrate/di/mod.rs index d989d777..990c6a6b 100644 --- a/src/calibrate/di/mod.rs +++ b/src/calibrate/di/mod.rs @@ -221,6 +221,16 @@ pub(crate) fn di_calibrate( // create a VisWritable for each output vis filename let mut out_writers: Vec<(VisOutputType, Box)> = vec![]; for (vis_type, file) in params.output_vis_filenames.iter() { + // Don't trust the user - delete `file` whether it is a file or a + // directory. Not doing this might make the functions below fail + // because they're expecting one of either a file or a directory. + if file.is_file() { + std::fs::remove_file(file)?; + } else if file.is_dir() { + std::fs::remove_dir_all(file)?; + } + // TODO: Symbolic links. + match vis_type { VisOutputType::Uvfits => { trace!(" - to uvfits {}", file.display()); @@ -354,6 +364,10 @@ pub(crate) fn di_calibrate( .write_uvfits_antenna_table(&obs_context.tile_names, &obs_context.tile_xyzs)?; } } + + for (_, file) in params.output_vis_filenames.iter() { + info!("Calibrated visibilities written to {}", file.display()); + } } Ok(sols) diff --git a/src/calibrate/params/mod.rs b/src/calibrate/params/mod.rs index 90ce8a34..5cb53d47 100644 --- a/src/calibrate/params/mod.rs +++ b/src/calibrate/params/mod.rs @@ -450,10 +450,12 @@ impl CalibrateParams { ext.and_then(|s| VisOutputType::from_str(s).ok()), ) { (Some(sol_type), None) => { + trace!("{} is a solution output", file.display()); can_write_to_file(&file)?; cal_sols.push((sol_type, file)); }, (None, Some(vis_type)) => { + trace!("{} is a visibility output", file.display()); can_write_to_file(&file)?; vis_out.push((vis_type, file)); }, @@ -1348,6 +1350,55 @@ fn range_or_comma_separated(collection: &[usize], prefix: Option<&str>) -> Strin /// only to be unable to write to a file at the end. This code _doesn't_ alter /// the file if it exists. fn can_write_to_file(file: &Path) -> Result<(), InvalidArgsError> { + trace!("Testing whether we can write to {}", file.display()); + + if file.is_dir() { + let exists = can_write_to_dir(file)?; + if exists { + warn!("Will overwrite the existing directory '{}'", file.display()); + } + } else { + let exists = can_write_to_file_inner(file)?; + if exists { + warn!("Will overwrite the existing file '{}'", file.display()); + } + } + + Ok(()) +} + +/// Iterate over all of the files and subdirectories of a directory and test +/// whether we can write to them. Note that testing whether directories are +/// writable is very weak; in my testing, changing a subdirectories owner to +/// root and running this function suggested that the file was writable, but it +/// was not. This appears to be a limitation of operating systems, and there's +/// not even a reliable way of checking if *your* user is able to write to a +/// directory. Files are much more rigorously tested. +fn can_write_to_dir(dir: &Path) -> Result { + let exists = dir.exists(); + + let metadata = std::fs::metadata(dir)?; + let permissions = metadata.permissions(); + if permissions.readonly() { + return Err(InvalidArgsError::FileNotWritable { + file: dir.display().to_string(), + }); + } + + // Test whether every single entry in `dir` is writable. + for entry in std::fs::read_dir(dir)? { + let entry = entry?.path(); + if entry.is_file() { + can_write_to_file_inner(&entry)?; + } else if entry.is_dir() { + can_write_to_dir(&entry)?; + } + } + + Ok(exists) +} + +fn can_write_to_file_inner(file: &Path) -> Result { let file_exists = file.exists(); match OpenOptions::new() @@ -1358,10 +1409,10 @@ fn can_write_to_file(file: &Path) -> Result<(), InvalidArgsError> { { // File is writable. Ok(_) => { - if file_exists { - warn!("Will overwrite the existing file '{}'", file.display()) - } else { - // file didn't exist before, remove. + // If the file in question didn't already exist, `OpenOptions::new` + // creates it as part of its work. We don't want to keep the 0-sized + // file; remove it if it didn't exist before. + if !file_exists { std::fs::remove_file(file).map_err(InvalidArgsError::IO)?; } } @@ -1395,5 +1446,5 @@ fn can_write_to_file(file: &Path) -> Result<(), InvalidArgsError> { } } - Ok(()) + Ok(file_exists) } diff --git a/tests/integration/calibrate/cli_args.rs b/tests/integration/calibrate/cli_args.rs index 25003d79..b4d20957 100644 --- a/tests/integration/calibrate/cli_args.rs +++ b/tests/integration/calibrate/cli_args.rs @@ -4,31 +4,104 @@ //! Tests against the command-line interface for calibration. +use itertools::Itertools; + use approx::assert_abs_diff_eq; use mwa_hyperdrive_common::{ clap::{ErrorKind::WrongNumberOfValues, Parser}, + itertools, marlu::constants::{MWA_HEIGHT_M, MWA_LAT_DEG, MWA_LONG_DEG}, }; use crate::*; #[test] -fn test_calibrate_help_is_correct() { +fn test_hyperdrive_help_is_correct() { + let mut stdouts = vec![]; + // First with --help let cmd = hyperdrive().arg("--help").ok(); assert!(cmd.is_ok()); let (stdout, stderr) = get_cmd_output(cmd); assert!(stderr.is_empty()); - assert!(stdout.contains("di-calibrate")); - assert!(stdout.contains("Perform direction-independent calibration on the input MWA data")); + stdouts.push(stdout); - // Second with -h + // Then with -h let cmd = hyperdrive().arg("-h").ok(); assert!(cmd.is_ok()); let (stdout, stderr) = get_cmd_output(cmd); assert!(stderr.is_empty()); - assert!(stdout.contains("di-calibrate")); - assert!(stdout.contains("Perform direction-independent calibration on the input MWA data")); + stdouts.push(stdout); + + for stdout in stdouts { + assert!(stdout.contains("di-calibrate")); + assert!(stdout.contains("Perform direction-independent calibration on the input MWA data")); + } +} + +#[test] +fn test_di_calibrate_help_is_correct() { + let mut stdouts = vec![]; + + // First with --help + let cmd = hyperdrive().args(["di-calibrate", "--help"]).ok(); + assert!(cmd.is_ok()); + let (stdout, stderr) = get_cmd_output(cmd); + assert!(stderr.is_empty()); + stdouts.push(stdout); + + // Then with -h + let cmd = hyperdrive().args(["di-calibrate", "-h"]).ok(); + assert!(cmd.is_ok()); + let (stdout, stderr) = get_cmd_output(cmd); + assert!(stderr.is_empty()); + stdouts.push(stdout); + + for stdout in stdouts { + // Output visibility formats are correctly specified. + let mut iter = stdout.split("\n\n").filter(|s| s.contains("--outputs ")); + let outputs_line = iter.next(); + assert!( + outputs_line.is_some(), + "No lines containing '--outputs ' were found in di-calibrate's help text" + ); + assert!( + iter.next().is_none(), + "More than one '--outputs ' was found; this should not be possible" + ); + let outputs_line = outputs_line.unwrap().split_ascii_whitespace().join(" "); + assert!( + outputs_line.contains("Supported calibrated visibility outputs: uvfits, ms"), + "--outputs did not list expected vis outputs. The line is: {outputs_line}" + ); + assert!( + outputs_line.contains("Supported calibration solution formats: fits, bin"), + "--outputs did not list expected vis outputs. The line is: {outputs_line}" + ); + + let mut iter = stdout + .split("\n\n") + .filter(|s| s.contains("--model-filename ")); + let model_line = iter.next(); + assert!( + model_line.is_some(), + "No lines containing '--model-filename ' were found in di-calibrate's help text" + ); + assert!( + iter.next().is_none(), + "More than one '--model-filename ' was found; this should not be possible" + ); + let model_line = model_line.unwrap().split_ascii_whitespace().join(" "); + assert!( + model_line.contains("Supported formats: uvfits"), + "--model-filename did not list expected vis outputs. The line is: {model_line}" + ); + // TODO: Fix model output to ms + assert!( + !model_line.contains("Supported formats: uvfits, ms"), + "--model-filename did not list expected vis outputs. The line is: {model_line}" + ); + } } #[test]