From 3f4a93153273c6125e38c6907ffe36dcb0c24c8b Mon Sep 17 00:00:00 2001 From: danielshih Date: Fri, 26 Jun 2026 01:46:15 +0000 Subject: [PATCH 1/2] Add --cargo passthrough for cargo-pgrx install/package/run/test #2135 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Monorepo/submodule layouts fail because cargo resolves .cargo/config.toml from the CWD upward, so building from the parent repo picks up the wrong config and cargo metadata (the first cargo call) dies — e.g. a private registry declared only in the submodule's config. Add a repeatable --cargo that is forwarded verbatim to every cargo invocation cargo-pgrx makes (cargo metadata, cargo rustc, get_version, cargo test), so global flags like --config/--offline/--locked reach all of them: cargo pgrx install --cargo=--config=child/.cargo/config.toml --- cargo-pgrx/src/cargo.rs | 2 +- cargo-pgrx/src/command/bench.rs | 31 +++++- cargo-pgrx/src/command/connect.rs | 1 + cargo-pgrx/src/command/get.rs | 2 +- cargo-pgrx/src/command/install.rs | 48 ++++++--- cargo-pgrx/src/command/package.rs | 12 ++- cargo-pgrx/src/command/regress.rs | 1 + cargo-pgrx/src/command/run.rs | 8 ++ cargo-pgrx/src/command/schema.rs | 1 + cargo-pgrx/src/command/start.rs | 2 + cargo-pgrx/src/command/stop.rs | 2 + cargo-pgrx/src/command/sudo_install.rs | 3 + cargo-pgrx/src/command/test.rs | 14 ++- cargo-pgrx/src/manifest.rs | 3 +- cargo-pgrx/src/metadata.rs | 47 ++++++++- cargo-pgrx/tests/cargo_flags_passthrough.rs | 102 ++++++++++++++++++++ 16 files changed, 260 insertions(+), 19 deletions(-) create mode 100644 cargo-pgrx/tests/cargo_flags_passthrough.rs diff --git a/cargo-pgrx/src/cargo.rs b/cargo-pgrx/src/cargo.rs index 65e3fbb670..1afe2a3e91 100644 --- a/cargo-pgrx/src/cargo.rs +++ b/cargo-pgrx/src/cargo.rs @@ -152,7 +152,7 @@ impl Cargo { cmd.arg(features.features.join(" ")); } - // And now the miscellaneous build flags! + // And now the miscellaneous build flags!, `PGRX_BUILD_FLAGS` stays build-only. let flags = env::var("PGRX_BUILD_FLAGS").unwrap_or_default(); for arg in flags.split_ascii_whitespace() { cmd.arg(arg); diff --git a/cargo-pgrx/src/command/bench.rs b/cargo-pgrx/src/command/bench.rs index 17dc021c28..70d97e40d9 100644 --- a/cargo-pgrx/src/command/bench.rs +++ b/cargo-pgrx/src/command/bench.rs @@ -82,6 +82,9 @@ pub(crate) struct Bench { features: clap_cargo::Features, #[clap(long)] target: Option, + /// Extra cargo flags forwarded to every `cargo` invocation. Repeatable and split on whitespace: `--cargo=--config=foo` or `--cargo "--offline --frozen"`. + #[clap(long = "cargo", value_name = "FLAG", allow_hyphen_values = true)] + cargo: Vec, #[clap(from_global, action = clap::ArgAction::Count)] verbose: u8, /// Custom `postgresql.conf` settings in the form of `key=value` @@ -99,6 +102,7 @@ impl CommandExecute for Bench { &self.features, self.package.as_deref(), self.manifest_path.as_deref(), + &self.cargo, )?; let mut resolved_features = self.features.clone(); let (pg_config, _) = pg_config_and_version( @@ -117,7 +121,7 @@ impl CommandExecute for Bench { } let postgresql_conf = collect_postgresql_conf_settings(&self.postgresql_conf)?; - let extversion = crate::command::install::get_version(&package_manifest_path)?; + let extversion = crate::command::install::get_version(&package_manifest_path, &self.cargo)?; let mut features = resolved_features; ensure_feature(&mut features, "pg_bench"); let profile = CargoProfile::from_flags( @@ -138,6 +142,7 @@ impl CommandExecute for Bench { false, self.target.as_deref(), &postgresql_conf, + &self.cargo, )?; if self.resetdb { @@ -2302,4 +2307,28 @@ mod tests { assert_eq!(format_wait_duration(1), "1 second"); assert_eq!(format_wait_duration(2), "2 seconds"); } + + #[test] + fn cargo_flag_defaults_to_empty() { + let bench = parse_bench(&["cargo", "pgrx", "bench"]); + assert!(bench.cargo.is_empty()); + } + + #[test] + fn cargo_flag_is_repeatable() { + let bench = parse_bench(&[ + "cargo", + "pgrx", + "bench", + "--cargo=--offline", + "--cargo", + "--config foo", + ]); + assert_eq!(bench.cargo, vec!["--offline".to_string(), "--config foo".to_string()]); + // and the shared splitter tokenizes each value the same way build/metadata will + assert_eq!( + crate::metadata::split_cargo_flags(&bench.cargo), + vec!["--offline", "--config", "foo"] + ); + } } diff --git a/cargo-pgrx/src/command/connect.rs b/cargo-pgrx/src/command/connect.rs index 33c4bda262..70958225de 100644 --- a/cargo-pgrx/src/command/connect.rs +++ b/cargo-pgrx/src/command/connect.rs @@ -52,6 +52,7 @@ impl CommandExecute for Connect { &Features::default(), self.package.as_deref(), self.manifest_path.as_deref(), + &[], )?; let (pg_config, _pg_version) = match pg_config_and_version( &pgrx, diff --git a/cargo-pgrx/src/command/get.rs b/cargo-pgrx/src/command/get.rs index 1a9b43971c..274e848416 100644 --- a/cargo-pgrx/src/command/get.rs +++ b/cargo-pgrx/src/command/get.rs @@ -34,7 +34,7 @@ impl CommandExecute for Get { #[tracing::instrument(level = "error", skip(self))] fn execute(self) -> eyre::Result<()> { let metadata = - crate::metadata::metadata(&Default::default(), self.manifest_path.as_deref()) + crate::metadata::metadata(&Default::default(), self.manifest_path.as_deref(), &[]) .wrap_err("couldn't get cargo metadata")?; crate::metadata::validate(self.manifest_path.as_deref(), &metadata)?; let package_manifest_path = diff --git a/cargo-pgrx/src/command/install.rs b/cargo-pgrx/src/command/install.rs index c4cd6cd07c..88fbfebd34 100644 --- a/cargo-pgrx/src/command/install.rs +++ b/cargo-pgrx/src/command/install.rs @@ -57,6 +57,9 @@ pub(crate) struct Install { pub(crate) features: clap_cargo::Features, #[clap(long)] pub(crate) target: Option, + /// Extra cargo flags forwarded to every `cargo` invocation. Repeatable and split on whitespace: `--cargo=--config=foo` or `--cargo "--offline --frozen"`. + #[clap(long = "cargo", value_name = "FLAG", allow_hyphen_values = true)] + pub(crate) cargo: Vec, #[clap(from_global, action = ArgAction::Count)] pub(crate) verbose: u8, } @@ -72,8 +75,10 @@ impl CommandExecute for Install { return sudo_install.execute(); } - let metadata = crate::metadata::metadata(&self.features, self.manifest_path.as_deref()) - .wrap_err("couldn't get cargo metadata")?; + let cargo_flags = std::mem::take(&mut self.cargo); + let metadata = + crate::metadata::metadata(&self.features, self.manifest_path.as_deref(), &cargo_flags) + .wrap_err("couldn't get cargo metadata")?; crate::metadata::validate(self.manifest_path.as_deref(), &metadata)?; let package_manifest_path = crate::manifest::manifest_path(&metadata, self.package.as_deref()) @@ -110,6 +115,7 @@ impl CommandExecute for Install { None, &self.features, self.target.as_deref(), + &cargo_flags, )?; Ok(()) } @@ -147,6 +153,7 @@ pub(crate) fn install_extension( base_directory: Option, features: &clap_cargo::Features, target: Option<&str>, + cargo_flags: &[String], ) -> eyre::Result> { let mut output_tracking = Vec::new(); @@ -158,7 +165,7 @@ pub(crate) fn install_extension( let build_manifest_path = manifest_path_for_build(user_manifest_path, user_package, package_manifest_path); let build_command_output = - build_extension(build_manifest_path, user_package, profile, features, target)?; + build_extension(build_manifest_path, user_package, profile, features, target, cargo_flags)?; let build_command_bytes = build_command_output.stdout; let build_command_reader = BufReader::new(build_command_bytes.as_slice()); let build_command_stream = CargoMessage::parse_stream(build_command_reader); @@ -192,12 +199,13 @@ pub(crate) fn install_extension( true, package_manifest_path, &mut output_tracking, + cargo_flags, )?; } { let so_name = if versioned_so { - let extver = get_version(package_manifest_path)?; + let extver = get_version(package_manifest_path, cargo_flags)?; // note: versioned so-name format must agree with pgrx-utils format!("{extname}-{extver}") } else { @@ -234,6 +242,7 @@ pub(crate) fn install_extension( false, package_manifest_path, &mut output_tracking, + cargo_flags, )?; } @@ -248,6 +257,7 @@ pub(crate) fn install_extension( &extdir, true, &mut output_tracking, + cargo_flags, )?; println!("{} installing {}", " Finished".bold().green(), extname); @@ -261,6 +271,7 @@ fn copy_file( do_filter: bool, package_manifest_path: &Path, output_tracking: &mut Vec, + cargo_flags: &[String], ) -> eyre::Result<()> { let Some(dest_dir) = dest.parent() else { // what fresh hell could ever cause such an error? @@ -282,7 +293,7 @@ fn copy_file( // we want to filter the contents of the file we're to copy let input = fs::read_to_string(src) .wrap_err_with(|| format!("failed to read `{}`", src.display()))?; - let input = filter_contents(package_manifest_path, input)?; + let input = filter_contents(package_manifest_path, input, cargo_flags)?; fs::write(&dest, input).wrap_err_with(|| { format!("failed writing `{}` to `{}`", src.display(), dest.display()) @@ -303,6 +314,7 @@ pub(crate) fn build_extension( profile: &CargoProfile, features: &clap_cargo::Features, target: Option<&str>, + cargo_flags: &[String], ) -> eyre::Result { let flags = std::env::var("PGRX_BUILD_FLAGS").unwrap_or_default(); @@ -341,6 +353,10 @@ pub(crate) fn build_extension( command.arg(arg); } + for arg in crate::metadata::split_cargo_flags(cargo_flags) { + command.arg(arg); + } + if let Some(target) = target { command.arg("--target"); command.arg(target); @@ -388,10 +404,11 @@ fn copy_sql_files( extdir: &Path, skip_build: bool, output_tracking: &mut Vec, + cargo_flags: &[String], ) -> eyre::Result<()> { let (_, extname) = find_control_file(package_manifest_path)?; { - let version = get_version(package_manifest_path)?; + let version = get_version(package_manifest_path, cargo_flags)?; let filename = format!("{extname}--{version}.sql"); let dest = extdir.join(filename); @@ -432,6 +449,7 @@ fn copy_sql_files( true, package_manifest_path, output_tracking, + cargo_flags, )?; } } @@ -482,7 +500,7 @@ pub(crate) fn find_library_file( static CARGO_VERSION: OnceLock = OnceLock::new(); -pub(crate) fn get_version(manifest_path: &Path) -> eyre::Result { +pub(crate) fn get_version(manifest_path: &Path, cargo_flags: &[String]) -> eyre::Result { let path_string = manifest_path.to_owned(); if let Some(version) = @@ -494,8 +512,12 @@ pub(crate) fn get_version(manifest_path: &Path) -> eyre::Result { let version = match get_property(manifest_path, "default_version")? { Some(v) => { if v == "@CARGO_VERSION@" { - let metadata = crate::metadata::metadata(&Default::default(), Some(manifest_path)) - .wrap_err("couldn't get cargo metadata")?; + let metadata = crate::metadata::metadata( + &Default::default(), + Some(manifest_path), + cargo_flags, + ) + .wrap_err("couldn't get cargo metadata")?; crate::metadata::validate(Some(manifest_path), &metadata)?; let manifest_path = crate::manifest::manifest_path(&metadata, None) .wrap_err("Couldn't get manifest path")?; @@ -587,7 +609,11 @@ pub(crate) fn format_display_path(path: &Path) -> eyre::Result { Ok(out) } -fn filter_contents(manifest_path: &Path, mut input: String) -> eyre::Result { +fn filter_contents( + manifest_path: &Path, + mut input: String, + cargo_flags: &[String], +) -> eyre::Result { if input.contains("@GIT_HASH@") { // avoid doing this if we don't actually have the token // the project might not be a git repo so running `git` @@ -595,7 +621,7 @@ fn filter_contents(manifest_path: &Path, mut input: String) -> eyre::Result, + /// Extra cargo flags forwarded to every `cargo` invocation. Repeatable and split on whitespace: `--cargo=--config=foo` or `--cargo "--offline --frozen"`. + #[clap(long = "cargo", value_name = "FLAG", allow_hyphen_values = true)] + pub(crate) cargo: Vec, #[clap(from_global, action = ArgAction::Count)] pub(crate) verbose: u8, } @@ -53,8 +56,10 @@ pub(crate) struct Package { impl Package { pub(crate) fn perform(mut self) -> eyre::Result<(PathBuf, Vec)> { warn_if_pg_bench_enabled(&self.features, "package"); - let metadata = crate::metadata::metadata(&self.features, self.manifest_path.as_deref()) - .wrap_err("couldn't get cargo metadata")?; + let cargo_flags = std::mem::take(&mut self.cargo); + let metadata = + crate::metadata::metadata(&self.features, self.manifest_path.as_deref(), &cargo_flags) + .wrap_err("couldn't get cargo metadata")?; crate::metadata::validate(self.manifest_path.as_deref(), &metadata)?; let package_manifest_path = crate::manifest::manifest_path(&metadata, self.package.as_deref()) @@ -96,6 +101,7 @@ impl Package { self.test, &self.features, self.target.as_deref(), + &cargo_flags, )?; Ok((out_dir, output_files)) @@ -125,6 +131,7 @@ pub(crate) fn package_extension( is_test: bool, features: &clap_cargo::Features, target: Option<&str>, + cargo_flags: &[String], ) -> eyre::Result> { let out_dir_exists = out_dir.try_exists().wrap_err_with(|| { format!("failed to access {} while packaging extension", out_dir.display()) @@ -144,6 +151,7 @@ pub(crate) fn package_extension( Some(out_dir), features, target, + cargo_flags, ) } diff --git a/cargo-pgrx/src/command/regress.rs b/cargo-pgrx/src/command/regress.rs index 9ce4ab62bf..e79e0622d8 100644 --- a/cargo-pgrx/src/command/regress.rs +++ b/cargo-pgrx/src/command/regress.rs @@ -360,6 +360,7 @@ impl CommandExecute for Regress { &self.features, self.package.as_deref(), self.manifest_path.as_deref(), + &[], )?; let extname = get_property(&manifest_path, "extname")? .expect("extension name property `extname` should always be known"); diff --git a/cargo-pgrx/src/command/run.rs b/cargo-pgrx/src/command/run.rs index 47d066dcff..4c3d654633 100644 --- a/cargo-pgrx/src/command/run.rs +++ b/cargo-pgrx/src/command/run.rs @@ -56,6 +56,9 @@ pub(crate) struct Run { install_only: bool, #[clap(long)] valgrind: bool, + /// Extra cargo flags forwarded to every `cargo` invocation. Repeatable and split on whitespace: `--cargo=--config=foo` or `--cargo "--offline --frozen"`. + #[clap(long = "cargo", value_name = "FLAG", allow_hyphen_values = true)] + cargo: Vec, } impl From<&Regress> for Run { @@ -73,6 +76,7 @@ impl From<&Regress> for Run { pgcli: false, install_only: false, valgrind: regress.valgrind, + cargo: Vec::new(), } } } @@ -98,6 +102,7 @@ impl Run { &self.features, self.package.as_deref(), self.manifest_path.as_deref(), + &self.cargo, )?; let (pg_config, _pg_version) = pg_config_and_version( &pgrx, @@ -130,6 +135,7 @@ impl Run { self.valgrind, self.target.as_deref(), postgresql_conf, + &self.cargo, )?; Ok((pg_config, dbname)) @@ -165,6 +171,7 @@ pub(crate) fn run( use_valgrind: bool, target: Option<&str>, postgresql_conf: &HashMap, + cargo_flags: &[String], ) -> eyre::Result<()> { // stop postgres stop_postgres(pg_config)?; @@ -180,6 +187,7 @@ pub(crate) fn run( None, features, target, + cargo_flags, )?; if install_only { diff --git a/cargo-pgrx/src/command/schema.rs b/cargo-pgrx/src/command/schema.rs index d4dc6e07bd..f5df7e5743 100644 --- a/cargo-pgrx/src/command/schema.rs +++ b/cargo-pgrx/src/command/schema.rs @@ -97,6 +97,7 @@ impl CommandExecute for Schema { &self.features, self.package.as_deref(), self.manifest_path.as_deref(), + &[], )?; // This does meaningful mutation, unfortunately let (_pg_config, _pg_version) = pg_config_and_version( diff --git a/cargo-pgrx/src/command/start.rs b/cargo-pgrx/src/command/start.rs index 77a434fc7a..2d48ec84ff 100644 --- a/cargo-pgrx/src/command/start.rs +++ b/cargo-pgrx/src/command/start.rs @@ -54,6 +54,7 @@ impl CommandExecute for Start { &clap_cargo::Features::default(), me.package.as_deref(), me.manifest_path.as_deref(), + &[], )?; let (pg_config, _) = @@ -65,6 +66,7 @@ impl CommandExecute for Start { &clap_cargo::Features::default(), self.package.as_deref(), self.manifest_path.as_deref(), + &[], )?; let postgresql_conf = collect_postgresql_conf_settings(&self.postgresql_conf)?; diff --git a/cargo-pgrx/src/command/stop.rs b/cargo-pgrx/src/command/stop.rs index 72e51b5fac..e0697af6f2 100644 --- a/cargo-pgrx/src/command/stop.rs +++ b/cargo-pgrx/src/command/stop.rs @@ -41,6 +41,7 @@ impl CommandExecute for Stop { &clap_cargo::Features::default(), me.package.as_deref(), me.manifest_path.as_deref(), + &[], )?; let (pg_config, _) = pg_config_and_version(pgrx, &package_manifest, me.pg_version, None, false)?; @@ -53,6 +54,7 @@ impl CommandExecute for Stop { &clap_cargo::Features::default(), self.package.as_deref(), self.manifest_path.as_deref(), + &[], )?; if self.pg_version == Some("all".into()) { for v in crate::manifest::all_pg_in_both_tomls(&package_manifest, &pgrx) { diff --git a/cargo-pgrx/src/command/sudo_install.rs b/cargo-pgrx/src/command/sudo_install.rs index 70ed4f3882..28413cc551 100644 --- a/cargo-pgrx/src/command/sudo_install.rs +++ b/cargo-pgrx/src/command/sudo_install.rs @@ -20,6 +20,7 @@ pub(crate) struct SudoInstall { features: clap_cargo::Features, target: Option, verbose: u8, + cargo: Vec, } impl From for SudoInstall { @@ -35,6 +36,7 @@ impl From for SudoInstall { features: value.features, target: value.target, verbose: value.verbose, + cargo: value.cargo, } } } @@ -52,6 +54,7 @@ impl From for Package { features: value.features, verbose: value.verbose, target: value.target, + cargo: value.cargo, } } } diff --git a/cargo-pgrx/src/command/test.rs b/cargo-pgrx/src/command/test.rs index 9d669606f2..cc1dbe44e7 100644 --- a/cargo-pgrx/src/command/test.rs +++ b/cargo-pgrx/src/command/test.rs @@ -50,6 +50,9 @@ pub(crate) struct Test { pgdata: Option, #[clap(flatten)] features: clap_cargo::Features, + /// Extra cargo flags forwarded to every `cargo` invocation. Repeatable and split on whitespace: `--cargo=--config=foo` or `--cargo "--offline --frozen"`. + #[clap(long = "cargo", value_name = "FLAG", allow_hyphen_values = true)] + cargo: Vec, #[clap(from_global, action = clap::ArgAction::Count)] verbose: u8, } @@ -87,6 +90,7 @@ impl CommandExecute for Test { &me.testnames, me.runas, me.pgdata, + &me.cargo, )?; Ok(()) @@ -96,6 +100,7 @@ impl CommandExecute for Test { &self.features, self.package.as_deref(), self.manifest_path.as_deref(), + &self.cargo, )?; let pgrx = Pgrx::from_config()?; @@ -125,7 +130,7 @@ impl CommandExecute for Test { testnames = tracing::field::Empty, ?profile, ))] -pub fn test_extension( +pub(crate) fn test_extension( pg_config: &PgConfig, package_manifest_path: &Path, profile: &CargoProfile, @@ -134,6 +139,7 @@ pub fn test_extension( testnames: &[String], runas: Option, pgdata: Option, + cargo_flags: &[String], ) -> eyre::Result<()> { #[cfg(target_os = "windows")] if runas.is_some() { @@ -163,6 +169,12 @@ pub fn test_extension( .env("PGRX_ALL_FEATURES", if features.all_features { "true" } else { "false" }) .env("PGRX_BUILD_PROFILE", profile.name()) .env("PGRX_NO_SCHEMA", if no_schema { "true" } else { "false" }); + + // The `--cargo` passthrough reaches every cargo invocation; here, `cargo test`. + for arg in crate::metadata::split_cargo_flags(cargo_flags) { + command.arg(arg); + } + apply_resolved_manifest_to_test_command(&mut command, package_manifest_path); if let Some(runas) = runas { diff --git a/cargo-pgrx/src/manifest.rs b/cargo-pgrx/src/manifest.rs index f28777b010..2b30b25cc8 100644 --- a/cargo-pgrx/src/manifest.rs +++ b/cargo-pgrx/src/manifest.rs @@ -230,8 +230,9 @@ pub(crate) fn get_package_manifest( features: &Features, package_name: Option<&str>, manifest_path: Option<&Path>, + cargo_flags: &[String], ) -> eyre::Result<(Manifest, PathBuf)> { - let metadata = crate::metadata::metadata(features, manifest_path) + let metadata = crate::metadata::metadata(features, manifest_path, cargo_flags) .wrap_err("couldn't get cargo metadata")?; crate::metadata::validate(manifest_path, &metadata)?; let package_manifest_path = crate::manifest::manifest_path(&metadata, package_name) diff --git a/cargo-pgrx/src/metadata.rs b/cargo-pgrx/src/metadata.rs index 15cf60737e..3313063f5e 100644 --- a/cargo-pgrx/src/metadata.rs +++ b/cargo-pgrx/src/metadata.rs @@ -14,19 +14,34 @@ use owo_colors::*; use semver::VersionReq; use std::path::Path; -pub fn metadata( +pub(crate) fn metadata( features: &clap_cargo::Features, manifest_path: Option<&Path>, + cargo_flags: &[String], ) -> eyre::Result { let mut metadata_command = MetadataCommand::new(); if let Some(manifest_path) = manifest_path { metadata_command.manifest_path(manifest_path.to_owned()); } + + // The `--cargo` passthrough must reach `cargo metadata` — the first cargo invocation, where config issues (private registries, etc.) fail first. + let cargo_flags = split_cargo_flags(cargo_flags); + if !cargo_flags.is_empty() { + metadata_command.other_options(cargo_flags); + } features.forward_metadata(&mut metadata_command); let metadata = metadata_command.exec()?; Ok(metadata) } +/// Split each `--cargo` value on whitespace, so both the single-token form +/// (`--cargo=--config=child/.cargo/config.toml`) and the quoted-string form +/// (`--cargo "--config foo"`) work, forwarding each token as one argv element. +// ponytail: naive whitespace split, same as PGRX_BUILD_FLAGS in cargo.rs. This splits *every* value, including the single-token `--cargo=--config=x` form, so a flag value containing spaces (e.g. a config path with a space) is NOT supported by any form; use a space-free path, or pass config inline as `--cargo=--config=key="value"`. Upgrade path if that ever bites: shell-style tokenization that honors quotes instead of a raw whitespace split. +pub(crate) fn split_cargo_flags(cargo_flags: &[String]) -> Vec { + cargo_flags.iter().flat_map(|f| f.split_ascii_whitespace().map(str::to_owned)).collect() +} + #[tracing::instrument(level = "error", skip_all)] pub fn validate(path: Option<&Path>, metadata: &Metadata) -> eyre::Result<()> { let cargo_pgrx_version = env!("CARGO_PKG_VERSION"); @@ -91,3 +106,33 @@ cargo-pgrx and pgrx library versions must be identical. Ok(()) } + +#[cfg(test)] +mod tests { + use super::split_cargo_flags; + + #[test] + fn split_cargo_flags_handles_both_forms() { + // single-token form is passed through untouched + assert_eq!( + split_cargo_flags(&["--config=child/.cargo/config.toml".into()]), + vec!["--config=child/.cargo/config.toml"] + ); + // quoted-string form is split into separate argv tokens + assert_eq!( + split_cargo_flags(&["--config foo --offline".into()]), + vec!["--config", "foo", "--offline"] + ); + // repeatable `--cargo` and whitespace-splitting compose + assert_eq!( + split_cargo_flags(&["--offline".into(), "--config foo".into()]), + vec!["--offline", "--config", "foo"] + ); + // documented limitation: a value with an embedded space is split too, + // even in the single-token form, so space-containing paths are unsupported + assert_eq!( + split_cargo_flags(&["--config=/my path/config.toml".into()]), + vec!["--config=/my", "path/config.toml"] + ); + } +} diff --git a/cargo-pgrx/tests/cargo_flags_passthrough.rs b/cargo-pgrx/tests/cargo_flags_passthrough.rs new file mode 100644 index 0000000000..d87cc10614 --- /dev/null +++ b/cargo-pgrx/tests/cargo_flags_passthrough.rs @@ -0,0 +1,102 @@ +//LICENSE Portions Copyright 2019-2021 ZomboDB, LLC. +//LICENSE +//LICENSE Portions Copyright 2021-2023 Technology Concepts & Design, Inc. +//LICENSE +//LICENSE Portions Copyright 2023-2023 PgCentral Foundation, Inc. +//LICENSE +//LICENSE All rights reserved. +//LICENSE +//LICENSE Use of this source code is governed by the MIT license that can be found in the LICENSE file. + +//! Tests for the `--cargo` passthrough (issue #2135). +//! +//! `--cargo` flags must reach **every** cargo invocation cargo-pgrx makes — +//! in particular `cargo metadata`, the first one — while `PGRX_BUILD_FLAGS` +//! stays build-only. We assert this by spawning the real binary with a bogus +//! flag and checking *where* it fails: at `cargo metadata` (forwarded) or not. + +use std::path::PathBuf; +use std::process::Command; + +const CARGO_PGRX: &str = env!("CARGO_BIN_EXE_cargo-pgrx"); +/// A flag cargo doesn't recognize, so whichever cargo invocation receives it +/// fails loudly and unmistakably. +const BOGUS: &str = "--pgrx-cargo-flags-probe"; + +/// Create a throwaway crate and run `cargo pgrx install` against it with the +/// given extra CLI args and env vars. Returns the combined stdout+stderr. +fn install_with(test_tag: &str, extra_args: &[&str], envs: &[(&str, &str)]) -> String { + let dir: PathBuf = + std::env::temp_dir().join(format!("pgrx-cargoflags-{test_tag}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + let status = Command::new(env!("CARGO")) + .args(["new", "--lib", "--name", "cargoflagsprobe"]) + .arg(&dir) + .status() + .expect("failed to scaffold temp crate"); + assert!(status.success(), "could not create temp crate"); + + let mut cmd = Command::new(CARGO_PGRX); + cmd.args(["pgrx", "install", "--manifest-path"]).arg(dir.join("Cargo.toml")); + cmd.args(extra_args); + for (k, v) in envs { + cmd.env(k, v); + } + let output = cmd.output().expect("failed to spawn cargo-pgrx"); + let mut combined = String::from_utf8_lossy(&output.stderr).into_owned(); + combined.push_str(&String::from_utf8_lossy(&output.stdout)); + + let _ = std::fs::remove_dir_all(&dir); + combined +} + +/// `--cargo` must reach `cargo metadata`: a bogus flag there makes the very +/// first step — `cargo metadata` — fail. +#[test] +fn cargo_flag_reaches_cargo_metadata() { + let out = install_with("cargo", &[&format!("--cargo={BOGUS}")], &[]); + assert!( + out.contains("couldn't get cargo metadata"), + "expected the bogus --cargo flag to make `cargo metadata` fail, got:\n{out}" + ); + assert!( + out.contains(BOGUS), + "expected cargo's error to mention the forwarded flag `{BOGUS}`, got:\n{out}" + ); +} + +/// The quoted-string form (`--cargo "--flag-a --flag-b"`) is split on +/// whitespace: two valid cargo flags packed into one `--cargo` value get past +/// `cargo metadata`. Without splitting, the mashed-together token would be an +/// unknown argument and `cargo metadata` would reject it. +#[test] +fn cargo_flag_whitespace_string_is_split() { + let out = install_with("split", &["--cargo", "--offline --color=never"], &[]); + assert!( + !out.contains("couldn't get cargo metadata"), + "two valid flags in one --cargo value should be split and pass `cargo metadata`, got:\n{out}" + ); +} + +/// `PGRX_BUILD_FLAGS` must stay build-only: a bogus flag there must NOT break +/// `cargo metadata` (the run gets past it and fails later, for other reasons). +/// This is the property that keeps `--cargo` distinct from build-only flags. +#[test] +fn pgrx_build_flags_does_not_reach_cargo_metadata() { + let out = install_with("build", &[], &[("PGRX_BUILD_FLAGS", BOGUS)]); + assert!( + !out.contains("couldn't get cargo metadata"), + "PGRX_BUILD_FLAGS must not be forwarded to `cargo metadata`, but it failed there:\n{out}" + ); +} + +/// Sanity: with no `--cargo` flag, `cargo metadata` is not what fails for a +/// bogus reason — it succeeds and the run fails later (no control file, etc.). +#[test] +fn no_flags_does_not_break_cargo_metadata() { + let out = install_with("none", &[], &[]); + assert!( + !out.contains("couldn't get cargo metadata"), + "a clean run should get past `cargo metadata`, got:\n{out}" + ); +} From 4471b67ff011203e69785e4786c31f8d60eb7ca8 Mon Sep 17 00:00:00 2001 From: danielshih Date: Mon, 13 Jul 2026 13:53:33 +0000 Subject: [PATCH 2/2] Add support for `--cargo` passthrough flags in regress & schema commands --- cargo-pgrx/src/cargo.rs | 31 ++++++++++++++++++++ cargo-pgrx/src/command/install.rs | 1 + cargo-pgrx/src/command/regress.rs | 6 +++- cargo-pgrx/src/command/run.rs | 47 ++++++++++++++++++++++++++++++- cargo-pgrx/src/command/schema.rs | 45 +++++++++++++++++++++++++++-- 5 files changed, 125 insertions(+), 5 deletions(-) diff --git a/cargo-pgrx/src/cargo.rs b/cargo-pgrx/src/cargo.rs index 1afe2a3e91..460c90e5f6 100644 --- a/cargo-pgrx/src/cargo.rs +++ b/cargo-pgrx/src/cargo.rs @@ -26,6 +26,8 @@ pub struct Cargo { more_args: BTreeMap>, // Extra arguments passed after `cargo rustc --`. rustc_args: Vec, + // `--cargo` passthrough flags, forwarded verbatim (after whitespace-splitting). + cargo_flags: Vec, } impl Cargo { @@ -75,6 +77,12 @@ impl Cargo { self } + /// `--cargo` passthrough flags, forwarded to this cargo invocation. + pub fn cargo_flags(mut self, flags: &[String]) -> Self { + self.cargo_flags = crate::metadata::split_cargo_flags(flags); + self + } + pub fn rustc_args(mut self, args: I) -> Self where I: IntoIterator, @@ -110,6 +118,7 @@ impl Cargo { subcmd: _, more_args, rustc_args, + cargo_flags, } = self; // set most-interesting flags first, like profile, target, and manifest-path @@ -158,6 +167,11 @@ impl Cargo { cmd.arg(arg); } + // `--cargo` passthrough: forward each (already whitespace-split) token verbatim. + for arg in cargo_flags { + cmd.arg(arg); + } + // set envs if let Some(log_level) = log_level { cmd.env("RUST_LOG", log_level); @@ -393,4 +407,21 @@ mod tests { "expected no-GC rustc args after `--`: {args:?}" ); } + + #[test] + fn cargo_flags_are_forwarded_and_whitespace_split() { + // `cargo pgrx schema` (and other builds) route `--cargo` through the + // `Cargo` builder; both the single-token and quoted-string forms must + // reach the spawned command as separate argv tokens. + let command = Cargo::default() + .subcommand("rustc") + .cargo_flags(&["--offline".into(), "--config foo".into()]) + .into_command(); + let args = + command.get_args().map(|arg| arg.to_string_lossy().into_owned()).collect::>(); + + assert!(args.contains(&"--offline".to_string()), "missing --offline: {args:?}"); + assert!(args.contains(&"--config".to_string()), "quoted form not split: {args:?}"); + assert!(args.contains(&"foo".to_string()), "quoted form not split: {args:?}"); + } } diff --git a/cargo-pgrx/src/command/install.rs b/cargo-pgrx/src/command/install.rs index 88fbfebd34..d94ba4c639 100644 --- a/cargo-pgrx/src/command/install.rs +++ b/cargo-pgrx/src/command/install.rs @@ -429,6 +429,7 @@ fn copy_sql_files( // explicit ALTER EXTENSION would be redundant. false, output_tracking, + cargo_flags, )?; } diff --git a/cargo-pgrx/src/command/regress.rs b/cargo-pgrx/src/command/regress.rs index e79e0622d8..644f1b10da 100644 --- a/cargo-pgrx/src/command/regress.rs +++ b/cargo-pgrx/src/command/regress.rs @@ -102,6 +102,10 @@ pub(crate) struct Regress { /// Run Postgres under valgrind while executing the regression tests #[clap(long)] pub(crate) valgrind: bool, + + /// Extra cargo flags forwarded to every `cargo` invocation. Repeatable and split on whitespace: `--cargo=--config=foo` or `--cargo "--offline --frozen"`. + #[clap(long = "cargo", value_name = "FLAG", allow_hyphen_values = true)] + pub(crate) cargo: Vec, } impl Regress { @@ -360,7 +364,7 @@ impl CommandExecute for Regress { &self.features, self.package.as_deref(), self.manifest_path.as_deref(), - &[], + &self.cargo, )?; let extname = get_property(&manifest_path, "extname")? .expect("extension name property `extname` should always be known"); diff --git a/cargo-pgrx/src/command/run.rs b/cargo-pgrx/src/command/run.rs index 4c3d654633..7d04b36670 100644 --- a/cargo-pgrx/src/command/run.rs +++ b/cargo-pgrx/src/command/run.rs @@ -76,7 +76,7 @@ impl From<&Regress> for Run { pgcli: false, install_only: false, valgrind: regress.valgrind, - cargo: Vec::new(), + cargo: regress.cargo.clone(), } } } @@ -255,3 +255,48 @@ pub(crate) fn exec_psql(pg_config: &PgConfig, dbname: &str, pgcli: bool) -> eyre tracing::trace!(status_code = %output.status, command = %command_str, "Finished"); Ok(()) } + +#[cfg(test)] +mod tests { + use super::Run; + use crate::command::regress::Regress; + use clap::Parser; + + /// Mirror the real CLI shape (global `--verbose` on the top-level parser, + /// `Regress` as a subcommand) so `Regress`'s `from_global` verbose resolves. + #[derive(Parser)] + struct Cli { + #[arg(short = 'v', long, action = clap::ArgAction::Count, global = true)] + verbose: u8, + #[command(subcommand)] + cmd: Cmd, + } + + #[derive(clap::Subcommand)] + enum Cmd { + Regress(Regress), + } + + /// `cargo pgrx regress` builds too, so `--cargo` must be exposed on it and + /// forwarded into the `Run` it delegates to. This locks in both: clap parses + /// the repeatable flag, and `Run::from(&Regress)` carries it over (not the + /// empty vec it used to hard-code). + #[test] + fn regress_cargo_flags_are_parsed_and_forwarded_to_run() { + let cli = Cli::try_parse_from([ + "cargo-pgrx", + "regress", + "--cargo", + "--offline", + "--cargo", + "--config=child/.cargo/config.toml", + ]) + .expect("regress should parse repeatable --cargo flags"); + let Cmd::Regress(regress) = cli.cmd; + + assert_eq!(regress.cargo, vec!["--offline", "--config=child/.cargo/config.toml"]); + + let run = Run::from(®ress); + assert_eq!(run.cargo, regress.cargo, "Run::from(&Regress) must forward --cargo"); + } +} diff --git a/cargo-pgrx/src/command/schema.rs b/cargo-pgrx/src/command/schema.rs index f5df7e5743..861185b4f7 100644 --- a/cargo-pgrx/src/command/schema.rs +++ b/cargo-pgrx/src/command/schema.rs @@ -74,6 +74,9 @@ pub(crate) struct Schema { /// already-installed extension. #[clap(long)] no_alter_extension: bool, + /// Extra cargo flags forwarded to every `cargo` invocation. Repeatable and split on whitespace: `--cargo=--config=foo` or `--cargo "--offline --frozen"`. + #[clap(long = "cargo", value_name = "FLAG", allow_hyphen_values = true)] + cargo: Vec, } impl CommandExecute for Schema { @@ -97,7 +100,7 @@ impl CommandExecute for Schema { &self.features, self.package.as_deref(), self.manifest_path.as_deref(), - &[], + &self.cargo, )?; // This does meaningful mutation, unfortunately let (_pg_config, _pg_version) = pg_config_and_version( @@ -129,6 +132,7 @@ impl CommandExecute for Schema { items, attach, &mut vec![], + &self.cargo, ) } } @@ -175,6 +179,7 @@ pub(crate) fn generate_schema_for_cli( items: Option<&[String]>, attach: bool, output_tracking: &mut Vec, + cargo_flags: &[String], ) -> eyre::Result<()> { let manifest = Manifest::from_path(package_manifest_path)?; let features_arg = features.features.join(" "); @@ -190,7 +195,8 @@ pub(crate) fn generate_schema_for_cli( .std_streams([cargo::Stdio::Null, cargo::Stdio::Null, cargo::Stdio::Inherit]) .manifest_path(user_manifest_path.map(|p| p.to_owned())) .log_level(log_level) - .features(features.clone()); + .features(features.clone()) + .cargo_flags(cargo_flags); if !skip_build { // NB: The only path where this happens is via the command line using `cargo pgrx schema` @@ -412,12 +418,45 @@ fn first_build( #[cfg(test)] mod tests { - use super::{decode_section_entities, split_positional_args}; + use super::{Schema, decode_section_entities, split_positional_args}; + use clap::Parser; fn strs(args: &[&str]) -> Vec { args.iter().map(|s| (*s).to_owned()).collect() } + /// Mirror the real CLI shape so `Schema`'s `from_global` verbose resolves. + #[derive(Parser)] + struct Cli { + #[arg(short = 'v', long, action = clap::ArgAction::Count, global = true)] + verbose: u8, + #[command(subcommand)] + cmd: Cmd, + } + + #[derive(clap::Subcommand)] + enum Cmd { + Schema(Schema), + } + + /// `cargo pgrx schema` builds too, so it must expose repeatable `--cargo` + /// (forwarded to `cargo metadata` and the schema-gen build). + #[test] + fn schema_parses_repeatable_cargo_flags() { + let cli = Cli::try_parse_from([ + "cargo-pgrx", + "schema", + "--cargo", + "--offline", + "--cargo", + "--config=child/.cargo/config.toml", + ]) + .expect("schema should parse repeatable --cargo flags"); + let Cmd::Schema(schema) = cli.cmd; + + assert_eq!(schema.cargo, vec!["--offline", "--config=child/.cargo/config.toml"]); + } + #[test] fn test_missing_schema_section_errors() { let root_path = env!("CARGO_MANIFEST_DIR");