From 81936956663a480607a3de0b4541530e5a6a9cb2 Mon Sep 17 00:00:00 2001 From: "David E. Wheeler" Date: Fri, 12 Jan 2024 15:10:29 -0500 Subject: [PATCH] Refactor extension build command handling 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. --- .gitignore | 1 + cli/Cargo.toml | 2 +- cli/src/commands/containers.rs | 56 +++++++++++++++++++---------- cli/src/commands/generic_build.rs | 60 +++++++++---------------------- cli/src/commands/license.rs | 2 ++ cli/src/commands/pgrx.rs | 2 ++ 6 files changed, 59 insertions(+), 64 deletions(-) diff --git a/.gitignore b/.gitignore index c591d4fc..8c26d071 100644 --- a/.gitignore +++ b/.gitignore @@ -3,3 +3,4 @@ temp.sh **/.DS_Store poetry.lock site/ +.vscode/ diff --git a/cli/Cargo.toml b/cli/Cargo.toml index 77542929..879ebefd 100644 --- a/cli/Cargo.toml +++ b/cli/Cargo.toml @@ -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" diff --git a/cli/src/commands/containers.rs b/cli/src/commands/containers.rs index a51cef7f..7b5aef20 100644 --- a/cli/src/commands/containers.rs +++ b/cli/src/commands/containers.rs @@ -68,9 +68,10 @@ pub async fn exec_in_container( docker: &Docker, container_id: &str, command: Vec<&str>, + dir: Option<&str>, env: Option>, ) -> Result { - 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) } @@ -79,6 +80,7 @@ pub async fn exec_in_container_with_exit_code( docker: &Docker, container_id: &str, command: Vec<&str>, + dir: Option<&str>, env: Option>, ) -> Result<(String, Option), anyhow::Error> { println!("Executing in container: {:?}", command.join(" ")); @@ -86,6 +88,7 @@ pub async fn exec_in_container_with_exit_code( let config = CreateExecOptions { cmd: Some(command), env, + working_dir: dir, attach_stdout: Some(true), attach_stderr: Some(true), ..Default::default() @@ -175,7 +178,7 @@ async fn read_from_container( container_id: &str, path: &str, ) -> anyhow::Result { - 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) } @@ -186,12 +189,24 @@ pub async fn find_installed_extension_files( inclusion_patterns: &[glob::Pattern], ) -> Result { 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 @@ -413,11 +428,18 @@ 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( @@ -425,6 +447,7 @@ pub async fn package_installed_extension_files( container_id, vec!["pg_config", "--pkglibdir"], None, + None, ) .await?; let pkglibdir = pkglibdir.trim(); @@ -638,6 +661,7 @@ pub async fn locate_makefile( container_id, vec!["find", ".", "-type", "f", "-iname", "Makefile"], None, + None, ) .await?; @@ -665,14 +689,6 @@ 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, @@ -680,10 +696,10 @@ pub async fn makefile_contains_target( dir: &str, target: &str, ) -> anyhow::Result { - 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); @@ -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?; @@ -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) { diff --git a/cli/src/commands/generic_build.rs b/cli/src/commands/generic_build.rs index 6021150e..cedf638e 100644 --- a/cli/src/commands/generic_build.rs +++ b/cli/src/commands/generic_build.rs @@ -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, }; @@ -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..."); @@ -133,6 +133,7 @@ pub async fn build_generic( &temp_container.id, vec!["mkdir", "/usr/licenses/"], None, + None, ) .await?; @@ -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!"); @@ -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?; diff --git a/cli/src/commands/license.rs b/cli/src/commands/license.rs index 28f3cfbe..b3552a30 100644 --- a/cli/src/commands/license.rs +++ b/cli/src/commands/license.rs @@ -104,6 +104,7 @@ pub async fn find_licenses(docker: Docker, container_id: &str) -> Result