Cluster records the runtime _strategy_, not the runtime

Author: allenap

README.md

@@ -85,7 +85,7 @@ versions that are not supported upstream).
 use postgresfixture::prelude::*;
 for runtime in strategy::default().runtimes() {
   let data_dir = tempdir::TempDir::new("data")?;
-  let cluster = Cluster::new(&data_dir, &runtime)?;
+  let cluster = Cluster::new(&data_dir, runtime)?;
   cluster.start()?;
   assert_eq!(cluster.databases()?, vec!["postgres", "template0", "template1"]);
   let mut conn = cluster.connect("template1")?;

src/cluster.rs

@@ -25,56 +25,54 @@ pub use error::ClusterError;
 /// stop, and destroy the cluster. There's no protection against concurrent
 /// changes to the cluster made by other processes, but the functions in the
 /// [`coordinate`][`crate::coordinate`] module may help.
-#[derive(Clone, Debug)]
 pub struct Cluster {
     /// The data directory of the cluster.
     ///
     /// Corresponds to the `PGDATA` environment variable.
     datadir: PathBuf,
-    /// The installation of PostgreSQL to use with this cluster.
-    runtime: runtime::Runtime,
+    /// How to select the PostgreSQL installation to use with this cluster.
+    strategy: Box<dyn runtime::strategy::RuntimeStrategy>,
 }
 
 impl Cluster {
     /// Represent a cluster at the given path.
-    ///
-    /// This will use the given strategy to determine an appropriate runtime to
-    /// use with the cluster in the given data directory, if it exists. If an
-    /// appropriate runtime cannot be found, [`ClusterError::RuntimeNotFound`]
-    /// will be returned.
     pub fn new<P: AsRef<Path>, S: runtime::strategy::RuntimeStrategy>(
         datadir: P,
-        strategy: &S,
+        strategy: S,
     ) -> Result<Self, ClusterError> {
-        let datadir = datadir.as_ref();
-        let version = version(datadir)?;
-        let runtime = match version {
-            None => strategy
+        Ok(Self {
+            datadir: datadir.as_ref().to_owned(),
+            strategy: Box::new(strategy),
+        })
+    }
+
+    /// Determine the runtime to use with this cluster.
+    fn runtime(&self) -> Result<runtime::Runtime, ClusterError> {
+        match version(self)? {
+            None => self
+                .strategy
                 .fallback()
                 .ok_or_else(|| ClusterError::RuntimeDefaultNotFound),
-            Some(version) => strategy
+            Some(version) => self
+                .strategy
                 .select(&version)
                 .ok_or_else(|| ClusterError::RuntimeNotFound(version)),
-        }?;
-        Ok(Self {
-            datadir: datadir.to_owned(),
-            runtime,
-        })
+        }
     }
 
-    fn ctl(&self) -> Command {
-        let mut command = self.runtime.execute("pg_ctl");
+    fn ctl(&self) -> Result<Command, ClusterError> {
+        let mut command = self.runtime()?.execute("pg_ctl");
         command.env("PGDATA", &self.datadir);
         command.env("PGHOST", &self.datadir);
-        command
+        Ok(command)
     }
 
     /// Check if this cluster is running.
     ///
     /// Tries to distinguish carefully between "definitely running", "definitely
     /// not running", and "don't know". The latter results in `ClusterError`.
     pub fn running(&self) -> Result<bool, ClusterError> {
-        let output = self.ctl().arg("status").output()?;
+        let output = self.ctl()?.arg("status").output()?;
         let code = match output.status.code() {
             // Killed by signal; return early.
             None => return Err(ClusterError::Other(output)),
@@ -83,11 +81,12 @@ impl Cluster {
             // More work required to decode what this means.
             Some(code) => code,
         };
+        let runtime = self.runtime()?;
         // PostgreSQL has evolved to return different error codes in
         // later versions, so here we check for specific codes to avoid
         // masking errors from insufficient permissions or missing
         // executables, for example.
-        let running = match self.runtime.version {
+        let running = match runtime.version {
             // PostgreSQL 10.x and later.
             version::Version::Post10(_major, _minor) => {
                 // PostgreSQL 10
@@ -162,7 +161,7 @@ impl Cluster {
             Some(running) => Ok(running),
             // TODO: Perhaps include the exit code from `pg_ctl status` in the
             // error message, and whatever it printed out.
-            None => Err(ClusterError::UnsupportedVersion(self.runtime.version)),
+            None => Err(ClusterError::UnsupportedVersion(runtime.version)),
         }
     }
 
@@ -197,7 +196,7 @@ impl Cluster {
             // Create the cluster and report back that we did so.
             fs::create_dir_all(&self.datadir)?;
             #[allow(clippy::suspicious_command_arg_space)]
-            self.ctl()
+            self.ctl()?
                 .arg("init")
                 .arg("-s")
                 .arg("-o")
@@ -236,7 +235,7 @@ impl Cluster {
         // postgres options:
         //  -h <arg> -- host name; empty arg means Unix socket only.
         //  -k -- socket directory.
-        self.ctl()
+        self.ctl()?
             .arg("start")
             .arg("-l")
             .arg(self.logfile())
@@ -266,7 +265,7 @@ impl Cluster {
     }
 
     pub fn shell(&self, database: &str) -> Result<ExitStatus, ClusterError> {
-        let mut command = self.runtime.execute("psql");
+        let mut command = self.runtime()?.execute("psql");
         command.arg("--quiet");
         command.env("PGDATA", &self.datadir);
         command.env("PGHOST", &self.datadir);
@@ -280,7 +279,7 @@ impl Cluster {
         command: T,
         args: &[T],
     ) -> Result<ExitStatus, ClusterError> {
-        let mut command = self.runtime.command(command);
+        let mut command = self.runtime()?.command(command);
         command.args(args);
         command.env("PGDATA", &self.datadir);
         command.env("PGHOST", &self.datadir);
@@ -338,7 +337,7 @@ impl Cluster {
         // pg_ctl options:
         //  -w -- wait for shutdown to complete.
         //  -m <mode> -- shutdown mode.
-        self.ctl()
+        self.ctl()?
             .arg("stop")
             .arg("-s")
             .arg("-w")

src/cluster/tests.rs

@@ -18,7 +18,7 @@ fn runtimes() -> Box<dyn Iterator<Item = Runtime>> {
 fn cluster_new() -> TestResult {
     for runtime in runtimes() {
         println!("{runtime:?}");
-        let cluster = Cluster::new("some/path", &runtime)?;
+        let cluster = Cluster::new("some/path", runtime)?;
         assert_eq!(Path::new("some/path"), cluster.datadir);
         assert!(!cluster.running()?);
     }
@@ -29,7 +29,7 @@ fn cluster_new() -> TestResult {
 fn cluster_does_not_exist() -> TestResult {
     for runtime in runtimes() {
         println!("{runtime:?}");
-        let cluster = Cluster::new("some/path", &runtime)?;
+        let cluster = Cluster::new("some/path", runtime)?;
         assert!(!exists(&cluster));
     }
     Ok(())
@@ -40,10 +40,10 @@ fn cluster_does_exist() -> TestResult {
     for runtime in runtimes() {
         println!("{runtime:?}");
         let data_dir = tempdir::TempDir::new("data")?;
-        let cluster = Cluster::new(&data_dir, &runtime)?;
+        let cluster = Cluster::new(&data_dir, runtime.clone())?;
         cluster.create()?;
         assert!(exists(&cluster));
-        let cluster = Cluster::new(&data_dir, &runtime)?;
+        let cluster = Cluster::new(&data_dir, runtime)?;
         assert!(exists(&cluster));
     }
     Ok(())
@@ -53,7 +53,7 @@ fn cluster_does_exist() -> TestResult {
 fn cluster_has_no_version_when_it_does_not_exist() -> TestResult {
     for runtime in runtimes() {
         println!("{runtime:?}");
-        let cluster = Cluster::new("some/path", &runtime)?;
+        let cluster = Cluster::new("some/path", runtime)?;
         assert!(matches!(version(&cluster), Ok(None)));
     }
     Ok(())
@@ -69,7 +69,7 @@ fn cluster_has_version_when_it_does_exist() -> TestResult {
         let pg_version: PartialVersion = runtime.version.into();
         let pg_version = pg_version.widened(); // e.g. 9.6.5 -> 9.6 or 14.3 -> 14.
         std::fs::write(&version_file, format!("{pg_version}\n"))?;
-        let cluster = Cluster::new(&data_dir, &runtime)?;
+        let cluster = Cluster::new(&data_dir, runtime)?;
         assert!(matches!(version(&cluster), Ok(Some(_))));
     }
     Ok(())
@@ -80,7 +80,7 @@ fn cluster_has_pid_file() -> TestResult {
     let data_dir = PathBuf::from("/some/where");
     for runtime in runtimes() {
         println!("{runtime:?}");
-        let cluster = Cluster::new(&data_dir, &runtime)?;
+        let cluster = Cluster::new(&data_dir, runtime)?;
         assert_eq!(
             PathBuf::from("/some/where/postmaster.pid"),
             cluster.pidfile()
@@ -94,7 +94,7 @@ fn cluster_has_log_file() -> TestResult {
     let data_dir = PathBuf::from("/some/where");
     for runtime in runtimes() {
         println!("{runtime:?}");
-        let cluster = Cluster::new(&data_dir, &runtime)?;
+        let cluster = Cluster::new(&data_dir, runtime)?;
         assert_eq!(
             PathBuf::from("/some/where/postmaster.log"),
             cluster.logfile()
@@ -108,7 +108,7 @@ fn cluster_create_creates_cluster() -> TestResult {
     for runtime in runtimes() {
         println!("{runtime:?}");
         let data_dir = tempdir::TempDir::new("data")?;
-        let cluster = Cluster::new(&data_dir, &runtime)?;
+        let cluster = Cluster::new(&data_dir, runtime)?;
         assert!(!exists(&cluster));
         assert!(cluster.create()?);
         assert!(exists(&cluster));
@@ -121,7 +121,7 @@ fn cluster_create_creates_cluster_with_neutral_locale_and_timezone() -> TestResu
     for runtime in runtimes() {
         println!("{runtime:?}");
         let data_dir = tempdir::TempDir::new("data")?;
-        let cluster = Cluster::new(&data_dir, &runtime)?;
+        let cluster = Cluster::new(&data_dir, runtime.clone())?;
         cluster.start()?;
         let mut conn = cluster.connect("postgres")?;
         let result = conn.query("SHOW ALL", &[])?;
@@ -190,7 +190,7 @@ fn cluster_create_does_nothing_when_it_already_exists() -> TestResult {
     for runtime in runtimes() {
         println!("{runtime:?}");
         let data_dir = tempdir::TempDir::new("data")?;
-        let cluster = Cluster::new(&data_dir, &runtime)?;
+        let cluster = Cluster::new(&data_dir, runtime)?;
         assert!(!exists(&cluster));
         assert!(cluster.create()?);
         assert!(exists(&cluster));
@@ -204,7 +204,7 @@ fn cluster_start_stop_starts_and_stops_cluster() -> TestResult {
     for runtime in runtimes() {
         println!("{runtime:?}");
         let data_dir = tempdir::TempDir::new("data")?;
-        let cluster = Cluster::new(&data_dir, &runtime)?;
+        let cluster = Cluster::new(&data_dir, runtime)?;
         cluster.create()?;
         assert!(!cluster.running()?);
         cluster.start()?;
@@ -220,7 +220,7 @@ fn cluster_destroy_stops_and_removes_cluster() -> TestResult {
     for runtime in runtimes() {
         println!("{runtime:?}");
         let data_dir = tempdir::TempDir::new("data")?;
-        let cluster = Cluster::new(&data_dir, &runtime)?;
+        let cluster = Cluster::new(&data_dir, runtime)?;
         cluster.create()?;
         cluster.start()?;
         assert!(exists(&cluster));
@@ -235,7 +235,7 @@ fn cluster_connect_connects() -> TestResult {
     for runtime in runtimes() {
         println!("{runtime:?}");
         let data_dir = tempdir::TempDir::new("data")?;
-        let cluster = Cluster::new(&data_dir, &runtime)?;
+        let cluster = Cluster::new(&data_dir, runtime)?;
         cluster.start()?;
         cluster.connect("template1")?;
         cluster.destroy()?;
@@ -248,7 +248,7 @@ fn cluster_databases_returns_vec_of_database_names() -> TestResult {
     for runtime in runtimes() {
         println!("{runtime:?}");
         let data_dir = tempdir::TempDir::new("data")?;
-        let cluster = Cluster::new(&data_dir, &runtime)?;
+        let cluster = Cluster::new(&data_dir, runtime)?;
         cluster.start()?;
 
         let expected: HashSet<String> = ["postgres", "template0", "template1"]
@@ -270,7 +270,7 @@ fn cluster_databases_with_non_plain_names_can_be_created_and_dropped() -> TestRe
     for runtime in runtimes() {
         println!("{runtime:?}");
         let data_dir = tempdir::TempDir::new("data")?;
-        let cluster = Cluster::new(&data_dir, &runtime)?;
+        let cluster = Cluster::new(&data_dir, runtime)?;
         cluster.start()?;
         cluster.createdb("foo-bar")?;
         cluster.createdb("Foo-BAR")?;

src/coordinate.rs

@@ -9,7 +9,7 @@
 //! let cluster_dir = tempdir::TempDir::new("cluster")?;
 //! let data_dir = cluster_dir.path().join("data");
 //! let runtime = strategy::default();
-//! let cluster = Cluster::new(&data_dir, &runtime)?;
+//! let cluster = Cluster::new(&data_dir, runtime)?;
 //! let lock_file = cluster_dir.path().join("lock");
 //! let lock = lock::UnlockedFile::try_from(lock_file.as_path())?;
 //! assert!(coordinate::run_and_stop(&cluster, lock, cluster::exists)?);

src/lib.rs

@@ -16,7 +16,7 @@
 //! use postgresfixture::prelude::*;
 //! for runtime in strategy::default().runtimes() {
 //!   let data_dir = tempdir::TempDir::new("data")?;
-//!   let cluster = Cluster::new(&data_dir, &runtime)?;
+//!   let cluster = Cluster::new(&data_dir, runtime)?;
 //!   cluster.start()?;
 //!   assert_eq!(cluster.databases()?, vec!["postgres", "template0", "template1"]);
 //!   let mut conn = cluster.connect("template1")?;

src/main.rs

@@ -128,7 +128,7 @@ where
         .with_section(|| lock_uuid.to_string().header("UUID for lock file:"))?;
 
     let strategy = runtime::strategy::default();
-    let cluster = cluster::Cluster::new(&database_dir, &strategy)?;
+    let cluster = cluster::Cluster::new(&database_dir, strategy)?;
 
     let runner = if destroy {
         coordinate::run_and_destroy

src/runtime/strategy.rs

@@ -18,7 +18,7 @@ pub type Runtimes<'a> = Box<dyn Iterator<Item = Runtime> + 'a>;
 ///
 /// This trait models those questions, and provides default implementations for
 /// #2 and #3.
-pub trait RuntimeStrategy {
+pub trait RuntimeStrategy: std::panic::RefUnwindSafe + 'static {
     /// Find all runtimes that this strategy knows about.
     fn runtimes(&self) -> Runtimes;