Merge #418

418: `update-tester` requires two versions of `cargo-pgx` r=rtwalker a=rtwalker

Now that #417 is merged, we can change the `update-tester` crate to require two versions of `pgx` to be installed: 
 * a 0.2-0.3 series pgx aka "old"
 * a 0.4 series or newer pgx (we don't actually use this one yet, but we will after #408)

Rather than insisting on having the `pgx`s installed in a specific location, the update tester takes in paths to the `*/bin/cargo-pgx` subcommands as arguments and then inspects the `Cargo.toml` during the checkout of each release to determine which one to invoke. 

I've also updated the `tools/build` script and the `.github/workflows/ci.yml` to use the new update tester.

Went ahead and bumped the `update-tester` version to `0.2.0` so I could more easily tell the difference while testing locally, though it probably makes little practical difference.

Co-authored-by: Ryan Walker <rwalker@timescale.com>

.github/workflows/ci.yml

@@ -316,4 +316,4 @@ jobs:
         #restore-keys: ${{ runner.os }}-test-old-versions-PREVIOUS
 
     - name: Run Update Tests
-      run: su postgres -c 'sh tools/build -pg$PGVERSION test-updates 2>&1'
+      run: su postgres -c 'sh tools/build -pg$PGVERSION -cargo-pgx /pgx/0.4/bin/cargo-pgx -cargo-pgx-old /pgx/0.2/bin/cargo-pgx test-updates 2>&1'

tools/build

@@ -16,7 +16,7 @@ die() {
 }
 
 usage() {
-    die 'build [ -n -pg1[234] ] ( test-crates | test-extension | test-post-install | test-doc | clippy )'
+    die 'build [ -n -pg1[234] ] ( test-crates | test-extension | test-post-install | test-doc | test-updates | clippy)'
 }
 
 require_pg_version() {
@@ -38,6 +38,15 @@ find_pg_config() {
     [ -x "$pg_config" ] || die "$pg_config not executable"
 }
 
+require_cargo_pgx() {
+    [ -n "$cargo_pgx" ] || die 'specify path to cargo-pgx (0.4 series or newer)'
+}
+
+require_cargo_pgx_old() {
+    [ -n "$cargo_pgx_old" ] || die 'specify path to cargo-pgx (0.2-0.3 series)'
+}
+
+
 [ $# -ge 1 ] || usage
 
 # For reasons we don't yet understand, pgx prevents the cargo cache from
@@ -77,6 +86,16 @@ while [ $# -gt 0 ]; do
             shift
             ;;
 
+        -cargo-pgx)
+            cargo_pgx="$1"
+            shift
+            ;;
+
+        -cargo-pgx-old)
+            cargo_pgx_old="$1"
+            shift
+            ;;
+
         -pgport)
             pg_port="$1"
             shift
@@ -139,6 +158,8 @@ while [ $# -gt 0 ]; do
         test-updates)
             require_pg_version
             find_pg_config
+            require_cargo_pgx
+            require_cargo_pgx_old
             (
                 export CARGO_TARGET_DIR="$top_CARGO_TARGET_DIR"
                 $nop cargo pgx start $pg
@@ -147,7 +168,9 @@ while [ $# -gt 0 ]; do
                  -p $pg_port \
                  --cache old-versions \
                  . \
-                 "$pg_config"
+                 "$pg_config" \
+                 "$cargo_pgx" \
+                 "$cargo_pgx_old"
             )
             ;;
 

tools/update-tester/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "update-tester"
-version = "0.1.0"
+version = "0.2.0"
 edition = "2021"
 
 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
@@ -12,4 +12,6 @@ postgres_connection_configuration = {path = "../../crates/scripting-utilities/po
 colored = "2.0.0"
 clap = { version = "2.33", features = ["wrap_help"] }
 postgres = "0.19.1"
+semver = "1.0.9"
+toml_edit = "0.14.3"
 xshell = "0.1.14"

tools/update-tester/Readme.md

@@ -25,7 +25,7 @@ the current version work correctly. At a high level:
 
 ```
 USAGE:
-    update-tester [OPTIONS] <dir> <pg_config>
+    update-tester [OPTIONS] <dir> <pg_config> <cargo_pgx> <cargo_pgx_old>
 
 FLAGS:
         --help       Prints help information
@@ -41,6 +41,8 @@ OPTIONS:
     -u, --user <username>        postgres user
 
 ARGS:
-    <dir>          Path in which to find the timescaledb-toolkit repo
-    <pg_config>    Path to pg_config for the DB we are using
-```
+    <dir>              Path in which to find the timescaledb-toolkit repo
+    <pg_config>        Path to pg_config for the DB we are using
+    <cargo_pgx>        Path to cargo-pgx (must be 0.4 series or newer)
+    <cargo_pgx_old>    Path to cargo-pgx 0.2-0.3 series
+```

tools/update-tester/src/installer.rs

@@ -1,27 +1,31 @@
-use std::{
-    collections::HashSet,
-    path::Path,
-};
+use std::{collections::HashSet, path::Path};
 
 use colored::Colorize;
-
+use semver::Version;
+use toml_edit::Document;
 use xshell::{cmd, cp, mkdir_p, pushd, pushenv, read_dir};
 
 use crate::{defer, quietly_run};
 
+#[allow(clippy::too_many_arguments)]
 pub fn install_all_versions(
     root_dir: &str,
     cache_dir: Option<&str>,
     pg_config: &str,
+    cargo_pgx: &str,
+    cargo_pgx_old: &str,
     current_version: &str,
     old_versions: &[String],
     reinstall: &HashSet<&str>,
 ) -> xshell::Result<()> {
     let extension_dir = path!(root_dir / "extension");
-    let install_toolkit = || -> xshell::Result<()> {
+    let install_toolkit = |pgx_version: Version| -> xshell::Result<()> {
         let _d = pushd(&extension_dir)?;
         let _e = pushenv("CARGO_TARGET_DIR", "../target/extension");
-        quietly_run(cmd!("cargo pgx install -c {pg_config}"))
+        match pgx_version >= Version::new(0, 4, 0) {
+            true => quietly_run(cmd!("{cargo_pgx} pgx install -c {pg_config}")),
+            false => quietly_run(cmd!("{cargo_pgx_old} pgx install -c {pg_config}")),
+        }
     };
     let post_install = || -> xshell::Result<()> {
         let _d = pushd(root_dir)?;
@@ -35,32 +39,46 @@ pub fn install_all_versions(
         restore_from_cache(cache_dir, pg_config)?
     }
 
-    let base_checkout = get_current_checkout()?;
-    // Install the versions in reverse-time order.
-    // Since later versions tend to be supersets of old versions,
-    // I expect compilation to be faster this way - Josh
-    for version in old_versions.iter().rev() {
-        let force_reinstall = reinstall.contains(&**version);
-        if !force_reinstall && version_is_installed(pg_config, version)? {
-            eprintln!("{} {}", "Already Installed".blue(), version);
-            continue
+    {
+        let base_checkout = get_current_checkout()?;
+        let pgx_version = get_pgx_version(
+            &std::fs::read_to_string("extension/Cargo.toml").expect("unable to read Cargo.toml"),
+        );
+        // Install the versions in reverse-time order.
+        // Since later versions tend to be supersets of old versions,
+        // I expect compilation to be faster this way - Josh
+        for version in old_versions.iter().rev() {
+            let force_reinstall = reinstall.contains(&**version);
+            if !force_reinstall && version_is_installed(pg_config, version)? {
+                eprintln!("{} {}", "Already Installed".blue(), version);
+                continue;
+            }
+            eprintln!("{} {}", "Installing".bold().cyan(), version);
+            let tag_version = tag_version(version);
+            quietly_run(cmd!("git fetch origin tag {tag_version}"))?;
+            quietly_run(cmd!("git checkout tags/{tag_version}"))?;
+            let _d = defer(|| quietly_run(cmd!("git checkout {base_checkout}")));
+            let pgx_version = get_pgx_version(
+                &std::fs::read_to_string("extension/Cargo.toml")
+                    .expect("unable to read Cargo.toml"),
+            );
+            install_toolkit(pgx_version)?;
+            post_install()?;
+            eprintln!("{} {}", "Finished".bold().green(), version);
         }
-        eprintln!("{} {}", "Installing".bold().cyan(), version);
-        let tag_version = tag_version(version);
-        quietly_run(cmd!("git fetch origin tag {tag_version}"))?;
-        quietly_run(cmd!("git checkout tags/{tag_version}"))?;
-        let _d = defer(|| quietly_run(cmd!("git checkout {base_checkout}")));
-        install_toolkit()?;
-        post_install()?;
-        eprintln!("{} {}", "Finished".bold().green(), version);
-    }
 
-    if let Some(cache_dir) = cache_dir {
-        save_to_cache(cache_dir, pg_config)?;
-    }
+        if let Some(cache_dir) = cache_dir {
+            save_to_cache(cache_dir, pg_config)?;
+        }
 
-    eprintln!("{} {} ({})", "Installing Current".bold().cyan(), current_version, base_checkout);
-    install_toolkit()?;
+        eprintln!(
+            "{} {} ({})",
+            "Installing Current".bold().cyan(),
+            current_version,
+            base_checkout
+        );
+        install_toolkit(pgx_version)?;
+    }
     post_install()?;
     eprintln!("{}", "Finished Current".bold().green());
 
@@ -77,6 +95,18 @@ fn get_current_checkout() -> xshell::Result<String> {
     cmd!("git rev-parse --verify HEAD").read()
 }
 
+fn get_pgx_version(cargo_toml_contents: &str) -> Version {
+    let cargo = cargo_toml_contents
+        .parse::<Document>()
+        .expect("invalid Cargo.toml");
+
+    cargo["dependencies"]["pgx"]
+        .as_str()
+        .expect("expected pgx to only have a version")
+        .parse()
+        .expect("cannot parse pgx version")
+}
+
 // We were unprincipled with some of our old versions, so the version from
 // the control file is `x.y`, while the tag is `x.y.0`. This function translates
 // from the control file version to the tag version (in a rather hacky way)

tools/update-tester/src/main.rs

@@ -48,6 +48,8 @@ fn main() {
         (@arg REINSTALL: --reinstall [versions] "comma-separated list of versions to force reinstall")
         (@arg ROOT_DIR: +required <dir> "Path in which to find the timescaledb-toolkit repo")
         (@arg PG_CONFIG: +required <pg_config> "Path to pg_config for the DB we are using")
+        (@arg CARGO_PGX: +required <cargo_pgx> "Path to cargo-pgx (must be 0.4 series or newer)")
+        (@arg CARGO_PGX_OLD: +required <cargo_pgx_old> "Path to cargo-pgx 0.2-0.3 series")
     )
     .get_matches();
 
@@ -71,8 +73,20 @@ fn main() {
         .unwrap_or_else(HashSet::new);
 
     let pg_config = matches.value_of("PG_CONFIG").expect("missing pg_config");
+    let cargo_pgx = matches.value_of("CARGO_PGX").expect("missing cargo_pgx");
+    let cargo_pgx_old = matches
+        .value_of("CARGO_PGX_OLD")
+        .expect("missing cargo_pgx_old");
 
-    let res = try_main(root_dir, cache_dir, &connection_config, pg_config, reinstall);
+    let res = try_main(
+        root_dir,
+        cache_dir,
+        &connection_config,
+        pg_config,
+        cargo_pgx,
+        cargo_pgx_old,
+        reinstall,
+    );
     if let Err(err) = res {
         eprintln!("{}", err);
         process::exit(1);
@@ -84,6 +98,8 @@ fn try_main(
     cache_dir: Option<&str>,
     db_conn: &ConnectionConfig<'_>,
     pg_config: &str,
+    cargo_pgx: &str,
+    cargo_pgx_old: &str,
     reinstall: HashSet<&str>,
 ) -> xshell::Result<()> {
     let (current_version, old_versions) = get_version_info(root_dir)?;
@@ -97,6 +113,8 @@ fn try_main(
         root_dir,
         cache_dir,
         pg_config,
+        cargo_pgx,
+        cargo_pgx_old,
         &current_version,
         &old_versions,
         &reinstall,