Skip to content

Commit

Permalink
chore: lots of lints and fixes (#1523)
Browse files Browse the repository at this point in the history 
* it is an anti-pattern for a fn to take a reference, and clone it right
away. If a fn needs to own a value, a value ownership should be passed
in, not cloned inside the fn. This way the caller can decide to clone
something, or to pass it in as is
* use &Path instead of &PathBuf (same as &str instead of &String)
* use .cmp for comparison
* some spelling
* lots of `match` statements can be simplified
* use `.is_null()` instead of comparing with the null ptr
* do not impl ToString, impl Display instead for "free" ToString

---------

Co-authored-by: Jubilee <[email protected]>
  • Loading branch information
@nyurik @workingjubilee
nyurik and workingjubilee authored Feb 7, 2024
1 parent e2c4ff9 commit 12842b6
Show file tree
Hide file tree
Showing 43 changed files with 240 additions and 343 deletions.
2 changes: 1 addition & 1 deletion Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ cargo-pgrx = { path = "cargo-pgrx" }
pgrx-macros = { path = "./pgrx-macros", version = "=0.12.0-alpha.0" }
pgrx-pg-sys = { path = "./pgrx-pg-sys", version = "=0.12.0-alpha.0" }
pgrx-sql-entity-graph = { path = "./pgrx-sql-entity-graph", version = "=0.12.0-alpha.0" }
pgrx-pg-config = { path = "./pgrx-pg-config/", version = "=0.12.0-alpha.0" }
pgrx-pg-config = { path = "./pgrx-pg-config", version = "=0.12.0-alpha.0" }

cargo_toml = "0.16" # used for building projects
eyre = "0.6.9" # simplifies error-handling
Expand Down
50 changes: 19 additions & 31 deletions cargo-pgrx/src/command/init.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use std::collections::HashMap;
use std::fs::File;
use std::io::{Read, Write};
use std::num::NonZeroUsize;
use std::path::PathBuf;
use std::path::{Path, PathBuf};
use std::process::{Command, Stdio};
use std::sync::OnceLock;

Expand Down Expand Up @@ -208,7 +208,7 @@ pub(crate) fn init_pgrx(pgrx: &Pgrx, init: &Init) -> eyre::Result<()> {
output_configs.sort_by(|a, b| {
a.major_version()
.unwrap_or_else(|e| panic!("{e}: could not determine major version for: `{a:?}`"))
.cmp(&b.major_version().ok().expect("could not determine major version"))
.cmp(&b.major_version().expect("could not determine major version"))
});
for pg_config in output_configs.iter() {
validate_pg_config(pg_config)?;
Expand All @@ -231,7 +231,7 @@ pub(crate) fn init_pgrx(pgrx: &Pgrx, init: &Init) -> eyre::Result<()> {
#[tracing::instrument(level = "error", skip_all, fields(pg_version = %pg_config.version()?, pgrx_home))]
fn download_postgres(
pg_config: &PgConfig,
pgrx_home: &PathBuf,
pgrx_home: &Path,
init: &Init,
) -> eyre::Result<PgConfig> {
use crate::command::build_agent_for_url;
Expand Down Expand Up @@ -263,15 +263,10 @@ fn download_postgres(
make_install_postgres(pg_config, &pgdir, init) // returns a new PgConfig object
}

fn untar(
bytes: &[u8],
pgrxdir: &PathBuf,
pg_config: &PgConfig,
init: &Init,
) -> eyre::Result<PathBuf> {
fn untar(bytes: &[u8], pgrxdir: &Path, pg_config: &PgConfig, init: &Init) -> eyre::Result<PathBuf> {
let _token = init.jobserver.get().unwrap().acquire().unwrap();

let mut unpackdir = pgrxdir.clone();
let mut unpackdir = pgrxdir.to_path_buf();
unpackdir.push(&format!("{}_unpack", pg_config.version()?));
if unpackdir.exists() {
// delete everything at this path if it already exists
Expand All @@ -289,7 +284,7 @@ fn untar(
let mut tar_decoder = Archive::new(BzDecoder::new(bytes));
tar_decoder.unpack(&unpackdir)?;

let mut pgdir = pgrxdir.clone();
let mut pgdir = pgrxdir.to_path_buf();
pgdir.push(&pg_config.version()?);
if pgdir.exists() {
// delete everything at this path if it already exists
Expand Down Expand Up @@ -398,11 +393,11 @@ fn fixup_homebrew_for_icu(configure_cmd: &mut Command) {
}
}

fn configure_postgres(pg_config: &PgConfig, pgdir: &PathBuf, init: &Init) -> eyre::Result<()> {
fn configure_postgres(pg_config: &PgConfig, pgdir: &Path, init: &Init) -> eyre::Result<()> {
let _token = init.jobserver.get().unwrap().acquire().unwrap();

println!("{} Postgres v{}", " Configuring".bold().green(), pg_config.version()?);
let mut configure_path = pgdir.clone();
let mut configure_path = pgdir.to_path_buf();
configure_path.push("configure");
let mut command = std::process::Command::new(configure_path);
// Some of these are redundant with `--enable-debug`.
Expand Down Expand Up @@ -454,19 +449,16 @@ fn configure_postgres(pg_config: &PgConfig, pgdir: &PathBuf, init: &Init) -> eyr
if output.status.success() {
Ok(())
} else {
Err(std::io::Error::new(
std::io::ErrorKind::Other,
format!(
"{}\n{}{}",
command_str,
String::from_utf8(output.stdout).unwrap(),
String::from_utf8(output.stderr).unwrap()
),
))?
Err(std::io::Error::other(format!(
"{}\n{}{}",
command_str,
String::from_utf8(output.stdout).unwrap(),
String::from_utf8(output.stderr).unwrap()
)))?
}
}

fn make_postgres(pg_config: &PgConfig, pgdir: &PathBuf, init: &Init) -> eyre::Result<()> {
fn make_postgres(pg_config: &PgConfig, pgdir: &Path, init: &Init) -> eyre::Result<()> {
println!("{} Postgres v{}", " Compiling".bold().green(), pg_config.version()?);
let mut command = std::process::Command::new("make");

Expand Down Expand Up @@ -499,11 +491,7 @@ fn make_postgres(pg_config: &PgConfig, pgdir: &PathBuf, init: &Init) -> eyre::Re
}
}

fn make_install_postgres(
version: &PgConfig,
pgdir: &PathBuf,
init: &Init,
) -> eyre::Result<PgConfig> {
fn make_install_postgres(version: &PgConfig, pgdir: &Path, init: &Init) -> eyre::Result<PgConfig> {
println!(
"{} Postgres v{} to {}",
" Installing".bold().green(),
Expand Down Expand Up @@ -582,8 +570,8 @@ fn write_config(pg_configs: &Vec<PgConfig>, init: &Init) -> eyre::Result<()> {
Ok(())
}

fn get_pg_installdir(pgdir: &PathBuf) -> PathBuf {
let mut dir = PathBuf::from(pgdir);
fn get_pg_installdir(pgdir: &Path) -> PathBuf {
let mut dir = pgdir.to_path_buf();
dir.push("pgrx-install");
dir
}
Expand All @@ -602,7 +590,7 @@ fn is_root_user() -> bool {
false
}

pub(crate) fn initdb(bindir: &PathBuf, datadir: &PathBuf) -> eyre::Result<()> {
pub(crate) fn initdb(bindir: &Path, datadir: &Path) -> eyre::Result<()> {
println!(" {} data directory at {}", "Initializing".bold().green(), datadir.display());
let mut command = std::process::Command::new(format!("{}/initdb", bindir.display()));
command
Expand Down
72 changes: 35 additions & 37 deletions cargo-pgrx/src/command/install.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ pub(crate) fn install_extension(
);
copy_file(
&control_file,
&dest,
dest,
"control file",
true,
&package_manifest_path,
Expand Down Expand Up @@ -206,13 +206,13 @@ pub(crate) fn install_extension(
// process which will mash up all pointers in the .TEXT segment.
// this simulate linux's install(1) behavior
if dest.exists() {
std::fs::remove_file(&dest)
fs::remove_file(&dest)
.wrap_err_with(|| format!("unable to remove existing file {}", dest.display()))?;
}

copy_file(
&shlibpath,
&dest,
dest,
"shared library",
false,
&package_manifest_path,
Expand Down Expand Up @@ -240,8 +240,8 @@ pub(crate) fn install_extension(
}

fn copy_file(
src: &PathBuf,
dest: &PathBuf,
src: &Path,
dest: PathBuf,
msg: &str,
do_filter: bool,
package_manifest_path: impl AsRef<Path>,
Expand All @@ -262,28 +262,28 @@ fn copy_file(
})?,
};

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

if do_filter {
// we want to filter the contents of the file we're to copy
let input = std::fs::read_to_string(src)
let input = fs::read_to_string(src)
.wrap_err_with(|| format!("failed to read `{}`", src.display()))?;
let mut input = filter_contents(package_manifest_path, input)?;

if src.display().to_string().ends_with(".control") {
input = filter_out_fields_in_control(pg_config, input)?;
}

std::fs::write(dest, input).wrap_err_with(|| {
fs::write(&dest, input).wrap_err_with(|| {
format!("failed writing `{}` to `{}`", src.display(), dest.display())
})?;
} else {
std::fs::copy(src, dest).wrap_err_with(|| {
fs::copy(src, &dest).wrap_err_with(|| {
format!("failed copying `{}` to `{}`", src.display(), dest.display())
})?;
}

output_tracking.push(dest.clone());
output_tracking.push(dest);

Ok(())
}
Expand Down Expand Up @@ -346,10 +346,10 @@ pub(crate) fn build_extension(

fn get_target_sql_file(
manifest_path: impl AsRef<Path>,
extdir: &PathBuf,
base_directory: &PathBuf,
extdir: &Path,
base_directory: PathBuf,
) -> eyre::Result<PathBuf> {
let mut dest = base_directory.clone();
let mut dest = base_directory;
dest.push(extdir);

let (_, extname) = find_control_file(&manifest_path)?;
Expand All @@ -367,12 +367,12 @@ fn copy_sql_files(
profile: &CargoProfile,
is_test: bool,
features: &clap_cargo::Features,
extdir: &PathBuf,
base_directory: &PathBuf,
extdir: &Path,
base_directory: &Path,
skip_build: bool,
output_tracking: &mut Vec<PathBuf>,
) -> eyre::Result<()> {
let dest = get_target_sql_file(&package_manifest_path, extdir, base_directory)?;
let dest = get_target_sql_file(&package_manifest_path, extdir, base_directory.to_path_buf())?;
let (_, extname) = find_control_file(&package_manifest_path)?;

crate::command::schema::generate_schema(
Expand All @@ -391,26 +391,24 @@ fn copy_sql_files(
)?;

// now copy all the version upgrade files too
if let Ok(dir) = std::fs::read_dir("sql/") {
for sql in dir {
if let Ok(sql) = sql {
let filename = sql.file_name().into_string().unwrap();

if filename.starts_with(&format!("{extname}--")) && filename.ends_with(".sql") {
let mut dest = base_directory.clone();
dest.push(extdir);
dest.push(filename);

copy_file(
&sql.path(),
&dest,
"extension schema upgrade file",
true,
&package_manifest_path,
output_tracking,
pg_config,
)?;
}
if let Ok(dir) = fs::read_dir("sql/") {
for sql in dir.flatten() {
let filename = sql.file_name().into_string().unwrap();

if filename.starts_with(&format!("{extname}--")) && filename.ends_with(".sql") {
let mut dest = base_directory.to_path_buf();
dest.push(extdir);
dest.push(filename);

copy_file(
&sql.path(),
dest,
"extension schema upgrade file",
true,
&package_manifest_path,
output_tracking,
pg_config,
)?;
}
}
}
Expand All @@ -419,7 +417,7 @@ fn copy_sql_files(

#[tracing::instrument(level = "error", skip_all)]
pub(crate) fn find_library_file(
manifest: &cargo_toml::Manifest,
manifest: &Manifest,
build_command_messages: &Vec<cargo_metadata::Message>,
) -> eyre::Result<PathBuf> {
let target_name = manifest.target_name()?;
Expand Down
45 changes: 18 additions & 27 deletions cargo-pgrx/src/command/new.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,20 +51,18 @@ pub(crate) fn create_crate_template(
name: &str,
is_bgworker: bool,
) -> eyre::Result<()> {
create_directory_structure(&path)?;
create_control_file(&path, name)?;
create_cargo_toml(&path, name)?;
create_dotcargo_config_toml(&path, name)?;
create_lib_rs(&path, name, is_bgworker)?;
create_git_ignore(&path, name)?;
create_pgrx_embed_rs(&path)?;
create_directory_structure(path.clone())?;
create_control_file(path.clone(), name)?;
create_cargo_toml(path.clone(), name)?;
create_dotcargo_config_toml(path.clone(), name)?;
create_lib_rs(path.clone(), name, is_bgworker)?;
create_git_ignore(path.clone(), name)?;
create_pgrx_embed_rs(path)?;

Ok(())
}

fn create_directory_structure(path: &PathBuf) -> Result<(), std::io::Error> {
let mut src_dir = path.clone();

fn create_directory_structure(mut src_dir: PathBuf) -> Result<(), std::io::Error> {
src_dir.push("src");
std::fs::create_dir_all(&src_dir)?;

Expand All @@ -85,9 +83,7 @@ fn create_directory_structure(path: &PathBuf) -> Result<(), std::io::Error> {
Ok(())
}

fn create_control_file(path: &PathBuf, name: &str) -> Result<(), std::io::Error> {
let mut filename = path.clone();

fn create_control_file(mut filename: PathBuf, name: &str) -> Result<(), std::io::Error> {
filename.push(format!("{name}.control"));
let mut file = std::fs::File::create(filename)?;

Expand All @@ -96,9 +92,7 @@ fn create_control_file(path: &PathBuf, name: &str) -> Result<(), std::io::Error>
Ok(())
}

fn create_cargo_toml(path: &PathBuf, name: &str) -> Result<(), std::io::Error> {
let mut filename = path.clone();

fn create_cargo_toml(mut filename: PathBuf, name: &str) -> Result<(), std::io::Error> {
filename.push("Cargo.toml");
let mut file = std::fs::File::create(filename)?;

Expand All @@ -107,9 +101,7 @@ fn create_cargo_toml(path: &PathBuf, name: &str) -> Result<(), std::io::Error> {
Ok(())
}

fn create_dotcargo_config_toml(path: &PathBuf, _name: &str) -> Result<(), std::io::Error> {
let mut filename = path.clone();

fn create_dotcargo_config_toml(mut filename: PathBuf, _name: &str) -> Result<(), std::io::Error> {
filename.push(".cargo");
filename.push("config.toml");
let mut file = std::fs::File::create(filename)?;
Expand All @@ -119,9 +111,11 @@ fn create_dotcargo_config_toml(path: &PathBuf, _name: &str) -> Result<(), std::i
Ok(())
}

fn create_lib_rs(path: &PathBuf, name: &str, is_bgworker: bool) -> Result<(), std::io::Error> {
let mut filename = path.clone();

fn create_lib_rs(
mut filename: PathBuf,
name: &str,
is_bgworker: bool,
) -> Result<(), std::io::Error> {
filename.push("src");
filename.push("lib.rs");
let mut file = std::fs::File::create(filename)?;
Expand All @@ -137,9 +131,7 @@ fn create_lib_rs(path: &PathBuf, name: &str, is_bgworker: bool) -> Result<(), st
Ok(())
}

fn create_git_ignore(path: &PathBuf, _name: &str) -> Result<(), std::io::Error> {
let mut filename = path.clone();

fn create_git_ignore(mut filename: PathBuf, _name: &str) -> Result<(), std::io::Error> {
filename.push(".gitignore");
let mut file = std::fs::File::create(filename)?;

Expand All @@ -148,8 +140,7 @@ fn create_git_ignore(path: &PathBuf, _name: &str) -> Result<(), std::io::Error>
Ok(())
}

fn create_pgrx_embed_rs(path: &PathBuf) -> Result<(), std::io::Error> {
let mut filename = path.clone();
fn create_pgrx_embed_rs(mut filename: PathBuf) -> Result<(), std::io::Error> {
filename.push("src");
filename.push("bin");
filename.push(format!("pgrx_embed.rs"));
Expand Down
Loading

0 comments on commit 12842b6

Please sign in to comment.