Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cargo pgrx test --runas envar passing #1674

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ owo-colors = { version = "3.5", features = [ "supports-colors" ] } # for output
proc-macro2 = { version = "1.0.78", features = [ "span-locations" ] }
quote = "1.0.33"
regex = "1.1" # used for build/test
shlex = "1.3" # shell lexing, also used by many of our deps
syn = { version = "2", features = [ "extra-traits", "full", "parsing" ] }
toml = "0.8" # our config files
thiserror = "1"
Expand Down
2 changes: 1 addition & 1 deletion pgrx-pg-sys/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,10 @@ pgrx-pg-config.workspace = true

eyre.workspace = true
proc-macro2.workspace = true
shlex.workspace = true
syn.workspace = true
walkdir.workspace = true

bindgen = { version = "0.69", default-features = false, features = ["runtime"] }
clang-sys = { version = "1", features = ["clang_6_0", "runtime"] }
quote = "1.0.33"
shlex = "1.3" # shell lexing, also used by many of our deps
1 change: 1 addition & 0 deletions pgrx-tests/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ eyre.workspace = true
libc.workspace = true
owo-colors.workspace = true
regex.workspace = true
shlex.workspace = true
thiserror.workspace = true

paste = "1"
Expand Down
23 changes: 23 additions & 0 deletions pgrx-tests/src/framework.rs
Original file line number Diff line number Diff line change
Expand Up @@ -546,7 +546,30 @@ fn start_pg(loglines: LogLines) -> eyre::Result<String> {
cmd.arg(postmaster_path);
cmd
} else if let Some(runas) = get_runas() {
#[inline]
fn accept_envar(var: &str) -> bool {
// taken from https://doc.rust-lang.org/cargo/reference/environment-variables.html
var.starts_with("CARGO")
|| var.starts_with("RUST")
|| var.starts_with("DEP_")
|| ["OUT_DIR", "TARGET", "HOST", "NUM_JOBS", "OPT_LEVEL", "DEBUG", "PROFILE"]
.contains(&var)
Comment on lines +552 to +556
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, regex maybe?

}

let mut cmd = sudo_command(runas);

// when running the `postmaster` process via `sudo`, we need to copy the cargo/rust-related
// environment variables and pass as arguments to sudo, ahead of the `postmaster` command itself
//
// This ensures that in-process #[pg_test]s will see the `CARGO_xxx` envars they expect
for (var, value) in std::env::vars() {
if accept_envar(&var) {
let env_as_arg = format!("{var}={}", shlex::try_quote(&value)?);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we use shlex, we should just skip the env vars that we can't quote the values of correctly, or we should panic more noisily, or we should stop iterating the env vars entirely. here is one of those options.

Suggested change
let env_as_arg = format!("{var}={}", shlex::try_quote(&value)?);
let Ok(quoted_val) = shlex::try_quote(&value) else {
continue
};
let env_as_arg = format!("{var}={quoted_val}");

cmd.arg(env_as_arg);
}
}
Comment on lines +565 to +570
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alternatively, instead of splitting and then reapplying these, why not use sudo's --preserve-env=var,var,var,...? are we worried about not having the required privilege?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn’t see that in the man page. Let me investigate that and make sure we can rely on it even being there here in 2023.


// now we can add the `postmaster` as the command for `sudo` to execute
cmd.arg(postmaster_path);
cmd
} else {
Expand Down
Loading