Skip to content

Commit

Permalink
Try to smartly propagate fs errors
Browse files Browse the repository at this point in the history
Path::exists coerces fs errors to "false".
When used in limited-write fs, like many build environs,
this loses the error data instead of passing it on.
Try to improve error messaging via propagating errors.
  • Loading branch information
workingjubilee committed Jul 4, 2023
1 parent d2e61c1 commit 72bd91b
Show file tree
Hide file tree
Showing 7 changed files with 42 additions and 36 deletions.
2 changes: 1 addition & 1 deletion cargo-pgrx/src/command/init.rs
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ pub(crate) fn init_pgrx(pgrx: &Pgrx, init: &Init) -> eyre::Result<()> {
} else {
let datadir = pg_config.data_dir()?;
let bindir = pg_config.bin_dir()?;
if !datadir.exists() {
if !datadir.try_exists()? {
initdb(&bindir, &datadir)?;
}
}
Expand Down
19 changes: 14 additions & 5 deletions cargo-pgrx/src/command/install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ use cargo_toml::Manifest;
use eyre::{eyre, WrapErr};
use owo_colors::OwoColorize;
use pgrx_pg_config::{cargo::PgrxManifestExt, get_target_dir, PgConfig, Pgrx};
use std::fs;
use std::io::BufReader;
use std::path::{Path, PathBuf};
use std::process::Stdio;
Expand Down Expand Up @@ -197,11 +198,19 @@ fn copy_file(
do_filter: bool,
package_manifest_path: impl AsRef<Path>,
) -> eyre::Result<()> {
if !dest.parent().unwrap().exists() {
std::fs::create_dir_all(dest.parent().unwrap()).wrap_err_with(|| {
format!("failed to create destination directory {}", dest.parent().unwrap().display())
})?;
}
let Some(dest_dir) = dest.parent() else {
// what fresh hell could ever cause such an error?
eyre::bail!("no directory to copy to: {}", dest.display())
};
match dest_dir.try_exists() {
Ok(false) => fs::create_dir_all(dest_dir).wrap_err_with(|| {
format!("failed to create destination directory {}", dest_dir.display())
})?,
Ok(true) => (),
Err(e) => Err(e).wrap_err_with(|| {
format!("failed to access {}, is it write-enabled?", dest_dir.display())
})?,
};

println!("{} {} to {}", " Copying".bold().green(), msg, format_display_path(&dest)?.cyan());

Expand Down
4 changes: 3 additions & 1 deletion cargo-pgrx/src/command/package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,9 @@ pub(crate) fn package_extension(
is_test: bool,
features: &clap_cargo::Features,
) -> eyre::Result<()> {
if !out_dir.exists() {
let out_dir_exists =
out_dir.try_exists().wrap_err("failed to access out-dir while packaging extension")?;
if !out_dir_exists {
std::fs::create_dir_all(&out_dir)?;
}

Expand Down
2 changes: 1 addition & 1 deletion cargo-pgrx/src/command/start.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ pub(crate) fn start_postgres(pg_config: &PgConfig) -> eyre::Result<()> {
let bindir = pg_config.bin_dir()?;
let port = pg_config.port()?;

if !datadir.exists() {
if !datadir.try_exists()? {
initdb(&bindir, &datadir)?;
}

Expand Down
12 changes: 6 additions & 6 deletions cargo-pgrx/src/command/status.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use eyre::eyre;
use owo_colors::OwoColorize;
use pgrx_pg_config::{PgConfig, PgConfigSelector, Pgrx};
use std::path::PathBuf;
use std::process::Stdio;
use std::process::{self, Stdio};

use crate::CommandExecute;

Expand Down Expand Up @@ -58,15 +58,15 @@ impl CommandExecute for Status {
#[tracing::instrument(level = "error", skip_all, fields(pg_version = %pg_config.version()?))]
pub(crate) fn status_postgres(pg_config: &PgConfig) -> eyre::Result<bool> {
let datadir = pg_config.data_dir()?;
let bindir = pg_config.bin_dir()?;

if !datadir.exists() {
if let Ok(false) = datadir.try_exists() {
// Postgres couldn't possibly be running if there's no data directory
// and even if it were, we'd have no way of knowing
return Ok(false);
}
} // if Err, let the filesystem and OS handle our impending failure

let mut command = std::process::Command::new(format!("{}/pg_ctl", bindir.display()));
let mut pg_ctl = pg_config.bin_dir()?;
pg_ctl.push("pg_ctl");
let mut command = process::Command::new(pg_ctl);
command.stdout(Stdio::piped()).stderr(Stdio::piped()).arg("status").arg("-D").arg(&datadir);
let command_str = format!("{:?}", command);
tracing::debug!(command = %command_str, "Running");
Expand Down
16 changes: 8 additions & 8 deletions pgrx-pg-config/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use std::error::Error;
use std::ffi::OsString;
use std::fmt::{self, Debug, Display, Formatter};
use std::io::ErrorKind;
use std::path::{Path, PathBuf};
use std::path::PathBuf;
use std::process::{Command, Stdio};
use std::str::FromStr;
use url::Url;
Expand Down Expand Up @@ -321,7 +321,7 @@ impl PgConfig {
}

pub fn bin_dir(&self) -> eyre::Result<PathBuf> {
Ok(Path::new(&self.run("--bindir")?).to_path_buf())
Ok(self.run("--bindir")?.into())
}

pub fn postmaster_path(&self) -> eyre::Result<PathBuf> {
Expand Down Expand Up @@ -522,13 +522,13 @@ impl Pgrx {
Err(_) => {
// we'll get what we need from cargo-pgrx' config.toml file
let path = Pgrx::config_toml()?;
if !path.exists() {
if !path.try_exists()? {
return Err(eyre!(
"{} not found. Have you run `{}` yet?",
path.display(),
"cargo pgrx init".bold().yellow()
));
}
};

match toml::from_str::<ConfigToml>(&std::fs::read_to_string(&path)?) {
Ok(configs) => {
Expand Down Expand Up @@ -620,10 +620,10 @@ impl Pgrx {
|v| Ok(v.into()),
)?;

if pgrx_home.exists() {
Ok(pgrx_home)
} else {
Err(PgrxHomeError::MissingPgrxHome(pgrx_home))
match pgrx_home.try_exists() {
Ok(true) => Ok(pgrx_home),
Ok(false) => Err(PgrxHomeError::MissingPgrxHome(pgrx_home)),
Err(e) => Err(PgrxHomeError::IoError(e)),
}
}

Expand Down
23 changes: 9 additions & 14 deletions pgrx-pg-sys/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ use pgrx_pg_config::{
};
use quote::{quote, ToTokens};
use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet};
use std::path::PathBuf;
use std::fs;
use std::path::{self, PathBuf}; // disambiguate path::Path and syn::Type::Path
use std::process::{Command, Output};
use syn::{ForeignItem, Item, ItemConst};

Expand Down Expand Up @@ -841,22 +842,16 @@ fn build_shim_for_version(
eprintln!("shim_src={}", shim_src.display());
eprintln!("shim_dst={}", shim_dst.display());

std::fs::create_dir_all(shim_dst).unwrap();
fs::create_dir_all(shim_dst).unwrap();

if !std::path::Path::new(&format!("{}/Makefile", shim_dst.display())).exists() {
std::fs::copy(
format!("{}/Makefile", shim_src.display()),
format!("{}/Makefile", shim_dst.display()),
)
.unwrap();
let makefile_dst = path::Path::join(shim_dst, "./Makefile");
if !makefile_dst.try_exists()? {
fs::copy(path::Path::join(shim_src, "./Makefile"), makefile_dst).unwrap();
}

if !std::path::Path::new(&format!("{}/pgrx-cshim.c", shim_dst.display())).exists() {
std::fs::copy(
format!("{}/pgrx-cshim.c", shim_src.display()),
format!("{}/pgrx-cshim.c", shim_dst.display()),
)
.unwrap();
let cshim_dst = path::Path::join(shim_dst, "./pgrx-cshim.c");
if !cshim_dst.try_exists()? {
fs::copy(path::Path::join(shim_src, "./pgrx-cshim.c"), cshim_dst).unwrap();
}

let make = env_tracked("MAKE").unwrap_or_else(|| "make".to_string());
Expand Down

0 comments on commit 72bd91b

Please sign in to comment.