diff --git a/README.rst b/README.rst index 4ab3656f..9aca5734 100644 --- a/README.rst +++ b/README.rst @@ -440,7 +440,22 @@ You can define settings via fixture factory arguments, command line options, or .. note:: - If the ``executable`` is not provided, the plugin attempts to find it by calling ``pg_config``. If that fails, it falls back to a common path like ``/usr/lib/postgresql/14/bin/pg_ctl``. + If the ``executable`` factory argument is not provided, the plugin looks for ``pg_ctl`` first at + the configured path (``--postgresql-exec`` / the ``postgresql_exec`` ini option, + ``/usr/lib/postgresql/14/bin/pg_ctl`` by default), then in the directory reported by + ``pg_config --bindir``. + + Only the ``executable`` factory argument is taken at face value; the other two are + verified to exist before being used. If neither is there, an ``ExecutableMissingException`` + is raised listing the locations that were checked. + +.. note:: + + ``postgresql_proc`` starts and manages a PostgreSQL server for you, so it needs the + PostgreSQL **server** installed locally. Installing only the client libraries + (``libpq-dev`` on Debian/Ubuntu) provides ``pg_config`` but no ``pg_ctl``, and the plugin + will refuse to start. To test against a server you run yourself — a dockerised one, for + instance — use the ``postgresql_noproc`` fixture instead. Examples ======== diff --git a/newsfragments/1031.bugfix.rst b/newsfragments/1031.bugfix.rst new file mode 100644 index 00000000..30b9be8f --- /dev/null +++ b/newsfragments/1031.bugfix.rst @@ -0,0 +1,10 @@ +``pg_ctl`` discovery no longer hands back a path that does not exist. A client-only PostgreSQL +install (Debian/Ubuntu's ``libpq-dev`` without the matching server package) makes +``pg_config --bindir`` point at a directory holding no ``pg_ctl``; that path used to be returned +unchecked, and the failure only surfaced later as ``Could not found ...`` when the executor read the +server version. The path is now verified up front, and the ``ExecutableMissingException`` names the +locations that were checked along with the ways to fix it: install the server package, point at a +``pg_ctl`` with ``--postgresql-exec``, or use ``postgresql_noproc`` against a server you run yourself. + +A broken ``pg_config`` is reported the same way. Only ``FileNotFoundError`` used to be handled, so a +non-executable or failing ``pg_config`` escaped as a raw ``PermissionError``/``CalledProcessError``. diff --git a/newsfragments/1031.docs.rst b/newsfragments/1031.docs.rst new file mode 100644 index 00000000..b6da45db --- /dev/null +++ b/newsfragments/1031.docs.rst @@ -0,0 +1,3 @@ +Corrected the README's description of how ``pg_ctl`` is located — it listed the discovery steps in +the wrong order — and spelled out that ``postgresql_proc`` needs a local PostgreSQL server, pointing +at ``postgresql_noproc`` for dockerised or otherwise externally managed servers. diff --git a/pytest_postgresql/exceptions.py b/pytest_postgresql/exceptions.py index 804aec07..0b3bf350 100644 --- a/pytest_postgresql/exceptions.py +++ b/pytest_postgresql/exceptions.py @@ -1,8 +1,54 @@ """pytest-postgresql's exceptions.""" +from typing import Iterable + class ExecutableMissingException(FileNotFoundError): - """Exception risen when PgConfig was not found.""" + """Exception raised when pg_ctl, needed to start a PostgreSQL server, was not found.""" + + _REMEDIES = ( + "To fix this, either:\n" + " - install the PostgreSQL server package " + "(apt install postgresql, dnf install postgresql-server, brew install postgresql),\n" + " - point the plugin at an existing pg_ctl with the --postgresql-exec command line option, " + "the postgresql_exec ini option, or the executable factory argument,\n" + " - or use the postgresql_noproc fixture to connect to a server you run yourself, " + "a dockerised one for instance." + ) + + @classmethod + def _no_pg_ctl(cls, cause: str, checked: Iterable[str]) -> "ExecutableMissingException": + """Report the locations that were probed, why they came up empty, and what to do.""" + locations = "\n".join(f" - {location}" for location in checked) + return cls( + f"Could not find pg_ctl, which is needed to start a PostgreSQL server.\n" + f"{cause}\n" + f"Checked:\n{locations}\n" + f"{cls._REMEDIES}" + ) + + @classmethod + def pg_config_unusable(cls, reason: str, checked: Iterable[str]) -> "ExecutableMissingException": + """pg_config is missing, not executable, hanging or failing, so it cannot be asked. + + :param reason: concise description of how running pg_config failed, so the reader + can tell "not installed" from "not executable" from "timed out" + :param checked: locations probed so far + """ + return cls._no_pg_ctl( + f"pg_config could not be run either ({reason}), so it could not be used to locate the PostgreSQL binaries.", + checked, + ) + + @classmethod + def not_in_bindir(cls, bindir: str, checked: Iterable[str]) -> "ExecutableMissingException": + """pg_config named a binaries directory, but no pg_ctl lives there.""" + return cls._no_pg_ctl( + f"pg_config reports PostgreSQL binaries live in {bindir}, but there's no pg_ctl there. " + f"That usually means only the PostgreSQL client libraries are installed " + f"(Debian/Ubuntu's libpq-dev, for example), without the matching server package.", + checked, + ) class PostgreSQLUnsupported(Exception): diff --git a/pytest_postgresql/executor.py b/pytest_postgresql/executor.py index 50c429c8..0e7b3553 100644 --- a/pytest_postgresql/executor.py +++ b/pytest_postgresql/executor.py @@ -342,11 +342,12 @@ def version(self) -> Any: """Detect postgresql version.""" try: version_string = subprocess.check_output([self.executable, "--version"]).decode("utf-8") - except FileNotFoundError as ex: + except OSError as ex: raise ExecutableMissingException( - f"Could not find {self.executable}. Is PostgreSQL server installed? " - f"Alternatively pg_config installed might be from different " - f"version that postgresql-server." + f"Could not run {self.executable} to read the PostgreSQL version: {ex}. " + f"Is the PostgreSQL server installed at that location? " + f"Point the plugin at a working pg_ctl with the --postgresql-exec command line option, " + f"the postgresql_exec ini option, or the executable factory argument." ) from ex matches = self.VERSION_RE.search(version_string) assert matches is not None diff --git a/pytest_postgresql/factories/_pg.py b/pytest_postgresql/factories/_pg.py new file mode 100644 index 00000000..8a7b0f1b --- /dev/null +++ b/pytest_postgresql/factories/_pg.py @@ -0,0 +1,75 @@ +"""Internal module for functions handling pg_ctl discovery.""" + +import logging +import os +import platform +import subprocess +from typing import Iterable + +from pytest_postgresql.config import PostgreSQLConfig +from pytest_postgresql.exceptions import ExecutableMissingException + +logger = logging.getLogger(__name__) + +PG_CTL_NAMES = ("pg_ctl.exe", "pg_ctl") if platform.system() == "Windows" else ("pg_ctl",) +"""Names to look for when probing the filesystem for pg_ctl. + +os.path.isfile matches the literal name, so finding the binary on Windows means asking +for pg_ctl.exe. Launching it is more forgiving: Windows appends .exe to an extensionless +program name, which is why the old code could run a path it never checked. +""" + +PG_CONFIG_TIMEOUT = 60 +"""Seconds to wait for pg_config, so a hung binary cannot stall fixture setup. + +Matches PostgreSQLExecutor's default subprocess timeout. +""" + + +def _pg_bindir(checked: Iterable[str]) -> str: + """Ask pg_config where PostgreSQL keeps its binaries. + + pg_config comes from the client development package, which on most distributions + can be installed without the server, so its answer is a hint rather than an answer. + + :param checked: locations probed so far, to name in the error if pg_config can't be run + :raises ExecutableMissingException: pg_config is missing, unusable, hanging or failing. + TimeoutExpired is a SubprocessError, so it needs no branch of its own. + """ + try: + return subprocess.check_output( + ["pg_config", "--bindir"], universal_newlines=True, timeout=PG_CONFIG_TIMEOUT + ).strip() + except (OSError, subprocess.SubprocessError) as ex: + logger.debug("Could not read the binaries directory from pg_config: %s", ex) + raise ExecutableMissingException.pg_config_unusable(f"{type(ex).__name__}: {ex}", checked) from ex + + +def _is_executable(path: str) -> bool: + """Whether path is a file this user can actually run. + + isfile is needed alongside the access check because X_OK on a directory only means + it can be traversed. On Windows os.access ignores X_OK, so this is an existence test. + """ + return os.path.isfile(path) and os.access(path, os.X_OK) + + +def _pg_exe(executable: str | None, config: PostgreSQLConfig) -> str: + """If executable is set, use it. Otherwise best effort to find the executable.""" + # an explicitly passed executable is taken at face value, it's not ours to second-guess + if executable is not None: + return executable + postgresql_ctl = config.exec + # check if that executable exists, as it's not on systems' PATH + if _is_executable(postgresql_ctl): + return postgresql_ctl + checked = [postgresql_ctl] + bindir = _pg_bindir(checked) + for name in PG_CTL_NAMES: + candidate = os.path.join(bindir, name) + if candidate in checked: + continue + checked.append(candidate) + if _is_executable(candidate): + return candidate + raise ExecutableMissingException.not_in_bindir(bindir, checked) diff --git a/pytest_postgresql/factories/process.py b/pytest_postgresql/factories/process.py index f38899d4..33027796 100644 --- a/pytest_postgresql/factories/process.py +++ b/pytest_postgresql/factories/process.py @@ -21,7 +21,6 @@ import os import os.path import platform -import subprocess import tempfile from pathlib import Path from typing import Callable, Iterable @@ -32,8 +31,8 @@ from pytest import FixtureRequest, TempPathFactory from pytest_postgresql.config import PostgreSQLConfig, get_config -from pytest_postgresql.exceptions import ExecutableMissingException from pytest_postgresql.executor import PostgreSQLExecutor +from pytest_postgresql.factories._pg import _pg_exe from pytest_postgresql.janitor import DatabaseJanitor logger = logging.getLogger(__name__) @@ -41,20 +40,6 @@ PortType = port_for.PortType # mypy requires explicit export -def _pg_exe(executable: str | None, config: PostgreSQLConfig) -> str: - """If executable is set, use it. Otherwise best effort to find the executable.""" - postgresql_ctl = executable or config.exec - # check if that executable exists, as it's no on systems' PATH - # only replace it if executable isn't passed manually - if not os.path.exists(postgresql_ctl) and executable is None: - try: - pg_bindir = subprocess.check_output(["pg_config", "--bindir"], universal_newlines=True).strip() - except FileNotFoundError as ex: - raise ExecutableMissingException("Could not find pg_config executable. Is it in system $PATH?") from ex - postgresql_ctl = os.path.join(pg_bindir, "pg_ctl") - return postgresql_ctl - - def _pg_port(port: PortType | None, config: PostgreSQLConfig, excluded_ports: Iterable[int]) -> int: """User specified port, otherwise find an unused port from config.""" pg_port = get_port(port, excluded_ports) or get_port(config.port, excluded_ports) diff --git a/tests/test_executor.py b/tests/test_executor.py index 60673289..7efc24bb 100644 --- a/tests/test_executor.py +++ b/tests/test_executor.py @@ -20,7 +20,7 @@ from pytest_postgresql.config import get_config from pytest_postgresql.exceptions import PostgreSQLUnsupported from pytest_postgresql.executor import PostgreSQLExecutor -from pytest_postgresql.factories import postgresql, postgresql_async, postgresql_proc +from pytest_postgresql.factories import _pg, postgresql, postgresql_async, postgresql_proc from pytest_postgresql.retry import retry @@ -250,7 +250,7 @@ def test_executor_init_with_password( """Test whether the executor initializes properly.""" config = get_config(request) monkeypatch.setenv("LC_ALL", locale) - pg_exe = process._pg_exe(None, config) + pg_exe = _pg._pg_exe(None, config) port = process._pg_port(None, config, []) tmpdir = tmp_path_factory.mktemp(f"pytest-postgresql-{request.node.name}") datadir, logfile_path = process._prepare_dir(tmpdir, port, "test") @@ -274,7 +274,7 @@ def test_executor_init_bad_tmp_path( ) -> None: r"""Test init with \ and space chars in the path.""" config = get_config(request) - pg_exe = process._pg_exe(None, config) + pg_exe = _pg._pg_exe(None, config) port = process._pg_port(None, config, []) tmpdir = tmp_path_factory.mktemp(f"pytest-postgresql-{request.node.name}") / r"a bad\path/" tmpdir.mkdir(parents=True, exist_ok=True) @@ -320,7 +320,9 @@ def test_executor_platform_template_selection( command template based on the platform. """ config = get_config(request) - pg_exe = process._pg_exe(None, config) + # The executor is never started here, only its command inspected, so an arbitrary + # path will do - discovering a real pg_ctl would tie this to an installed server. + pg_exe = "/usr/bin/pg_ctl" port = process._pg_port(-1, config, []) tmpdir = tmp_path_factory.mktemp(f"pytest-postgresql-{request.node.name}") datadir, logfile_path = process._prepare_dir(tmpdir, port, "test") @@ -359,7 +361,7 @@ def test_executor_with_special_chars_in_all_paths( postgres_options all at the same time. """ config = get_config(request) - pg_exe = process._pg_exe(None, config) + pg_exe = _pg._pg_exe(None, config) port = process._pg_port(None, config, []) # Create a tmpdir with spaces in the name tmpdir = tmp_path_factory.mktemp(f"pytest-postgresql-{request.node.name}") / "my test dir" @@ -772,7 +774,7 @@ def test_actual_postgresql_start_windows( correctly starts PostgreSQL on actual Windows systems. """ config = get_config(request) - pg_exe = process._pg_exe(None, config) + pg_exe = _pg._pg_exe(None, config) port = process._pg_port(None, config, []) tmpdir = tmp_path_factory.mktemp(f"pytest-postgresql-{request.node.name}") datadir, logfile_path = process._prepare_dir(tmpdir, port, "test") @@ -811,7 +813,7 @@ def test_actual_postgresql_start_unix( correctly starts PostgreSQL on actual Unix/Linux systems. """ config = get_config(request) - pg_exe = process._pg_exe(None, config) + pg_exe = _pg._pg_exe(None, config) port = process._pg_port(None, config, []) tmpdir = tmp_path_factory.mktemp(f"pytest-postgresql-{request.node.name}") datadir, logfile_path = process._prepare_dir(tmpdir, port, "test") @@ -847,7 +849,7 @@ def test_actual_postgresql_start_darwin( PostgreSQL on actual Darwin/macOS systems and uses the correct locale. """ config = get_config(request) - pg_exe = process._pg_exe(None, config) + pg_exe = _pg._pg_exe(None, config) port = process._pg_port(None, config, []) tmpdir = tmp_path_factory.mktemp(f"pytest-postgresql-{request.node.name}") datadir, logfile_path = process._prepare_dir(tmpdir, port, "test") diff --git a/tests/test_pg_exe.py b/tests/test_pg_exe.py new file mode 100644 index 00000000..e645adac --- /dev/null +++ b/tests/test_pg_exe.py @@ -0,0 +1,159 @@ +"""Tests for the pg_ctl discovery performed by the process fixture factory.""" + +import platform +import subprocess +from pathlib import Path +from unittest.mock import patch + +import pytest + +from pytest_postgresql.config import PostgreSQLConfig +from pytest_postgresql.exceptions import ExecutableMissingException +from pytest_postgresql.factories import _pg + +_PG_PROCESS = "pytest_postgresql.factories._pg" + + +def make_config(exec_path: str) -> PostgreSQLConfig: + """Config with the executable set, the rest being irrelevant for discovery.""" + return PostgreSQLConfig( + exec=exec_path, + host="127.0.0.1", + port=None, + port_search_count=5, + user="postgres", + password=None, + options="", + startparams="-w", + unixsocketdir="/tmp", + dbname="tests", + maintenance_dbname="postgres", + load=[], + load_autocommit=False, + postgres_options="", + drop_test_database=False, + ) + + +@pytest.fixture +def config(tmp_path: Path) -> PostgreSQLConfig: + """Config pointing its executable at a path that does not exist.""" + return make_config(str(tmp_path / "nonexistent" / "bin" / "pg_ctl")) + + +def make_pg_ctl(bindir: Path, name: str = "pg_ctl", mode: int = 0o755) -> Path: + """Create a stand-in for pg_ctl in bindir, executable unless told otherwise.""" + bindir.mkdir(parents=True, exist_ok=True) + pg_ctl = bindir / name + pg_ctl.write_text("") + pg_ctl.chmod(mode) + return pg_ctl + + +def test_explicit_executable_is_not_second_guessed(config: PostgreSQLConfig) -> None: + """An executable passed to the factory is returned even if it does not exist.""" + assert _pg._pg_exe("/nowhere/pg_ctl", config) == "/nowhere/pg_ctl" + + +def test_existing_configured_executable_wins(tmp_path: Path) -> None: + """A configured executable that exists is used without consulting pg_config.""" + pg_ctl = make_pg_ctl(tmp_path / "bin") + config = make_config(str(pg_ctl)) + with patch.object(_pg, "_pg_bindir") as bindir_mock: + assert _pg._pg_exe(None, config) == str(pg_ctl) + bindir_mock.assert_not_called() + + +def test_pg_config_bindir_is_used(tmp_path: Path, config: PostgreSQLConfig) -> None: + """pg_ctl found next to the binaries pg_config points at is used.""" + pg_ctl = make_pg_ctl(tmp_path / "pgconfig-bin") + with patch.object(_pg, "_pg_bindir", return_value=str(pg_ctl.parent)): + assert _pg._pg_exe(None, config) == str(pg_ctl) + + +def test_windows_executable_suffix_is_probed(tmp_path: Path, config: PostgreSQLConfig) -> None: + """On Windows the binary is found as pg_ctl.exe. + + Probing the filesystem does no PATHEXT resolution, so looking for a bare "pg_ctl" + would miss every Windows install and turn discovery into a hard failure there. + """ + pg_ctl = make_pg_ctl(tmp_path / "windows-bin", name="pg_ctl.exe") + with ( + patch.object(_pg, "PG_CTL_NAMES", ("pg_ctl.exe", "pg_ctl")), + patch.object(_pg, "_pg_bindir", return_value=str(pg_ctl.parent)), + ): + assert _pg._pg_exe(None, config) == str(pg_ctl) + + +@pytest.mark.skipif(platform.system() == "Windows", reason="os.access ignores X_OK on Windows") +def test_non_executable_configured_path_is_rejected(tmp_path: Path) -> None: + """A configured pg_ctl that exists but cannot be run is not handed back. + + It used to be accepted, deferring the failure to the version check. + """ + pg_ctl = make_pg_ctl(tmp_path / "bin", mode=0o644) + config = make_config(str(pg_ctl)) + with patch(f"{_PG_PROCESS}.subprocess.check_output", side_effect=FileNotFoundError("pg_config")): + with pytest.raises(ExecutableMissingException, match="Could not find pg_ctl"): + _pg._pg_exe(None, config) + + +@pytest.mark.skipif(platform.system() == "Windows", reason="os.access ignores X_OK on Windows") +def test_non_executable_discovered_path_is_rejected(tmp_path: Path, config: PostgreSQLConfig) -> None: + """A pg_ctl in pg_config's bindir that cannot be run is not handed back either.""" + pg_ctl = make_pg_ctl(tmp_path / "pgconfig-bin", mode=0o644) + with patch.object(_pg, "_pg_bindir", return_value=str(pg_ctl.parent)): + with pytest.raises(ExecutableMissingException) as exc_info: + _pg._pg_exe(None, config) + # the rejected candidate is still worth reporting as somewhere we looked + assert str(pg_ctl) in str(exc_info.value) + + +def test_client_only_install_raises_early(tmp_path: Path, config: PostgreSQLConfig) -> None: + """A pg_config without a matching server fails at discovery time, not at version check. + + This is the libpq-dev-without-postgresql-server case from issue #1031, where the + plugin used to hand back a path that does not exist and only blew up much later. + """ + client_bindir = tmp_path / "client-bin" + client_bindir.mkdir() + with patch.object(_pg, "_pg_bindir", return_value=str(client_bindir)): + with pytest.raises(ExecutableMissingException) as exc_info: + _pg._pg_exe(None, config) + message = str(exc_info.value) + assert str(client_bindir) in message + assert "libpq-dev" in message + assert "postgresql_noproc" in message + assert "--postgresql-exec" in message + + +@pytest.mark.parametrize( + "error", + ( + FileNotFoundError("pg_config"), + PermissionError("pg_config"), + subprocess.CalledProcessError(1, "pg_config"), + subprocess.TimeoutExpired("pg_config", _pg.PG_CONFIG_TIMEOUT), + ), +) +def test_broken_pg_config_surfaces_as_executable_missing(config: PostgreSQLConfig, error: Exception) -> None: + """A missing, unusable, hanging or failing pg_config is reported as a missing executable. + + Only FileNotFoundError used to be handled, so a non-executable pg_config escaped as + a bare PermissionError - the second symptom reported in issue #1031. + """ + with patch(f"{_PG_PROCESS}.subprocess.check_output", side_effect=error): + with pytest.raises(ExecutableMissingException) as exc_info: + _pg._pg_exe(None, config) + message = str(exc_info.value) + assert "Could not find pg_ctl" in message + # the configured path is still reported as somewhere we looked + assert config.exec in message + # how pg_config failed is what separates "not installed" from "not executable" + assert type(error).__name__ in message + + +def test_pg_bindir_returns_stripped_output() -> None: + """The binaries directory is read from pg_config without surrounding whitespace.""" + with patch(f"{_PG_PROCESS}.subprocess.check_output", return_value="/usr/lib/postgresql/17/bin\n"): + assert _pg._pg_bindir([]) == "/usr/lib/postgresql/17/bin"