Replace sudo param with maybe_sudo method.

The `maybe_sudo` method creates a new command based on whether `sudo`
might be needed and, if so, whether `pkglibdir` is writable. If `sudo`
is true and `pkglibdir` is not writable it creates a Command with `sudo`
as the program and the program passed as its first argument. Otherwise
it returns a Command with the passed program.

With that intelligence, remove the `sudo` param from the interface and
all methods. There will be no need to specify `sudo` as a boolean, as
the package can figure it out for itself.

In the future it might be necessary to allow the user to specify a path
to `sudo` (and `make` and `cargo` and whatever else?), but for now it
will be enough to have the user make sure that such commands are in the
path.

Also remove `mut` from `PgConfig::get`, as it's unnecessary and makes
its usage more difficult. Found while using it in `maybe_sudo`.

src/lib.rs

@@ -32,22 +32,17 @@ enum Build<P: AsRef<Path>> {
 impl<P: AsRef<Path>> Build<P> {
     /// 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<Build<P>, BuildError> {
+    fn new(pipe: &dist::Pipeline, dir: P, cfg: PgConfig) -> Result<Build<P>, 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<Build<P>, BuildError> {
+    fn detect(dir: P, cfg: PgConfig) -> Result<Build<P>, BuildError> {
         // Start with PGXS.
         let mut score = Pgxs::confidence(&dir);
         let mut pipe = dist::Pipeline::Pgxs;
@@ -67,8 +62,8 @@ impl<P: AsRef<Path>> Build<P> {
 
         // 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<P: AsRef<Path>> {
 
 impl<P: AsRef<Path>> Builder<P> {
     /// Creates and returns a new builder using the appropriate pipeline.
-    pub fn new(dir: P, meta: Release, cfg: PgConfig, sudo: bool) -> Result<Self, BuildError> {
+    pub fn new(dir: P, meta: Release, cfg: PgConfig) -> Result<Self, BuildError> {
         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 })

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,

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.

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<P: AsRef<Path>> {
-    sudo: bool,
     cfg: PgConfig,
     dir: P,
 }
 
 impl<P: AsRef<Path>> Pipeline<P> for Pgrx<P> {
-    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`].

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());

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<P: AsRef<Path>> {
-    sudo: bool,
     cfg: PgConfig,
     dir: P,
 }
 
 impl<P: AsRef<Path>> Pipeline<P> for Pgxs<P> {
-    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<P: AsRef<Path>> Pipeline<P> for Pgxs<P> {
 
     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(())
     }
 }

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,23 +125,23 @@ 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(())
 }
 
 #[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(())
 }
 
 #[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(())
 }

src/pipeline/mod.rs

@@ -8,7 +8,7 @@ use std::{io::Write, path::Path, process::Command};
 /// PGXN distributions.
 pub(crate) trait Pipeline<P: AsRef<Path>> {
     /// 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<P: AsRef<Path>> {
     /// 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<D: AsRef<Path>>(&self, dir: D) -> bool {
@@ -48,22 +64,15 @@ pub(crate) trait Pipeline<P: AsRef<Path>> {
         }
     }
 
-    /// Run a command. Runs it with elevated privileges using `sudo` unless
-    /// it's on Windows.
-    fn run<S, I>(&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<S, I>(&self, program: &str, args: I, sudo: bool) -> Result<(), BuildError>
     where
         I: IntoIterator<Item = S>,
         S: AsRef<std::ffi::OsStr>,
     {
         // 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() {

src/pipeline/tests.rs

@@ -12,7 +12,7 @@ struct TestPipeline<P: AsRef<Path>> {
 // Create a mock version of the trait.
 #[cfg(test)]
 impl<P: AsRef<Path>> Pipeline<P> for TestPipeline<P> {
-    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(())
+}