From 1bc18f2cf2b66d2a78568e420556b4431de484db Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Wed, 20 Jul 2022 15:39:51 +0300 Subject: [PATCH] Separate Postgres build dir from installation dir. Previously, Postgres was built in 'tmp_install/build', and installed into 'tmp_install'. In the CI, the 'build' directory was included in the final neon.tar.zst artifact that includes all the necessary binaries. That was unnecessary, the intermediate build results are not needed, only the final binaries. Separate the build directory so that the Postgres build happens in 'build', and it is installed into 'tmp_install'. That makes the final neon.tar.zst artifact smaller. The changes to the python tests are needed to find the 'pg_regress' binary in the installation directory. Previously, they would use the 'pg_regress' binary from the build directory, not the final installation location. --- .../actions/run-python-test-set/action.yml | 18 +++++++---- .github/workflows/build_and_test.yml | 11 +++++-- Makefile | 32 +++++++++++-------- .../batch_pg_regress/test_isolation.py | 12 ++++--- .../batch_pg_regress/test_neon_regress.py | 11 ++++--- .../batch_pg_regress/test_pg_regress.py | 14 +++++--- test_runner/fixtures/neon_fixtures.py | 7 ++-- 7 files changed, 68 insertions(+), 37 deletions(-) diff --git a/.github/actions/run-python-test-set/action.yml b/.github/actions/run-python-test-set/action.yml index 0d058d47c1..0ea35084cf 100644 --- a/.github/actions/run-python-test-set/action.yml +++ b/.github/actions/run-python-test-set/action.yml @@ -31,6 +31,13 @@ inputs: runs: using: "composite" steps: + - name: Checkout + if: inputs.needs_postgres_source == 'true' + uses: actions/checkout@v3 + with: + submodules: true + fetch-depth: 1 + - name: Get Neon artifact for restoration uses: actions/download-artifact@v3 with: @@ -44,12 +51,11 @@ runs: tar -xf ./neon-artifact/neon.tar.zst -C /tmp/neon/ rm -rf ./neon-artifact/ - - name: Checkout - if: inputs.needs_postgres_source == 'true' - uses: actions/checkout@v3 - with: - submodules: true - fetch-depth: 1 + # Restore the parts of the 'build' directory that were included in the + # tarball. This includes the regression test modules in + # src/test/regress/*.so. + mkdir -p build/ + cp -a /tmp/neon/pg_build/* build/ - name: Cache poetry deps id: cache_poetry diff --git a/.github/workflows/build_and_test.yml b/.github/workflows/build_and_test.yml index 5874aa9b5c..1f70a32aa9 100644 --- a/.github/workflows/build_and_test.yml +++ b/.github/workflows/build_and_test.yml @@ -88,7 +88,9 @@ jobs: id: cache_pg uses: actions/cache@v3 with: - path: tmp_install/ + path: | + tmp_install/ + build/src/test/regress/*.so key: v1-${{ runner.os }}-${{ matrix.build_type }}-pg-${{ steps.pg_ver.outputs.pg_rev }}-${{ hashFiles('Makefile') }} - name: Build postgres @@ -143,7 +145,12 @@ jobs: fi - name: Install postgres binaries - run: cp -a tmp_install /tmp/neon/pg_install + run: | + cp -a tmp_install /tmp/neon/pg_install + + # Include modules needed by the Postgres regression tests + mkdir -p /tmp/neon/pg_build/src/test/regress + cp -a build/src/test/regress/*.so /tmp/neon/pg_build/src/test/regress - name: Prepare neon artifact run: ZSTD_NBTHREADS=0 tar -C /tmp/neon/ -cf ./neon.tar.zst --zstd . diff --git a/Makefile b/Makefile index 2f5f2a40e1..5284aad11a 100644 --- a/Makefile +++ b/Makefile @@ -60,10 +60,14 @@ neon: postgres-headers $(CARGO_CMD_PREFIX) cargo build $(CARGO_BUILD_FLAGS) ### PostgreSQL parts -$(POSTGRES_INSTALL_DIR)/build/config.status: +# +# Postgres is built in the 'build' directory, and installed into +# $(POSTGRES_INSTALL_DIR), which defaults to 'tmp_install' +# +build/config.status: +@echo "Configuring postgres build" - mkdir -p $(POSTGRES_INSTALL_DIR)/build - (cd $(POSTGRES_INSTALL_DIR)/build && \ + mkdir -p build + (cd build && \ $(ROOT_PROJECT_DIR)/vendor/postgres/configure CFLAGS='$(PG_CFLAGS)' \ $(PG_CONFIGURE_OPTS) \ $(SECCOMP) \ @@ -71,44 +75,44 @@ $(POSTGRES_INSTALL_DIR)/build/config.status: # nicer alias for running 'configure' .PHONY: postgres-configure -postgres-configure: $(POSTGRES_INSTALL_DIR)/build/config.status +postgres-configure: build/config.status # Install the PostgreSQL header files into $(POSTGRES_INSTALL_DIR)/include .PHONY: postgres-headers postgres-headers: postgres-configure +@echo "Installing PostgreSQL headers" - $(MAKE) -C $(POSTGRES_INSTALL_DIR)/build/src/include MAKELEVEL=0 install + $(MAKE) -C build/src/include MAKELEVEL=0 install # Compile and install PostgreSQL and contrib/neon .PHONY: postgres postgres: postgres-configure \ - postgres-headers # to prevent `make install` conflicts with zenith's `postgres-headers` + postgres-headers # to prevent `make install` conflicts with neon's `postgres-headers` +@echo "Compiling PostgreSQL" - $(MAKE) -C $(POSTGRES_INSTALL_DIR)/build MAKELEVEL=0 install + $(MAKE) -C build MAKELEVEL=0 install +@echo "Compiling contrib/neon" - $(MAKE) -C $(POSTGRES_INSTALL_DIR)/build/contrib/neon install + $(MAKE) -C build/contrib/neon install +@echo "Compiling contrib/neon_test_utils" - $(MAKE) -C $(POSTGRES_INSTALL_DIR)/build/contrib/neon_test_utils install + $(MAKE) -C build/contrib/neon_test_utils install +@echo "Compiling pg_buffercache" - $(MAKE) -C $(POSTGRES_INSTALL_DIR)/build/contrib/pg_buffercache install + $(MAKE) -C build/contrib/pg_buffercache install +@echo "Compiling pageinspect" - $(MAKE) -C $(POSTGRES_INSTALL_DIR)/build/contrib/pageinspect install + $(MAKE) -C build/contrib/pageinspect install .PHONY: postgres-clean postgres-clean: - $(MAKE) -C $(POSTGRES_INSTALL_DIR)/build MAKELEVEL=0 clean + $(MAKE) -C build MAKELEVEL=0 clean # This doesn't remove the effects of 'configure'. .PHONY: clean clean: - cd $(POSTGRES_INSTALL_DIR)/build && $(MAKE) clean + cd build && $(MAKE) clean $(CARGO_CMD_PREFIX) cargo clean # This removes everything .PHONY: distclean distclean: - rm -rf $(POSTGRES_INSTALL_DIR) + rm -rf build $(POSTGRES_INSTALL_DIR) $(CARGO_CMD_PREFIX) cargo clean .PHONY: fmt diff --git a/test_runner/batch_pg_regress/test_isolation.py b/test_runner/batch_pg_regress/test_isolation.py index 0124459440..5180c21893 100644 --- a/test_runner/batch_pg_regress/test_isolation.py +++ b/test_runner/batch_pg_regress/test_isolation.py @@ -20,18 +20,22 @@ def test_isolation(neon_simple_env: NeonEnv, test_output_dir: Path, pg_bin, caps runpath = test_output_dir / 'regress' (runpath / 'testtablespace').mkdir(parents=True) + # Find the pg_isolation_regress binary + proc = pg_bin.run(['pg_config', '--libdir'], capture_output=True) + libdir = proc.stdout.decode().strip() + proc = pg_bin.run(['pg_config', '--bindir'], capture_output=True) + bindir = proc.stdout.decode().strip() + pg_isolation_regress = os.path.join(libdir, + 'postgresql/pgxs/src/test/isolation/pg_isolation_regress') + # Compute all the file locations that pg_isolation_regress will need. - build_path = os.path.join(pg_distrib_dir, 'build/src/test/isolation') src_path = os.path.join(base_dir, 'vendor/postgres/src/test/isolation') - bindir = os.path.join(pg_distrib_dir, 'bin') schedule = os.path.join(src_path, 'isolation_schedule') - pg_isolation_regress = os.path.join(build_path, 'pg_isolation_regress') pg_isolation_regress_command = [ pg_isolation_regress, '--use-existing', '--bindir={}'.format(bindir), - '--dlpath={}'.format(build_path), '--inputdir={}'.format(src_path), '--schedule={}'.format(schedule), ] diff --git a/test_runner/batch_pg_regress/test_neon_regress.py b/test_runner/batch_pg_regress/test_neon_regress.py index 66ea67d9f1..72ac5ef21f 100644 --- a/test_runner/batch_pg_regress/test_neon_regress.py +++ b/test_runner/batch_pg_regress/test_neon_regress.py @@ -20,19 +20,22 @@ def test_neon_regress(neon_simple_env: NeonEnv, test_output_dir: Path, pg_bin, c runpath = test_output_dir / 'regress' (runpath / 'testtablespace').mkdir(parents=True) + # Find the pg_regress binary and --bindir option to pass to it. + proc = pg_bin.run(['pg_config', '--libdir'], capture_output=True) + libdir = proc.stdout.decode().strip() + proc = pg_bin.run(['pg_config', '--bindir'], capture_output=True) + bindir = proc.stdout.decode().strip() + pg_regress = os.path.join(libdir, 'postgresql/pgxs/src/test/regress/pg_regress') + # Compute all the file locations that pg_regress will need. # This test runs neon specific tests - build_path = os.path.join(pg_distrib_dir, 'build/src/test/regress') src_path = os.path.join(base_dir, 'test_runner/neon_regress') - bindir = os.path.join(pg_distrib_dir, 'bin') schedule = os.path.join(src_path, 'parallel_schedule') - pg_regress = os.path.join(build_path, 'pg_regress') pg_regress_command = [ pg_regress, '--use-existing', '--bindir={}'.format(bindir), - '--dlpath={}'.format(build_path), '--schedule={}'.format(schedule), '--inputdir={}'.format(src_path), ] diff --git a/test_runner/batch_pg_regress/test_pg_regress.py b/test_runner/batch_pg_regress/test_pg_regress.py index b53bc21ca2..eeaeed0820 100644 --- a/test_runner/batch_pg_regress/test_pg_regress.py +++ b/test_runner/batch_pg_regress/test_pg_regress.py @@ -19,19 +19,23 @@ def test_pg_regress(neon_simple_env: NeonEnv, test_output_dir: pathlib.Path, pg_ runpath = test_output_dir / 'regress' (runpath / 'testtablespace').mkdir(parents=True) + # Find the pg_regress binary and --bindir option to pass to it. + proc = pg_bin.run(['pg_config', '--libdir'], capture_output=True) + libdir = proc.stdout.decode().strip() + proc = pg_bin.run(['pg_config', '--bindir'], capture_output=True) + bindir = proc.stdout.decode().strip() + pg_regress = os.path.join(libdir, 'postgresql/pgxs/src/test/regress/pg_regress') + # Compute all the file locations that pg_regress will need. - build_path = os.path.join(pg_distrib_dir, 'build/src/test/regress') src_path = os.path.join(base_dir, 'vendor/postgres/src/test/regress') - bindir = os.path.join(pg_distrib_dir, 'bin') schedule = os.path.join(src_path, 'parallel_schedule') - pg_regress = os.path.join(build_path, 'pg_regress') + dlpath = os.path.join(base_dir, 'build/src/test/regress') pg_regress_command = [ pg_regress, - '--bindir=""', '--use-existing', '--bindir={}'.format(bindir), - '--dlpath={}'.format(build_path), + '--dlpath={}'.format(dlpath), '--schedule={}'.format(schedule), '--inputdir={}'.format(src_path), ] diff --git a/test_runner/fixtures/neon_fixtures.py b/test_runner/fixtures/neon_fixtures.py index 3a6a233208..128a13d297 100644 --- a/test_runner/fixtures/neon_fixtures.py +++ b/test_runner/fixtures/neon_fixtures.py @@ -1372,7 +1372,10 @@ class PgBin: env.update(env_add) return env - def run(self, command: List[str], env: Optional[Env] = None, cwd: Optional[str] = None): + def run(self, + command: List[str], + env: Optional[Env] = None, + **kwargs) -> 'subprocess.CompletedProcess[str]': """ Run one of the postgres binaries. @@ -1389,7 +1392,7 @@ class PgBin: self._fixpath(command) log.info('Running command "{}"'.format(' '.join(command))) env = self._build_env(env) - subprocess.run(command, env=env, cwd=cwd, check=True) + return subprocess.run(command, env=env, check=True, **kwargs) def run_capture(self, command: List[str],