Small fixes.

di-calibrate cannot currently write its sky model to an ms -- the help
text has been updated to account for this and a test now exists to check
that the help text is correct.

There were problems around files and directories being overwritten. I've
done the best I can but it looks like we can't ever handle detecting
whether we have permission to remove a directory ahead of time. We may
want to improve an error message associated with attempting to remove a
directory we don't have permission to, if it's not already clear enough.

A message stating where calibrated visibilities have been written to was
lost; this is now back.

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);

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<dyn VisWritable>)> = 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)

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<bool, InvalidArgsError> {
+    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<bool, InvalidArgsError> {
     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)
 }

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]