diff --git a/src/lib.rs b/src/lib.rs index 6915f62..6e21bbe 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -32,22 +32,17 @@ enum Build> { impl> Build

{ /// Returns a build pipeline identified by `pipe`, or an error if `pipe` /// is unknown. - fn new( - pipe: &dist::Pipeline, - dir: P, - cfg: PgConfig, - sudo: bool, - ) -> Result, BuildError> { + fn new(pipe: &dist::Pipeline, dir: P, cfg: PgConfig) -> Result, BuildError> { match pipe { - dist::Pipeline::Pgxs => Ok(Build::Pgxs(Pgxs::new(dir, cfg, sudo))), - dist::Pipeline::Pgrx => Ok(Build::Pgrx(Pgrx::new(dir, cfg, sudo))), + dist::Pipeline::Pgxs => Ok(Build::Pgxs(Pgxs::new(dir, cfg))), + dist::Pipeline::Pgrx => Ok(Build::Pgrx(Pgrx::new(dir, cfg))), _ => Err(BuildError::UnknownPipeline(pipe.to_string())), } } /// Attempts to detect and return the appropriate build pipeline to build /// the contents of `dir`. Returns an error if no pipeline can do so. - fn detect(dir: P, cfg: PgConfig, sudo: bool) -> Result, BuildError> { + fn detect(dir: P, cfg: PgConfig) -> Result, BuildError> { // Start with PGXS. let mut score = Pgxs::confidence(&dir); let mut pipe = dist::Pipeline::Pgxs; @@ -67,8 +62,8 @@ impl> Build

{ // Construct the winner. match pipe { - dist::Pipeline::Pgrx => Ok(Build::Pgrx(Pgrx::new(dir, cfg, sudo))), - dist::Pipeline::Pgxs => Ok(Build::Pgxs(Pgxs::new(dir, cfg, sudo))), + dist::Pipeline::Pgrx => Ok(Build::Pgrx(Pgrx::new(dir, cfg))), + dist::Pipeline::Pgxs => Ok(Build::Pgxs(Pgxs::new(dir, cfg))), _ => unreachable!("unknown pipelines {pipe}"), } } @@ -83,15 +78,15 @@ pub struct Builder> { impl> Builder

{ /// Creates and returns a new builder using the appropriate pipeline. - pub fn new(dir: P, meta: Release, cfg: PgConfig, sudo: bool) -> Result { + pub fn new(dir: P, meta: Release, cfg: PgConfig) -> Result { let pipeline = if let Some(deps) = meta.dependencies() { if let Some(pipe) = deps.pipeline() { - Build::new(pipe, dir, cfg, sudo)? + Build::new(pipe, dir, cfg)? } else { - Build::detect(dir, cfg, sudo)? + Build::detect(dir, cfg)? } } else { - Build::detect(dir, cfg, sudo)? + Build::detect(dir, cfg)? }; Ok(Builder { pipeline, meta }) diff --git a/src/pg_config/mod.rs b/src/pg_config/mod.rs index 8345d99..59ba4aa 100644 --- a/src/pg_config/mod.rs +++ b/src/pg_config/mod.rs @@ -48,7 +48,7 @@ impl PgConfig { /// Returns the `pg_config` value for `cfg`, which should be a lowercase /// string. - pub fn get(&mut self, cfg: &str) -> Option<&str> { + pub fn get(&self, cfg: &str) -> Option<&str> { match self.0.get(cfg) { Some(c) => Some(c.as_str()), None => None, diff --git a/src/pg_config/tests.rs b/src/pg_config/tests.rs index 35efc89..f106725 100644 --- a/src/pg_config/tests.rs +++ b/src/pg_config/tests.rs @@ -29,7 +29,7 @@ fn pg_config() -> Result<(), BuildError> { ]); // Parse its output. - let mut cfg = PgConfig::new(&path)?; + let cfg = PgConfig::new(&path)?; assert_eq!(&exp, &cfg.0); // Get lowercase. diff --git a/src/pgrx/mod.rs b/src/pgrx/mod.rs index 628a99f..f874fb1 100644 --- a/src/pgrx/mod.rs +++ b/src/pgrx/mod.rs @@ -12,14 +12,13 @@ use std::path::Path; /// [pgrx]: https://github.com/pgcentralfoundation/pgrx #[derive(Debug, PartialEq)] pub(crate) struct Pgrx> { - sudo: bool, cfg: PgConfig, dir: P, } impl> Pipeline

for Pgrx

{ - fn new(dir: P, cfg: PgConfig, sudo: bool) -> Self { - Pgrx { sudo, cfg, dir } + fn new(dir: P, cfg: PgConfig) -> Self { + Pgrx { cfg, dir } } /// Returns the directory passed to [`Self::new`]. diff --git a/src/pgrx/tests.rs b/src/pgrx/tests.rs index 9eb761d..79fff8f 100644 --- a/src/pgrx/tests.rs +++ b/src/pgrx/tests.rs @@ -28,25 +28,23 @@ fn confidence() -> Result<(), BuildError> { fn new() { let dir = Path::new(env!("CARGO_MANIFEST_DIR")); let cfg = PgConfig::from_map(HashMap::new()); - let pipe = Pgrx::new(dir, cfg.clone(), false); + let pipe = Pgrx::new(dir, cfg.clone()); assert_eq!(dir, pipe.dir); assert_eq!(&dir, pipe.dir()); assert_eq!(&cfg, pipe.pg_config()); - assert!(!pipe.sudo); let dir2 = dir.join("corpus"); let cfg2 = PgConfig::from_map(HashMap::from([("bindir".to_string(), "bin".to_string())])); - let pipe = Pgrx::new(dir2.as_path(), cfg2.clone(), true); + let pipe = Pgrx::new(dir2.as_path(), cfg2.clone()); assert_eq!(dir2, pipe.dir); assert_eq!(&dir2, pipe.dir()); assert_eq!(&cfg2, pipe.pg_config()); - assert!(pipe.sudo); } #[test] fn configure_et_al() { let dir = Path::new(env!("CARGO_MANIFEST_DIR")); - let pipe = Pgrx::new(dir, PgConfig::from_map(HashMap::new()), false); + let pipe = Pgrx::new(dir, PgConfig::from_map(HashMap::new())); assert!(pipe.configure().is_ok()); assert!(pipe.compile().is_ok()); assert!(pipe.test().is_ok()); diff --git a/src/pgxs/mod.rs b/src/pgxs/mod.rs index 5b77ef2..a51669c 100644 --- a/src/pgxs/mod.rs +++ b/src/pgxs/mod.rs @@ -17,14 +17,13 @@ use std::{ /// [PGXS]: https://www.postgresql.org/docs/current/extend-pgxs.html #[derive(Debug, PartialEq)] pub(crate) struct Pgxs> { - sudo: bool, cfg: PgConfig, dir: P, } impl> Pipeline

for Pgxs

{ - fn new(dir: P, cfg: PgConfig, sudo: bool) -> Self { - Pgxs { sudo, cfg, dir } + fn new(dir: P, cfg: PgConfig) -> Self { + Pgxs { cfg, dir } } /// Determines the confidence that the Pgxs pipeline can build the @@ -92,19 +91,19 @@ impl> Pipeline

for Pgxs

{ fn compile(&self) -> Result<(), BuildError> { info!("building extension"); - self.run("make", ["all"], self.sudo)?; + self.run("make", ["all"], false)?; Ok(()) } fn test(&self) -> Result<(), BuildError> { info!("testing extension"); - self.run("make", ["installcheck"], self.sudo)?; + self.run("make", ["installcheck"], false)?; Ok(()) } fn install(&self) -> Result<(), BuildError> { info!("installing extension"); - self.run("make", ["install"], self.sudo)?; + self.run("make", ["install"], true)?; Ok(()) } } diff --git a/src/pgxs/tests.rs b/src/pgxs/tests.rs index afa11ef..188bc3a 100644 --- a/src/pgxs/tests.rs +++ b/src/pgxs/tests.rs @@ -57,25 +57,23 @@ fn confidence() -> Result<(), BuildError> { fn new() { let dir = Path::new(env!("CARGO_MANIFEST_DIR")); let cfg = PgConfig::from_map(HashMap::new()); - let pipe = Pgxs::new(dir, cfg.clone(), false); + let pipe = Pgxs::new(dir, cfg.clone()); assert_eq!(dir, pipe.dir); assert_eq!(&dir, pipe.dir()); assert_eq!(&cfg, pipe.pg_config()); - assert!(!pipe.sudo); let dir2 = dir.join("corpus"); let cfg2 = PgConfig::from_map(HashMap::from([("bindir".to_string(), "bin".to_string())])); - let pipe = Pgxs::new(dir2.as_path(), cfg2.clone(), true); + let pipe = Pgxs::new(dir2.as_path(), cfg2.clone()); assert_eq!(dir2, pipe.dir); assert_eq!(&dir2, pipe.dir()); assert_eq!(&cfg2, pipe.pg_config()); - assert!(pipe.sudo); } #[test] fn configure() -> Result<(), BuildError> { let tmp = tempdir()?; - let pipe = Pgxs::new(&tmp, PgConfig::from_map(HashMap::new()), false); + let pipe = Pgxs::new(&tmp, PgConfig::from_map(HashMap::new())); // Try with no Configure file. if let Err(e) = pipe.configure() { @@ -127,7 +125,7 @@ fn configure() -> Result<(), BuildError> { #[test] fn compile() -> Result<(), BuildError> { let dir = Path::new(env!("CARGO_MANIFEST_DIR")); - let pipe = Pgxs::new(dir, PgConfig::from_map(HashMap::new()), false); + let pipe = Pgxs::new(dir, PgConfig::from_map(HashMap::new())); assert!(pipe.compile().is_err()); Ok(()) } @@ -135,7 +133,7 @@ fn compile() -> Result<(), BuildError> { #[test] fn test() -> Result<(), BuildError> { let dir = Path::new(env!("CARGO_MANIFEST_DIR")); - let pipe = Pgxs::new(dir, PgConfig::from_map(HashMap::new()), false); + let pipe = Pgxs::new(dir, PgConfig::from_map(HashMap::new())); assert!(pipe.test().is_err()); Ok(()) } @@ -143,7 +141,7 @@ fn test() -> Result<(), BuildError> { #[test] fn install() -> Result<(), BuildError> { let dir = Path::new(env!("CARGO_MANIFEST_DIR")); - let pipe = Pgxs::new(dir, PgConfig::from_map(HashMap::new()), false); + let pipe = Pgxs::new(dir, PgConfig::from_map(HashMap::new())); assert!(pipe.install().is_err()); Ok(()) } diff --git a/src/pipeline/mod.rs b/src/pipeline/mod.rs index 4b193ee..f8b4359 100644 --- a/src/pipeline/mod.rs +++ b/src/pipeline/mod.rs @@ -8,7 +8,7 @@ use std::{io::Write, path::Path, process::Command}; /// PGXN distributions. pub(crate) trait Pipeline> { /// Creates an instance of a Pipeline. - fn new(dir: P, pg_config: PgConfig, sudo: bool) -> Self; + fn new(dir: P, pg_config: PgConfig) -> Self; /// Returns a score for the confidence that this pipeline can build the /// contents of `dir`. A score of 0 means no confidence and 255 means the @@ -34,6 +34,22 @@ pub(crate) trait Pipeline> { /// Returns the PgConfig passed to [`new`]. fn pg_config(&self) -> &PgConfig; + // maybe_sudo returns a Command that starts with the sudo command if + // `sudo` is true and the `pkglibdir` returned by pg_config isn't + // writeable by the current user. + fn maybe_sudo(&self, program: &str, sudo: bool) -> Command { + if sudo { + if let Some(dir) = self.pg_config().get("pkglibdir") { + if !self.is_writeable(dir) { + let mut c = Command::new("sudo"); + c.arg(program); + return c; + } + } + } + Command::new(program) + } + /// Attempts to write a temporary file to `dir` and returns `true` on /// success and `false` on failure. The temporary file will be deleted. fn is_writeable>(&self, dir: D) -> bool { @@ -48,22 +64,15 @@ pub(crate) trait Pipeline> { } } - /// Run a command. Runs it with elevated privileges using `sudo` unless - /// it's on Windows. - fn run(&self, cmd: &str, args: I, sudo: bool) -> Result<(), BuildError> + /// Run a command. Runs it with elevated privileges when `sudo` is true + /// and `pg_config --pkglibdir` isn't writeable by the current user. + fn run(&self, program: &str, args: I, sudo: bool) -> Result<(), BuildError> where I: IntoIterator, S: AsRef, { // Use `sudo` if the param is set. - let mut cmd = if sudo { - let mut c = Command::new("sudo"); - c.arg(cmd); - c - } else { - Command::new(cmd) - }; - + let mut cmd = self.maybe_sudo(program, sudo); cmd.args(args); cmd.current_dir(self.dir()); match cmd.output() { diff --git a/src/pipeline/tests.rs b/src/pipeline/tests.rs index 8e3c206..b33bb19 100644 --- a/src/pipeline/tests.rs +++ b/src/pipeline/tests.rs @@ -12,7 +12,7 @@ struct TestPipeline> { // Create a mock version of the trait. #[cfg(test)] impl> Pipeline

for TestPipeline

{ - fn new(dir: P, cfg: PgConfig, _: bool) -> Self { + fn new(dir: P, cfg: PgConfig) -> Self { TestPipeline { dir, cfg } } @@ -47,7 +47,7 @@ fn run() -> Result<(), BuildError> { let cfg = PgConfig::from_map(HashMap::new()); // Test basic success. - let pipe = TestPipeline::new(&tmp, cfg, false); + let pipe = TestPipeline::new(&tmp, cfg); if let Err(e) = pipe.run("echo", ["hello"], false) { panic!("echo hello failed: {e}"); } @@ -104,9 +104,44 @@ fn is_writeable() -> Result<(), BuildError> { let tmp = tempdir()?; let cfg = PgConfig::from_map(HashMap::new()); - let pipe = TestPipeline::new(&tmp, cfg, false); + let pipe = TestPipeline::new(&tmp, cfg); assert!(pipe.is_writeable(&tmp)); assert!(!pipe.is_writeable(tmp.path().join(" nonesuch"))); Ok(()) } + +#[test] +fn maybe_sudo() -> Result<(), BuildError> { + let tmp = tempdir()?; + let cfg = PgConfig::from_map(HashMap::from([( + "pkglibdir".to_string(), + tmp.as_ref().display().to_string(), + )])); + let pipe = TestPipeline::new(&tmp, cfg); + + // Never use sudo when param is false. + let cmd = pipe.maybe_sudo("foo", false); + assert_eq!("foo", cmd.get_program().to_str().unwrap()); + + // Never use sudo when directory is writeable. + let cmd = pipe.maybe_sudo("foo", true); + assert_eq!("foo", cmd.get_program().to_str().unwrap()); + + // Use sudo when the directory is not writeable. + let cfg = PgConfig::from_map(HashMap::from([( + "pkglibdir".to_string(), + tmp.path().join("nonesuch").display().to_string(), + )])); + let pipe = TestPipeline::new(&tmp, cfg); + let cmd = pipe.maybe_sudo("foo", true); + assert_eq!("sudo", cmd.get_program().to_str().unwrap()); + let args: Vec<&std::ffi::OsStr> = cmd.get_args().collect(); + assert_eq!(args, &["foo"]); + + // Never use sudo when param is false. + let cmd = pipe.maybe_sudo("foo", false); + assert_eq!("foo", cmd.get_program().to_str().unwrap()); + + Ok(()) +} diff --git a/src/tests.rs b/src/tests.rs index de51072..68be89c 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -41,11 +41,11 @@ fn pgxs() { let tmp = tempdir().unwrap(); let cfg = PgConfig::from_map(HashMap::new()); let rel = Release::try_from(meta.clone()).unwrap(); - let builder = Builder::new(tmp.as_ref(), rel, cfg, false).unwrap(); + let builder = Builder::new(tmp.as_ref(), rel, cfg).unwrap(); let rel = Release::try_from(meta).unwrap(); let cfg = PgConfig::from_map(HashMap::new()); let exp = Builder { - pipeline: Build::Pgxs(Pgxs::new(tmp.as_ref(), cfg, false)), + pipeline: Build::Pgxs(Pgxs::new(tmp.as_ref(), cfg)), meta: rel, }; assert_eq!(exp, builder, "pgxs"); @@ -63,10 +63,10 @@ fn pgrx() { let tmp = tempdir().unwrap(); let cfg = PgConfig::from_map(HashMap::new()); let rel = Release::try_from(meta.clone()).unwrap(); - let builder = Builder::new(tmp.as_ref(), rel, cfg.clone(), false).unwrap(); + let builder = Builder::new(tmp.as_ref(), rel, cfg.clone()).unwrap(); let rel = Release::try_from(meta).unwrap(); let exp = Builder { - pipeline: Build::Pgrx(Pgrx::new(tmp.as_ref(), cfg.clone(), false)), + pipeline: Build::Pgrx(Pgrx::new(tmp.as_ref(), cfg.clone())), meta: rel, }; assert_eq!(exp, builder, "pgrx"); @@ -84,7 +84,7 @@ fn unsupported_pipeline() { let cfg = PgConfig::from_map(HashMap::new()); assert_eq!( BuildError::UnknownPipeline("meson".to_string()).to_string(), - Builder::new("dir", rel, cfg, true).unwrap_err().to_string(), + Builder::new("dir", rel, cfg).unwrap_err().to_string(), ); } @@ -112,7 +112,7 @@ fn detect_pipeline() -> Result<(), BuildError> { let tmp = tempdir()?; let dir = tmp.as_ref(); let cfg = PgConfig::from_map(HashMap::new()); - match Build::detect(dir, cfg.clone(), true) { + match Build::detect(dir, cfg.clone()) { Ok(_) => panic!("detect unexpectedly succeeded with empty dir"), Err(e) => assert_eq!( "cannot detect build pipeline and none specified", @@ -120,7 +120,7 @@ fn detect_pipeline() -> Result<(), BuildError> { ), } for meta in &metas { - match Builder::new(dir, no_pipe(meta), cfg.clone(), true) { + match Builder::new(dir, no_pipe(meta), cfg.clone()) { Ok(_) => panic!("detect unexpectedly succeeded with empty dir"), Err(e) => assert_eq!( "cannot detect build pipeline and none specified", @@ -131,25 +131,25 @@ fn detect_pipeline() -> Result<(), BuildError> { // Add an empty Makefile, PGXS should win. let mut makefile = File::create(dir.join("Makefile"))?; - match Build::detect(dir, cfg.clone(), true) { - Ok(p) => assert_eq!(Build::Pgxs(Pgxs::new(dir, cfg.clone(), true)), p), + match Build::detect(dir, cfg.clone()) { + Ok(p) => assert_eq!(Build::Pgxs(Pgxs::new(dir, cfg.clone())), p), Err(e) => panic!("Unexpectedly errored with Makefile: {e}"), } for meta in &metas { - match Builder::new(dir, no_pipe(meta), cfg.clone(), true) { - Ok(b) => assert_eq!(Build::Pgxs(Pgxs::new(dir, cfg.clone(), true)), b.pipeline), + match Builder::new(dir, no_pipe(meta), cfg.clone()) { + Ok(b) => assert_eq!(Build::Pgxs(Pgxs::new(dir, cfg.clone())), b.pipeline), Err(e) => panic!("Unexpectedly errored with Makefile: {e}"), } } // Add an empty cargo.toml, PGXS should still win. let mut cargo_toml = File::create(dir.join("Cargo.toml"))?; - match Build::detect(dir, cfg.clone(), false) { - Ok(p) => assert_eq!(Build::Pgxs(Pgxs::new(dir, cfg.clone(), false)), p), + match Build::detect(dir, cfg.clone()) { + Ok(p) => assert_eq!(Build::Pgxs(Pgxs::new(dir, cfg.clone())), p), Err(e) => panic!("Unexpectedly errored with Cargo.toml: {e}"), } for meta in &metas { - match Builder::new(dir, no_pipe(meta), cfg.clone(), true) { - Ok(b) => assert_eq!(Build::Pgxs(Pgxs::new(dir, cfg.clone(), true)), b.pipeline), + match Builder::new(dir, no_pipe(meta), cfg.clone()) { + Ok(b) => assert_eq!(Build::Pgxs(Pgxs::new(dir, cfg.clone())), b.pipeline), Err(e) => panic!("Unexpectedly errored with Cargo.toml: {e}"), } } @@ -157,13 +157,13 @@ fn detect_pipeline() -> Result<(), BuildError> { // Add pgrx to Cargo.toml; now pgrx should win. writeln!(&cargo_toml, "[dependencies]\npgrx = \"0.12.6\"")?; cargo_toml.flush()?; - match Build::detect(dir, cfg.clone(), true) { - Ok(p) => assert_eq!(Build::Pgrx(Pgrx::new(dir, cfg.clone(), true)), p), + match Build::detect(dir, cfg.clone()) { + Ok(p) => assert_eq!(Build::Pgrx(Pgrx::new(dir, cfg.clone())), p), Err(e) => panic!("Unexpectedly errored with pgrx dependency: {e}"), } for meta in &metas { - match Builder::new(dir, no_pipe(meta), cfg.clone(), false) { - Ok(b) => assert_eq!(Build::Pgrx(Pgrx::new(dir, cfg.clone(), false)), b.pipeline), + match Builder::new(dir, no_pipe(meta), cfg.clone()) { + Ok(b) => assert_eq!(Build::Pgrx(Pgrx::new(dir, cfg.clone())), b.pipeline), Err(e) => panic!("Unexpectedly errored with pgrx dependency: {e}"), } } @@ -171,17 +171,17 @@ fn detect_pipeline() -> Result<(), BuildError> { // Add PG_CONFIG to the Makefile, PGXS should win again. writeln!(&makefile, "PG_CONFIG ?= pg_config")?; makefile.flush()?; - match Build::detect(dir, cfg.clone(), false) { + match Build::detect(dir, cfg.clone()) { Ok(p) => assert_eq!( - Build::Pgxs(Pgxs::new(dir, PgConfig::from_map(HashMap::new()), false)), + Build::Pgxs(Pgxs::new(dir, PgConfig::from_map(HashMap::new()))), p ), Err(e) => panic!("Unexpectedly errored with PG_CONFIG var: {e}"), } for meta in &metas { - match Builder::new(dir, no_pipe(meta), cfg.clone(), false) { + match Builder::new(dir, no_pipe(meta), cfg.clone()) { Ok(b) => assert_eq!( - Build::Pgxs(Pgxs::new(dir, PgConfig::from_map(HashMap::new()), false)), + Build::Pgxs(Pgxs::new(dir, PgConfig::from_map(HashMap::new()))), b.pipeline ), Err(e) => panic!("Unexpectedly errored with PG_CONFIG var: {e}"),