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 filesystem errors to "false".
When used in restricted, networked, or other
"interesting" filesystems, like many build environs,
this can lose important error data instead of passing it on.
Try to improve error messaging via propagating errors.
  • Loading branch information
workingjubilee committed Jul 5, 2023
1 parent 8f12798 commit 922addb
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 922addb

Please sign in to comment.