Skip to content

Commit

Permalink
Refactor extension build command handling
Browse files Browse the repository at this point in the history
The contrib `Dockerfile`s run `./configure && make`, so not only is
there no need to do so here, but it was causing issues by running the
commands in the parent directory of the unpacked extension directory:

```
Executing in container: "su postgres -c bash -c \"././postgis-3.4.0/configure && make -C ./postgis-3.4.0 check && echo done\""
configure: error: source directory already configured; run "make distclean" there first
```

Apparently `./configure` won't re-run from a different directory unless
`make distclean` is run. But it's not even necessary! So remove the
check for the `configure` file in `run_tests()`, and then also remove
the `file_exists()` function, as it's no longer uses.

Then refactor `exec_in_container()` and
`exec_in_container_with_exit_code()` to take a directory argument, and
pass it to bollard's `CreateExecOptions()`, then pass the unpacked
directory name in the appropriate places.

That includes especially `make check` and `make installcheck`. Also
modify them so they no longe run as the `postgres` user --- which can
cause permission issues, as the source is unpacked and owned by root in
the `Dockerfile`s --- and instead set the `PGUSER` environment variable.
This ensure that all tests are run as the `root` system user but connect
to the `postgres` database as the `postgres` user.
  • Loading branch information
theory committed Jan 12, 2024
1 parent f9c6237 commit 8193695
Show file tree
Hide file tree
Showing 6 changed files with 59 additions and 64 deletions.
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@ temp.sh
**/.DS_Store
poetry.lock
site/
.vscode/
2 changes: 1 addition & 1 deletion cli/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "pg-trunk"
version = "0.12.11"
version = "0.12.12"
edition = "2021"
authors = ["Steven Miller", "Ian Stanton", "Vinícius Miguel"]
description = "A package manager for PostgreSQL extensions"
Expand Down
56 changes: 37 additions & 19 deletions cli/src/commands/containers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,10 @@ pub async fn exec_in_container(
docker: &Docker,
container_id: &str,
command: Vec<&str>,
dir: Option<&str>,
env: Option<Vec<&str>>,
) -> Result<String, anyhow::Error> {
exec_in_container_with_exit_code(docker, container_id, command, env)
exec_in_container_with_exit_code(docker, container_id, command, dir, env)
.await
.map(|(output, _code)| output)
}
Expand All @@ -79,13 +80,15 @@ pub async fn exec_in_container_with_exit_code(
docker: &Docker,
container_id: &str,
command: Vec<&str>,
dir: Option<&str>,
env: Option<Vec<&str>>,
) -> Result<(String, Option<i64>), anyhow::Error> {
println!("Executing in container: {:?}", command.join(" "));

let config = CreateExecOptions {
cmd: Some(command),
env,
working_dir: dir,
attach_stdout: Some(true),
attach_stderr: Some(true),
..Default::default()
Expand Down Expand Up @@ -175,7 +178,7 @@ async fn read_from_container(
container_id: &str,
path: &str,
) -> anyhow::Result<String> {
let contents = exec_in_container(docker, container_id, vec!["cat", path], None).await?;
let contents = exec_in_container(docker, container_id, vec!["cat", path], None, None).await?;

Ok(contents)
}
Expand All @@ -186,12 +189,24 @@ pub async fn find_installed_extension_files(
inclusion_patterns: &[glob::Pattern],
) -> Result<ExtensionFiles, anyhow::Error> {
let mut control_file = None;
let sharedir =
exec_in_container(docker, container_id, vec!["pg_config", "--sharedir"], None).await?;
let sharedir = exec_in_container(
docker,
container_id,
vec!["pg_config", "--sharedir"],
None,
None,
)
.await?;
let sharedir = sharedir.trim();

let pkglibdir =
exec_in_container(docker, container_id, vec!["pg_config", "--pkglibdir"], None).await?;
let pkglibdir = exec_in_container(
docker,
container_id,
vec!["pg_config", "--pkglibdir"],
None,
None,
)
.await?;
let pkglibdir = pkglibdir.trim();

// collect changes from container filesystem
Expand Down Expand Up @@ -413,18 +428,26 @@ pub async fn package_installed_extension_files(
let name = name.to_owned();
let extension_version = extension_version.to_owned();

let target_arch = exec_in_container(&docker, container_id, vec!["uname", "-m"], None).await?;
let target_arch =
exec_in_container(&docker, container_id, vec!["uname", "-m"], None, None).await?;
let target_arch = target_arch.trim().to_string();

let sharedir =
exec_in_container(&docker, container_id, vec!["pg_config", "--sharedir"], None).await?;
let sharedir = exec_in_container(
&docker,
container_id,
vec!["pg_config", "--sharedir"],
None,
None,
)
.await?;
let sharedir = sharedir.trim();

let pkglibdir = exec_in_container(
&docker,
container_id,
vec!["pg_config", "--pkglibdir"],
None,
None,
)
.await?;
let pkglibdir = pkglibdir.trim();
Expand Down Expand Up @@ -638,6 +661,7 @@ pub async fn locate_makefile(
container_id,
vec!["find", ".", "-type", "f", "-iname", "Makefile"],
None,
None,
)
.await?;

Expand Down Expand Up @@ -665,25 +689,17 @@ pub async fn locate_makefile(
))
}

/// Returns true if the file in `path` exists
pub async fn file_exists(docker: &Docker, container_id: &str, path: &str) -> bool {
exec_in_container_with_exit_code(docker, container_id, vec!["test", "-e", path], None)
.await
.map(|(_, exit_code)| exit_code == Some(0))
.unwrap_or(false)
}

/// Returns true if the Makefile in this container contains the given target
pub async fn makefile_contains_target(
docker: &Docker,
container_id: &str,
dir: &str,
target: &str,
) -> anyhow::Result<bool> {
let command = vec!["make", "-C", dir, "-q", target];
let command = vec!["make", "-q", target];

let (output, exit_code) =
exec_in_container_with_exit_code(docker, container_id, command, None).await?;
exec_in_container_with_exit_code(docker, container_id, command, Some(dir), None).await?;

if output.contains("is not supported") {
return Ok(false);
Expand All @@ -701,6 +717,7 @@ pub async fn start_postgres(docker: &Docker, container_id: &str) -> anyhow::Resu
container_id,
vec!["chown", "-R", "postgres:postgres", "/app"],
None,
None,
)
.await?;

Expand All @@ -714,6 +731,7 @@ pub async fn start_postgres(docker: &Docker, container_id: &str) -> anyhow::Resu
"bash -c \"mkdir db/ && /usr/lib/postgresql/15/bin/initdb -D /app/db && /usr/lib/postgresql/15/bin/pg_ctl -D /app/db -l logfile start\"",
],
None,
None,
).await?;

if status_code == Some(0) {
Expand Down
60 changes: 16 additions & 44 deletions cli/src/commands/generic_build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use tokio::sync::mpsc;
use tokio_task_manager::Task;

use crate::commands::containers::{
build_image, exec_in_container, exec_in_container_with_exit_code, file_exists, locate_makefile,
build_image, exec_in_container, exec_in_container_with_exit_code, locate_makefile,
makefile_contains_target, package_installed_extension_files, run_temporary_container,
start_postgres,
};
Expand Down Expand Up @@ -121,7 +121,7 @@ pub async fn build_generic(

println!("Determining installation files...");
let _exec_output =
exec_in_container(&docker, &temp_container.id, install_command, None).await?;
exec_in_container(&docker, &temp_container.id, install_command, None, None).await?;

// Search for license files to include
println!("Determining license files to include...");
Expand All @@ -133,6 +133,7 @@ pub async fn build_generic(
&temp_container.id,
vec!["mkdir", "/usr/licenses/"],
None,
None,
)
.await?;

Expand Down Expand Up @@ -191,40 +192,14 @@ async fn run_tests(
println!("make check was found in the Makefile");
start_postgres(docker, container_id).await?;

let configure_file = project_dir.join("configure");
let configure_file = configure_file.to_str().unwrap();

let configure_exists = file_exists(docker, container_id, configure_file).await;
let exit_code = if configure_exists {
let (_, exit_code) = exec_in_container_with_exit_code(
docker,
container_id,
vec![
"su",
"postgres",
"-c",
&format!("bash -c \"./{configure_file} && make -C {project_dir_utf8} check && echo done\"")
],
None,
)
.await?;

exit_code
} else {
let (_, exit_code) = exec_in_container_with_exit_code(
docker,
container_id,
vec![
"su",
"postgres",
"-c",
&format!("make -C {project_dir_utf8} check"),
],
None,
)
.await?;
exit_code
};
let (_, exit_code) = exec_in_container_with_exit_code(
docker,
container_id,
vec!["make", "check"],
Some(project_dir_utf8),
Some(vec!["PGUSER=postgres"]),
)
.await?;

anyhow::ensure!(matches!(exit_code, Some(0)), "Tests failed!");

Expand All @@ -239,20 +214,17 @@ async fn run_tests(
exec_in_container(
docker,
container_id,
vec!["make", "-C", project_dir_utf8, "install"],
vec!["make", "install"],
Some(project_dir_utf8),
None,
)
.await?;
let (_output, exit_code) = exec_in_container_with_exit_code(
docker,
container_id,
vec![
"su",
"postgres",
"-c",
&format!("make -C {project_dir_utf8} installcheck"),
],
None,
vec!["make", "installcheck"],
Some(project_dir_utf8),
Some(vec!["PGUSER=postgres"]),
)
.await?;

Expand Down
2 changes: 2 additions & 0 deletions cli/src/commands/license.rs
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ pub async fn find_licenses(docker: Docker, container_id: &str) -> Result<Vec<Str
"licen[cs]e.*",
],
None,
None,
)
.await?;
let lines = license_output.lines();
Expand Down Expand Up @@ -131,6 +132,7 @@ pub async fn copy_licenses(
license.to_string().as_str(),
"/usr/licenses/",
],
None,
env.clone(),
)
.await?;
Expand Down
2 changes: 2 additions & 0 deletions cli/src/commands/pgrx.rs
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ pub async fn build_pgrx(
"/",
],
None,
None,
)
.await?;

Expand All @@ -211,6 +212,7 @@ pub async fn build_pgrx(
&temp_container.id,
vec!["mkdir", "/usr/licenses/"],
None,
None,
)
.await?;

Expand Down

0 comments on commit 8193695

Please sign in to comment.