diff --git a/.github/scripts/ubuntu/install-pkg.sh b/.github/scripts/ubuntu/install-pkg.sh index abdfe4f76b..ebe44d1877 100755 --- a/.github/scripts/ubuntu/install-pkg.sh +++ b/.github/scripts/ubuntu/install-pkg.sh @@ -7,6 +7,10 @@ EPHEMERAL=" libexpat-dev libgles2-mesa-dev libxkbcommon-dev + python3-pytest + python3-pytest-timeout + python3-pytest-xdist + valgrind x11-utils x11-xserver-utils xauth diff --git a/.github/scripts/ubuntu/run-pyxtest-valgrind.sh b/.github/scripts/ubuntu/run-pyxtest-valgrind.sh new file mode 100755 index 0000000000..eded353f9f --- /dev/null +++ b/.github/scripts/ubuntu/run-pyxtest-valgrind.sh @@ -0,0 +1,78 @@ +#!/bin/bash +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 Enrico Weigelt, metux IT consult +# +# Build a debug Xvfb and run the pyxtest suite under valgrind memcheck. +# +# This is a dedicated, lightweight job: unlike run-xserver-build-and-test.sh +# it does not build Xorg/Xephyr/Xnest or fetch XTS/piglit/goxproto — pyxtest +# only exercises Xvfb here, and valgrind is slow enough already that we don't +# want to pay for the rest of the matrix too. It does still need a from-source +# xorgproto (util.sh's build_meson), since the Ubuntu runner image's +# presentproto is too old for meson.build's '>= 1.4' requirement. + +set -e + +if [ ! "$MESON_BUILDDIR" ]; then + echo "missing MESON_BUILDDIR" >&2 + exit 1 +fi + +if [ ! "$X11_BUILD_DIR" ]; then + echo "missing X11_BUILD_DIR" >&2 + exit 1 +fi + +. .github/scripts/util.sh + +echo "=== MESON_BUILDDIR=$MESON_BUILDDIR" +echo "=== X11_PREFIX=$X11_PREFIX" +echo "=== PKG_CONFIG_PATH=$PKG_CONFIG_PATH" + +mkdir -p "$X11_BUILD_DIR" +( + cd "$X11_BUILD_DIR" + build_meson xorgproto $(fdo_mirror xorgproto) "$PKG_XORGPROTO_REF" +) + +meson setup "$MESON_BUILDDIR" $MESON_ARGS +meson compile -C "$MESON_BUILDDIR" + +export XSERVER_BUILDDIR="$PWD/$MESON_BUILDDIR" +export XVFB_PATH="$XSERVER_BUILDDIR/hw/vfb/Xvfb" +export PYTHONDONTWRITEBYTECODE=1 + +# Per-test valgrind XML (+ a .txt summary when a test has findings) goes +# here, independent of pass/fail — see test/pyxtest/conftest.py's +# --valgrind-log-dir. Without this, valgrind's own output never leaves an +# ephemeral temp file, so a green run leaves no trace of what it checked +# and a failing run's report isn't visible without an API round-trip. +export VALGRIND_LOG_DIR="$PWD/valgrind-logs" +mkdir -p "$VALGRIND_LOG_DIR" + +# Run without -e so a failing/valgrind-tripped test still lets us write the +# job summary below before propagating the real exit code. +set +e +pytest -v --tb=short -n auto --timeout=300 --valgrind test/pyxtest/ +PYTEST_STATUS=$? +set -e + +if [ "${GITHUB_STEP_SUMMARY:-}" ]; then + { + echo "### Valgrind memcheck summary" + echo + shopt -s nullglob + findings=("$VALGRIND_LOG_DIR"/*.txt) + if [ "${#findings[@]}" -eq 0 ]; then + echo "No valgrind findings. Full per-test XML reports (leak-only included) are in the \`valgrind-logs\` artifact." + else + echo "${#findings[@]} test/server combination(s) with valgrind findings (full detail in the \`valgrind-logs\` artifact):" + echo + for f in "${findings[@]}"; do + echo "- \`$(basename "${f%.txt}")\`" + done + fi + } >>"$GITHUB_STEP_SUMMARY" +fi + +exit "$PYTEST_STATUS" diff --git a/.github/workflows/build-xserver.yml b/.github/workflows/build-xserver.yml index c7ec1969d6..a70abe8704 100644 --- a/.github/workflows/build-xserver.yml +++ b/.github/workflows/build-xserver.yml @@ -249,6 +249,57 @@ jobs: __BUILD/meson-logs/* __BUILD/test/piglit-results/* + xserver-build-ubuntu-valgrind: + # Dedicated lane for the pyxtest suite under valgrind memcheck + # (out-of-bounds reads/writes, use-after-free, uninitialised-value + # use). Kept separate from xserver-build-ubuntu* because valgrind is + # slow and this only needs Xvfb, not the full DDX/XTS/piglit matrix. + env: + MESON_ARGS: --buildtype=debug -Dprefix=/usr -Dxorg=false -Dxvfb=true -Dxnest=false -Dxephyr=false -Dwerror=true + runs-on: ubuntu-latest + needs: ubuntu-fetch-pkg + steps: + - name: Check out repository code + uses: actions/checkout@v6 + + - uses: ./.github/actions/ubuntu-install-pkg + + - name: prepare build environment + run: | + MACHINE=`gcc -dumpmachine` + echo "MACHINE=$MACHINE" >> "$GITHUB_ENV" + echo "PKG_CONFIG_PATH=$X11_PREFIX/share/pkgconfig:$X11_PREFIX/lib/$MACHINE/pkgconfig:$X11_PREFIX/lib/pkgconfig:$PKG_CONFIG_PATH" >> "$GITHUB_ENV" + + - name: X11 prereq cache + uses: actions/cache@v5 + with: + path: | + ${{ env.X11_PREFIX }} + key: ${{ runner.name }}-x11-deps-xorgproto-${{ hashFiles('.github/scripts/ubuntu/run-pyxtest-valgrind.sh') }} + restore-keys: ${{ runner.name }}-x11-deps-xorgproto- + + - name: build and run pyxtest under valgrind + run: .github/scripts/ubuntu/run-pyxtest-valgrind.sh + + - name: archive build logs + uses: actions/upload-artifact@v4 + with: + name: build-logs-valgrind + path: | + __BUILD/meson-logs/* + + - name: archive valgrind reports + # Upload even when the pytest step above failed - a failing + # run is exactly when the report is most needed, and it's + # what triggered writing this step's `if` in the first place. + if: always() + uses: actions/upload-artifact@v4 + with: + name: valgrind-logs + path: | + valgrind-logs/* + if-no-files-found: ignore + ubuntu-deps-image: # Content-addressed base image (apt deps + from-source prereqs baked in), # consumed by build-sdk-image below. The tag is a hash of the image's diff --git a/test/pyxtest/README.md b/test/pyxtest/README.md index a32f843ad3..ae63f697ad 100644 --- a/test/pyxtest/README.md +++ b/test/pyxtest/README.md @@ -107,6 +107,7 @@ pytest test/pyxtest/ -v --server-type=xvfb --server-type=xwayland |--------------------------------|-------------------------------------------| | `--valgrind` | Run all X servers under valgrind memcheck | | `--valgrind-suppressions=PATH` | Path to a valgrind suppressions file | +| `--valgrind-log-dir=PATH` | Persist a per-test valgrind XML report here (plus a `.txt` summary when there are findings), independent of pass/fail. Also settable via `VALGRIND_LOG_DIR`. | | `--server-type=TYPE` | Server type to test (`xvfb`, `xwayland`, `xorg`). Repeatable. Default: `xvfb` | | `--server-path=PATH` | Explicit path to the X server binary | | `--display=DISPLAY` | Connect to an existing server instead of starting one. Accepts `:N` or `N`. Mutually exclusive with `--valgrind` and `--server-path` | @@ -123,6 +124,8 @@ The server binary is located by checking, in order: `VALGRIND_SUPPRESSIONS` can point to a suppressions file. +`VALGRIND_LOG_DIR` is the environment-variable form of `--valgrind-log-dir`. + `XSERVER_ASAN` set to `1` indicates the server binary was built with AddressSanitizer. This is set automatically by meson when `-Db_sanitize=address` is used. It can also be set manually. diff --git a/test/pyxtest/conftest.py b/test/pyxtest/conftest.py index cf8bf141d5..1368076684 100644 --- a/test/pyxtest/conftest.py +++ b/test/pyxtest/conftest.py @@ -3,7 +3,9 @@ # pytest configuration and fixtures for X server testing. import os +import re import shutil +import warnings import pytest from pathlib import Path @@ -23,6 +25,16 @@ def pytest_addoption(parser): default=None, help="Path to valgrind suppressions file", ) + parser.addoption( + "--valgrind-log-dir", + default=None, + help="Directory to persist a per-test valgrind XML report into " + "(also honours the VALGRIND_LOG_DIR environment variable). " + "Written for every test run under valgrind, independent of " + "pass/fail, since the XML otherwise only exists in an ephemeral " + "temp file. A short human-readable .txt summary is written " + "alongside whenever valgrind reported anything.", + ) parser.addoption( "--server-type", action="append", @@ -61,6 +73,10 @@ def pytest_configure(config): "markers", "swapped_client: mark test as requiring a byte-swapped client" ) + log_dir = get_valgrind_log_dir(config) + if log_dir is not None: + log_dir.mkdir(parents=True, exist_ok=True) + # Validate --display against conflicting options display = config.getoption("--display", default=None) if display is not None: @@ -113,6 +129,24 @@ def get_valgrind_suppressions(config) -> Path | None: return None +def get_valgrind_log_dir(config) -> Path | None: + """Find the directory to persist per-test valgrind reports into.""" + explicit = config.getoption("--valgrind-log-dir") + if explicit: + return Path(explicit) + + env = os.environ.get("VALGRIND_LOG_DIR") + if env: + return Path(env) + + return None + + +def _sanitize_test_id(nodeid: str) -> str: + """Turn a pytest nodeid into a safe filename fragment.""" + return re.sub(r"[^A-Za-z0-9._-]+", "_", nodeid) + + def is_valgrind_available() -> bool: """Check if valgrind is available on the system.""" return shutil.which("valgrind") is not None @@ -163,6 +197,36 @@ def pytest_generate_tests(metafunc): metafunc.parametrize("xserver", server_types, indirect=True) +def _persist_valgrind_report(request, server_type, xml_path, errors): + """Copy a test's valgrind XML into --valgrind-log-dir, if configured. + + Runs for every test executed under valgrind, independent of whether + the test passed, failed, or only found leaks (which don't fail the + test) - otherwise the XML never leaves the ephemeral temp file it + was written to. Writes a short .txt summary alongside whenever + valgrind reported anything, so CI can surface findings without + parsing XML. + """ + log_dir = get_valgrind_log_dir(request.config) + if log_dir is None: + return + + stem = f"{_sanitize_test_id(request.node.nodeid)}-{server_type}" + dest = log_dir / f"{stem}.xml" + try: + shutil.copyfile(xml_path, dest) + except OSError as e: + warnings.warn(f"Failed to persist valgrind XML for {stem}: {e}", stacklevel=2) + return + + if errors: + summary = log_dir / f"{stem}.txt" + summary.write_text( + f"{len(errors)} valgrind finding(s) for {request.node.nodeid} " + f"({server_type}):\n\n" + "\n\n".join(str(e) for e in errors) + "\n" + ) + + def _start_server(request, server_type, log_file=None): """Start an X server of the given type for a test. @@ -212,6 +276,11 @@ def _start_server(request, server_type, log_file=None): valgrind_errors = server.stop() + if use_valgrind and server.valgrind_xml_path is not None: + _persist_valgrind_report( + request, server_type, server.valgrind_xml_path, valgrind_errors + ) + if use_asan and asan_errors: msg = f"AddressSanitizer found {len(asan_errors)} error(s):\n\n" msg += "\n\n".join(str(e) for e in asan_errors) diff --git a/test/pyxtest/xserver.py b/test/pyxtest/xserver.py index f106d6f35e..1a5e6ac0e7 100644 --- a/test/pyxtest/xserver.py +++ b/test/pyxtest/xserver.py @@ -451,6 +451,17 @@ class XServerProcess: pass return "" + @property + def valgrind_xml_path(self) -> Path | None: + """Path to this server's valgrind XML output, if valgrind was used. + + Stays valid after :meth:`stop`, so callers can persist it + elsewhere (the file itself is not deleted automatically). + """ + if self._valgrind_xml_file is None: + return None + return Path(self._valgrind_xml_file.name) + def check_valgrind_errors(self): """Parse and assert no valgrind errors occurred.""" if self._valgrind_xml_file is None: