Make the pgversion optional when set in features. (#402)

* Remove requirement for pgversion Signed-off-by: Ana Hobden <[email protected]> * Refactor a bit Signed-off-by: Ana Hobden <[email protected]> * fmt Signed-off-by: Ana Hobden <[email protected]> * Improve feature flag handling Signed-off-by: Ana Hobden <[email protected]> * Refine test feature handling Signed-off-by: Ana Hobden <[email protected]> * Refine flag usage in test Signed-off-by: Ana Hobden <[email protected]> * Let invalid pgversion be testname Signed-off-by: Ana Hobden <[email protected]> * Ensure pg_test is on tests Signed-off-by: Ana Hobden <[email protected]> * Remove leftover debug panic Signed-off-by: Ana Hobden <[email protected]> * Tidy docs and consistency in commands Signed-off-by: Ana Hobden <[email protected]> * Add remaining readme bits Signed-off-by: Ana Hobden <[email protected]> * fix pgx connect requiring manifest Signed-off-by: Ana Hobden <[email protected]>
pgcentralfoundation · Feb 3, 2022 · f884a35 · f884a35
1 parent 400a132
commit f884a35
Show file tree

Hide file tree

Showing 15 changed files with 617 additions and 381 deletions.
diff --git a/cargo-pgx/Cargo.toml b/cargo-pgx/Cargo.toml
@@ -13,7 +13,6 @@ keywords = ["database", "postgres", "postgresql", "extension"]
 readme = "README.md"
 exclude = [ "*.png" ]
 
-
 [dependencies]
 cargo_metadata = "0.14.1"
 cargo_toml = "0.11.3"

diff --git a/cargo-pgx/README.md b/cargo-pgx/README.md
diff --git a/cargo-pgx/src/command/connect.rs b/cargo-pgx/src/command/connect.rs
@@ -16,7 +16,7 @@ use pgx_utils::pg_config::{PgConfig, Pgx};
 pub(crate) struct Connect {
     /// Do you want to run against Postgres `pg10`, `pg11`, `pg12`, `pg13`, `pg14`?
     #[clap(env = "PG_VERSION")]
-    pg_version: String,
+    pg_version: Option<String>,
     /// The database to connect to (and create if the first time).  Defaults to a database with the same name as the current extension name
     #[clap(env = "DBNAME")]
     dbname: Option<String>,
@@ -26,14 +26,48 @@ pub(crate) struct Connect {
 
 impl CommandExecute for Connect {
     #[tracing::instrument(level = "error", skip(self))]
-    fn execute(self) -> eyre::Result<()> {
+    fn execute(mut self) -> eyre::Result<()> {
+        let pgx = Pgx::from_config()?;
+
+        let pg_version = match self.pg_version {
+            Some(pg_version) => match pgx.get(&pg_version) {
+                Ok(_) => pg_version,
+                Err(err) => {
+                    if self.dbname.is_some() {
+                        return Err(err);
+                    }
+                    // It's actually the dbname! We should infer from the manifest.
+                    self.dbname = Some(pg_version);
+
+                    let metadata = crate::metadata::metadata(&Default::default())?;
+                    crate::metadata::validate(&metadata)?;
+                    let manifest = crate::manifest::manifest(&metadata)?;
+
+                    let default_pg_version = crate::manifest::default_pg_version(&manifest)
+                        .ok_or(eyre!("No provided `pg$VERSION` flag."))?;
+                    default_pg_version
+                },
+            },
+            None => {
+                // We should infer from the manifest.
+                let metadata = crate::metadata::metadata(&Default::default())?;
+                crate::metadata::validate(&metadata)?;
+                let manifest = crate::manifest::manifest(&metadata)?;
+
+                let default_pg_version = crate::manifest::default_pg_version(&manifest)
+                    .ok_or(eyre!("No provided `pg$VERSION` flag."))?;
+                default_pg_version
+            }
+        };
+
         let dbname = match self.dbname {
             Some(dbname) => dbname,
             None => get_property("extname")
                 .wrap_err("could not determine extension name")?
                 .ok_or(eyre!("extname not found in control file"))?,
         };
-        connect_psql(Pgx::from_config()?.get(&self.pg_version)?, &dbname)
+
+        connect_psql(Pgx::from_config()?.get(&pg_version)?, &dbname)
     }
 }
 

diff --git a/cargo-pgx/src/command/install.rs b/cargo-pgx/src/command/install.rs
@@ -9,7 +9,7 @@ use cargo_metadata::MetadataCommand;
 use colored::Colorize;
 use eyre::{eyre, WrapErr};
 use pgx_utils::get_target_dir;
-use pgx_utils::pg_config::{PgConfig, Pgx};
+use pgx_utils::pg_config::PgConfig;
 use std::path::PathBuf;
 use std::process::{Command, Stdio};
 
@@ -35,37 +35,26 @@ pub(crate) struct Install {
 impl CommandExecute for Install {
     #[tracing::instrument(level = "error", skip(self))]
     fn execute(self) -> eyre::Result<()> {
-        let pg_config =
-            match std::env::var("PGX_TEST_MODE_VERSION") {
-                // for test mode, we want the pg_config specified in PGX_TEST_MODE_VERSION
-                Ok(pgver) => match Pgx::from_config()?.get(&pgver) {
-                    Ok(pg_config) => pg_config.clone(),
-                    Err(e) => return Err(e).wrap_err(
-                        "PGX_TEST_MODE_VERSION does not contain a valid postgres version number",
-                    ),
-                },
-
-                // otherwise, the user just ran "cargo pgx install", and we use whatever "pg_config" is configured
-                Err(_) => match self.pg_config {
-                    None => PgConfig::from_path(),
-                    Some(config) => PgConfig::new(PathBuf::from(config)),
-                },
-            };
-
-        install_extension(
-            &pg_config,
-            self.release,
-            self.no_schema,
-            None,
-            &self.features,
-        )
+        let metadata = crate::metadata::metadata(&self.features)?;
+        crate::metadata::validate(&metadata)?;
+        let manifest = crate::manifest::manifest(&metadata)?;
+
+        let pg_config = match self.pg_config {
+            None => PgConfig::from_path(),
+            Some(config) => PgConfig::new(PathBuf::from(config)),
+        };
+        let pg_version = format!("pg{}", pg_config.major_version()?);
+        let features = crate::manifest::features_for_version(self.features, &manifest, &pg_version);
+
+        install_extension(&pg_config, self.release, self.no_schema, None, &features)
     }
 }
 
 #[tracing::instrument(skip_all, fields(
     pg_version = %pg_config.version()?,
     release = is_release,
     base_directory = tracing::field::Empty,
+    features = ?features.features,
 ))]
 pub(crate) fn install_extension(
     pg_config: &PgConfig,
@@ -81,7 +70,6 @@ pub(crate) fn install_extension(
     );
 
     let (control_file, extname) = find_control_file()?;
-    let major_version = pg_config.major_version()?;
 
     if get_property("relocatable")? != Some("false".into()) {
         return Err(eyre!(
@@ -90,7 +78,7 @@ pub(crate) fn install_extension(
         ));
     }
 
-    build_extension(major_version, is_release, &features)?;
+    build_extension(is_release, &features)?;
 
     println!();
     println!("installing extension");
@@ -169,37 +157,24 @@ fn copy_file(src: &PathBuf, dest: &PathBuf, msg: &str, do_filter: bool) -> eyre:
 }
 
 pub(crate) fn build_extension(
-    major_version: u16,
     is_release: bool,
     features: &clap_cargo::Features,
 ) -> eyre::Result<()> {
-    let pgx_build_features =
-        std::env::var("PGX_BUILD_FEATURES").unwrap_or(format!("pg{}", major_version));
     let flags = std::env::var("PGX_BUILD_FLAGS").unwrap_or_default();
 
-    let features_arg = if !features.features.is_empty() {
-        use std::fmt::Write;
-        let mut additional_features = features.features.join(" ");
-        let _ = write!(&mut additional_features, " {}", pgx_build_features);
-        additional_features
-    } else {
-        pgx_build_features
-    };
-
     let mut command = Command::new("cargo");
     command.arg("build");
     if is_release {
         command.arg("--release");
     }
 
+    let features_arg = features.features.join(" ");
     if !features_arg.trim().is_empty() {
         command.arg("--features");
         command.arg(&features_arg);
-        // While this is not 'correct' `cargo` does not let us negate features.
-        command.arg("--no-default-features");
     }
 
-    if features.no_default_features && features_arg.trim().is_empty() {
+    if features.no_default_features {
         command.arg("--no-default-features");
     }
 

diff --git a/cargo-pgx/src/command/package.rs b/cargo-pgx/src/command/package.rs
@@ -28,11 +28,18 @@ pub(crate) struct Package {
 impl CommandExecute for Package {
     #[tracing::instrument(level = "error", skip(self))]
     fn execute(self) -> eyre::Result<()> {
+        let metadata = crate::metadata::metadata(&self.features)?;
+        crate::metadata::validate(&metadata)?;
+        let manifest = crate::manifest::manifest(&metadata)?;
+
         let pg_config = match self.pg_config {
             None => PgConfig::from_path(),
             Some(config) => PgConfig::new(PathBuf::from(config)),
         };
-        package_extension(&pg_config, self.debug, &self.features)
+        let pg_version = format!("pg{}", pg_config.major_version()?);
+        let features = crate::manifest::features_for_version(self.features, &manifest, &pg_version);
+
+        package_extension(&pg_config, self.debug, &features)
     }
 }
 

diff --git a/cargo-pgx/src/command/run.rs b/cargo-pgx/src/command/run.rs
@@ -20,7 +20,7 @@ use std::{os::unix::process::CommandExt, process::Command};
 pub(crate) struct Run {
     /// Do you want to run against Postgres `pg10`, `pg11`, `pg12`, `pg13`, `pg14`?
     #[clap(env = "PG_VERSION")]
-    pg_version: String,
+    pg_version: Option<String>,
     /// The database to connect to (and create if the first time).  Defaults to a database with the same name as the current extension name
     dbname: Option<String>,
     /// Compile for release mode (default is debug)
@@ -37,18 +37,48 @@ pub(crate) struct Run {
 
 impl CommandExecute for Run {
     #[tracing::instrument(level = "error", skip(self))]
-    fn execute(self) -> eyre::Result<()> {
+    fn execute(mut self) -> eyre::Result<()> {
+        let metadata = crate::metadata::metadata(&self.features)?;
+        crate::metadata::validate(&metadata)?;
+        let manifest = crate::manifest::manifest(&metadata)?;
+        let pgx = Pgx::from_config()?;
+
+        let (pg_config, pg_version) = match self.pg_version {
+            Some(pg_version) => {
+                match pgx.get(&pg_version) {
+                    Ok(pg_config) => (pg_config, pg_version),
+                    Err(err) => {
+                        if self.dbname.is_some() {
+                            return Err(err);
+                        }
+                        // It's actually the dbname! We should infer from the manifest.
+                        self.dbname = Some(pg_version);
+                        let default_pg_version = crate::manifest::default_pg_version(&manifest)
+                            .ok_or(eyre!("No provided `pg$VERSION` flag."))?;
+                        (pgx.get(&default_pg_version)?, default_pg_version)
+                    },
+                }
+            },
+            None => {
+                // We should infer from the manifest.
+                let default_pg_version = crate::manifest::default_pg_version(&manifest)
+                    .ok_or(eyre!("No provided `pg$VERSION` flag."))?;
+                (pgx.get(&default_pg_version)?, default_pg_version)
+            },
+        };
+        let features = crate::manifest::features_for_version(self.features, &manifest, &pg_version);
+
         let dbname = match self.dbname {
             Some(dbname) => dbname,
             None => get_property("extname")?.ok_or(eyre!("could not determine extension name"))?,
         };
-
+        
         run_psql(
-            Pgx::from_config()?.get(&self.pg_version)?,
+            pg_config,
             &dbname,
             self.release,
             self.no_schema,
-            &self.features,
+            &features,
         )
     }
 }

diff --git a/cargo-pgx/src/command/schema.rs b/cargo-pgx/src/command/schema.rs
@@ -59,6 +59,10 @@ impl CommandExecute for Schema {
     #[tracing::instrument(level = "error", skip(self))]
     fn execute(self) -> eyre::Result<()> {
         let (_, extname) = crate::command::get::find_control_file()?;
+        let metadata = crate::metadata::metadata(&Default::default())?;
+        crate::metadata::validate(&metadata)?;
+        let manifest = crate::manifest::manifest(&metadata)?;
+
         let out = match self.out {
             Some(out) => out,
             None => format!(
@@ -80,30 +84,31 @@ impl CommandExecute for Schema {
             }
         };
 
-        let pg_config =
-            match std::env::var("PGX_TEST_MODE_VERSION") {
-                // for test mode, we want the pg_config specified in PGX_TEST_MODE_VERSION
-                Ok(pgver) => match Pgx::from_config()?.get(&pgver) {
-                    Ok(pg_config) => pg_config.clone(),
-                    Err(e) => return Err(e).wrap_err(
-                        "PGX_TEST_MODE_VERSION does not contain a valid postgres version number",
-                    ),
-                },
+        let (pg_config, pg_version) = match self.pg_config {
+            None => match self.pg_version {
+                None => {
+                    let pg_version = match self.pg_version {
+                        Some(s) => s,
+                        None => crate::manifest::default_pg_version(&manifest)
+                            .ok_or(eyre!("No provided `pg$VERSION` flag."))?,
+                    };
+                    (Pgx::from_config()?.get(&pg_version)?.clone(), pg_version)
+                }
+                Some(pgver) => (Pgx::from_config()?.get(&pgver)?.clone(), pgver),
+            },
+            Some(config) => {
+                let pg_config = PgConfig::new(PathBuf::from(config));
+                let pg_version = format!("pg{}", pg_config.major_version()?);
+                (pg_config, pg_version)
+            }
+        };
 
-                // otherwise, the user just ran "cargo pgx install", and we use whatever "pg_config" is configured
-                Err(_) => match self.pg_config {
-                    None => match self.pg_version {
-                        None => PgConfig::from_path(),
-                        Some(pgver) => Pgx::from_config()?.get(&pgver)?.clone(),
-                    },
-                    Some(config) => PgConfig::new(PathBuf::from(config)),
-                },
-            };
+        let features = crate::manifest::features_for_version(self.features, &manifest, &pg_version);
 
         generate_schema(
             &pg_config,
             self.release,
-            &self.features,
+            &features,
             &out,
             self.dot,
             log_level,
@@ -132,10 +137,7 @@ pub(crate) fn generate_schema(
     manual: bool,
     skip_build: bool,
 ) -> eyre::Result<()> {
-    crate::validate::validate(&features)?;
-
     let (control_file, _extname) = find_control_file()?;
-    let major_version = pg_config.major_version()?;
 
     // If not manual, we should ensure a few files exist and are what is expected.
     if !manual {
@@ -191,17 +193,6 @@ pub(crate) fn generate_schema(
         ));
     }
 
-    let pgx_build_features =
-        std::env::var("PGX_BUILD_FEATURES").unwrap_or(format!("pg{}", major_version));
-    let features_arg = if !features.features.is_empty() {
-        use std::fmt::Write;
-        let mut additional_features = features.features.join(" ");
-        let _ = write!(&mut additional_features, " {}", pgx_build_features);
-        additional_features
-    } else {
-        pgx_build_features
-    };
-
     let flags = std::env::var("PGX_BUILD_FLAGS").unwrap_or_default();
 
     if !skip_build {
@@ -216,14 +207,13 @@ pub(crate) fn generate_schema(
             command.env("RUST_LOG", log_level);
         }
 
+        let features_arg = features.features.join(" ");
         if !features_arg.trim().is_empty() {
             command.arg("--features");
             command.arg(&features_arg);
-            // While this is not 'correct' `cargo` does not let us negate features.
-            command.arg("--no-default-features");
         }
 
-        if features.no_default_features && features_arg.trim().is_empty() {
+        if features.no_default_features {
             command.arg("--no-default-features");
         }