Various small fixes (#853)

* Fallback to provided configuration path if canonicalisation fails

Resolves #823

* Correct misleading logging from configuration sources

Resolves #825

* Use tempfile::tempfile to ensure clean-up

`tempfile::NamedTempFile` cleans up when dropped, but this can be
circumvented if the destructor never runs (e.g., receiving a SIGINT
signal). `tempfile::tempfile`, however, is automatically removed by the
OS when its last file handle is closed.

Note: This required re-implementing `OutputFile::persist` to copy the
buffer contents from the staging file to the actual output.

Resolves #843

* Flush before rewinding staged buffer

* Update CHANGELOG

CHANGELOG.md

@@ -67,6 +67,7 @@ This name should be decided amongst the team before the release.
 - [#822](https://github.com/tweag/topiary/pull/822) Various Bash fixes and improvements
 - [#813](https://github.com/tweag/topiary/pull/813) In-place writing on Windows (also introduced a minimal Windows CI)
 - [#826](https://github.com/tweag/topiary/pull/826) Various Tree-sitter query fixes, thanks to @mkatychev
+- [#853](https://github.com/tweag/topiary/pull/853) Small fixes to CLI logging and IO
 
 ## v0.5.1 - Fragrant Frangipani - 2024-10-22
 

topiary-cli/src/io.rs

@@ -2,11 +2,11 @@ use std::{
     ffi::OsString,
     fmt::{self, Display},
     fs::File,
-    io::{stdin, stdout, ErrorKind, Read, Result, Write},
-    path::{Path, PathBuf},
+    io::{self, Read, Result, Seek, Write},
+    path::PathBuf,
 };
 
-use tempfile::NamedTempFile;
+use tempfile::tempfile;
 use topiary_config::Configuration;
 use topiary_core::{Language, TopiaryQuery};
 
@@ -152,7 +152,7 @@ impl InputFile<'_> {
 impl Read for InputFile<'_> {
     fn read(&mut self, buf: &mut [u8]) -> Result<usize> {
         match &mut self.source {
-            InputSource::Stdin => stdin().lock().read(buf),
+            InputSource::Stdin => io::stdin().lock().read(buf),
 
             InputSource::Disk(path, fd) => {
                 if fd.is_none() {
@@ -246,7 +246,7 @@ pub enum OutputFile {
     Disk {
         // NOTE We stage to a file, rather than writing
         // to memory (e.g., Vec<u8>), to ensure atomicity
-        staged: NamedTempFile,
+        staged: File,
         output: OsString,
     },
 }
@@ -255,29 +255,25 @@ impl OutputFile {
     pub fn new(path: &str) -> CLIResult<Self> {
         match path {
             "-" => Ok(Self::Stdout),
-            file => {
-                // `canonicalize` if the given path exists, otherwise fallback to what was given
-                let path = Path::new(file).canonicalize().or_else(|e| match e.kind() {
-                    ErrorKind::NotFound => Ok(file.into()),
-                    _ => Err(e),
-                })?;
-
-                // The call to `parent` will only return `None` if `path` is the root directory,
-                // but that doesn't make sense as an output file, so unwrapping is safe
-                let parent = path.parent().unwrap();
-
-                Ok(Self::Disk {
-                    staged: NamedTempFile::new_in(parent)?,
-                    output: file.into(),
-                })
-            }
+            file => Ok(Self::Disk {
+                staged: tempfile()?,
+                output: file.into(),
+            }),
         }
     }
 
     // This function must be called to persist the output to disk
     pub fn persist(self) -> CLIResult<()> {
-        if let Self::Disk { staged, output } = self {
-            staged.persist(output)?;
+        if let Self::Disk { mut staged, output } = self {
+            // Rewind to the beginning of the staged output
+            staged.flush()?;
+            staged.rewind()?;
+
+            // Open the actual output for writing and copy the staged contents
+            let mut writer = File::create(&output)?;
+            let bytes = io::copy(&mut staged, &mut writer)?;
+
+            log::debug!("Wrote {bytes} bytes to {}", &output.to_string_lossy());
         }
 
         Ok(())
@@ -296,14 +292,14 @@ impl fmt::Display for OutputFile {
 impl Write for OutputFile {
     fn write(&mut self, buf: &[u8]) -> Result<usize> {
         match self {
-            Self::Stdout => stdout().lock().write(buf),
+            Self::Stdout => io::stdout().lock().write(buf),
             Self::Disk { staged, .. } => staged.write(buf),
         }
     }
 
     fn flush(&mut self) -> Result<()> {
         match self {
-            Self::Stdout => stdout().lock().flush(),
+            Self::Stdout => io::stdout().lock().flush(),
             Self::Disk { staged, .. } => staged.flush(),
         }
     }

topiary-config/src/source.rs

@@ -29,16 +29,21 @@ impl Source {
     /// 4. Built-in configuration (per `Self::builtin_nickel()`)
     pub fn fetch_all(file: &Option<PathBuf>) -> Vec<Self> {
         let candidates = [
-            Some(find_os_configuration_dir_config()),
-            find_workspace_configuration_dir_config(),
-            file.clone(),
+            ("OS", Some(find_os_configuration_dir_config())),
+            ("workspace", find_workspace_configuration_dir_config()),
+            ("CLI", file.clone()),
         ];
 
         // We always include the built-in configuration, as a fallback
+        log::info!("Adding built-in configuration to merge");
         let mut res: Vec<Self> = vec![Self::Builtin];
 
-        for candidate in candidates {
+        for (hint, candidate) in candidates {
             if let Some(path) = Self::find(&candidate) {
+                log::info!(
+                    "Adding {hint}-specified configuration to merge: {}",
+                    path.to_string_lossy()
+                );
                 res.push(Self::File(path));
             }
         }
@@ -54,25 +59,21 @@ impl Source {
     /// 3. `~/.config/topiary/languages.ncl` (or equivalent)
     /// 4. Built-in configuration (per `Self::builtin_nickel()`)
     pub fn fetch_one(file: &Option<PathBuf>) -> Self {
-        let cli_specified = Self::find(&file.clone()).map(Self::File);
-        let workspace_specified =
-            Self::find(&find_workspace_configuration_dir_config()).map(Self::File);
-        let os_config_specified =
-            Self::find(&Some(find_os_configuration_dir_config())).map(Self::File);
-
-        if let Some(res) = cli_specified {
-            log::info!("Using CLI-specified configuration: {res}");
-            res
-        } else if let Some(res) = workspace_specified {
-            log::info!("Using workspace-specified configuration: {res}");
-            res
-        } else if let Some(res) = os_config_specified {
-            log::info!("Using global os-specified configuration: {res}");
-            res
-        } else {
-            log::info!("Using built-in configuration");
-            Self::Builtin
+        let candidates = [
+            ("CLI", file.clone()),
+            ("workspace", find_workspace_configuration_dir_config()),
+            ("OS", Some(find_os_configuration_dir_config())),
+        ];
+
+        for (hint, candidate) in candidates {
+            if let Some(source) = Self::find(&candidate).map(Self::File) {
+                log::info!("Using {hint}-specified configuration: {source}");
+                return source;
+            }
         }
+
+        log::info!("Using built-in configuration");
+        Self::Builtin
     }
 
     /// Attempts to find a configuration file, given a `path` parameter. If `path` is `None`, then
@@ -95,7 +96,7 @@ impl Source {
                     Some(candidate)
                 } else {
                     log::info!(
-                        "Could not find configuration file: {}. Defaulting to built-in configuration.",
+                        "Could not find configuration file: {}.",
                         candidate.to_string_lossy()
                     );
                     None
@@ -124,9 +125,13 @@ impl fmt::Display for Source {
             Self::Builtin => write!(f, "Built-in configuration"),
 
             Self::File(path) => {
-                // We only stringify the path when we know it exists, so the call to `canonicalize`
-                // is safe to unwrap. (All bets are off, if called from elsewhere.)
-                write!(f, "{}", path.canonicalize().unwrap().to_string_lossy())
+                // If the configuration is provided through a file, then we know by this point that
+                // it must exist and so the call to `canonicalize` will succeed. However, special
+                // cases -- such as process substitution, which creates a temporary FIFO -- may
+                // fail if the shell has cleaned things up from under us; in which case, we
+                // fallback to the original `path`.
+                let config = path.canonicalize().unwrap_or(path.clone());
+                write!(f, "{}", config.to_string_lossy())
             }
         }
     }