From 6872fa956076e6f62c8b244c328ef3b36e3800cf Mon Sep 17 00:00:00 2001 From: Esteban Zimanyi Date: Fri, 10 Jul 2026 21:51:18 +0200 Subject: [PATCH] Derive the catalog from installed headers so struct layouts resolve MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Struct field types and byte offsets in meos-idl.json come out FFI-accurate when run.py parses the installed MEOS headers instead of the source tree. `make install` writes the generated, self-contained headers — meos_export.h splices postgres_ext_defs.in.h in place of the source's `#include ` — so libclang resolves the bool / uint8 / Datum fields and their real byte offsets rather than degrading every field to int at offset -1 (unusable for the FFI bindings' #[repr(C)] structs and cgo/cffi field access). - The provision-meos action, on the build-libmeos path, stages the MEOS headers into a dedicated prefix and derives the catalog from there. A shared prefix like /usr/local/include carries unrelated system headers (OpenSSL, PostgreSQL) that run.py's header glob would otherwise pull into the catalog; the dedicated prefix holds only MEOS headers. The catalog-only path keeps the source-tree parse. - parser.py adds the compiler's own system include search path (from `clang -E -v`) so / / size_t resolve; size_t is force-included because meos.h uses it without an explicit include. - extract_struct runs field types through _c_spelling, normalising clang's _Bool keyword to the catalog's bool spelling. - pytest.yml builds libmeos, derives the installed catalog, and runs the suite. - tests/test_struct_layout.py guards the resolved Span / STBox / TInstant layouts. Span resolves to spantype:uint8@0, basetype:uint8@8, lower_inc:bool@16, upper_inc:bool@24, padding:char[4]@32, lower:Datum@64, upper:Datum@128. --- .github/actions/provision-meos/action.yml | 45 ++++++++++-- .github/workflows/pytest.yml | 49 +++++-------- parser/extractors.py | 19 +++-- parser/parser.py | 71 ++++++++++++++++++- tests/test_struct_layout.py | 84 +++++++++++++++++++++++ 5 files changed, 223 insertions(+), 45 deletions(-) create mode 100644 tests/test_struct_layout.py diff --git a/.github/actions/provision-meos/action.yml b/.github/actions/provision-meos/action.yml index 4e3fc33..7ce8beb 100644 --- a/.github/actions/provision-meos/action.yml +++ b/.github/actions/provision-meos/action.yml @@ -20,7 +20,7 @@ inputs: outputs: catalog-path: description: Absolute path to the generated meos-idl.json. - value: ${{ steps.catalog.outputs.catalog-path }} + value: ${{ steps.catalog.outputs.catalog-path || steps.catalog_installed.outputs.catalog-path }} libmeos-prefix: description: Install prefix of libmeos when build-libmeos=true (else empty). value: ${{ steps.libmeos.outputs.libmeos-prefix }} @@ -67,12 +67,15 @@ runs: python -m pip install --upgrade pip pip install -r "$REPO_ROOT/requirements.txt" - # Generate the catalog exactly as a downstream consumer does: parse the - # pinned MobilityDB MEOS headers with libclang and emit output/meos-idl.json, - # pointing the @sqlfn/@ingroup extraction (MDB_SRC_ROOT) at the SAME pinned - # checkout so the catalog is reproducibly equivalent to that commit. + # Catalog-only path (no libmeos build): parse the pinned MobilityDB MEOS + # source headers with libclang and emit output/meos-idl.json, pointing the + # @sqlfn/@ingroup extraction (MDB_SRC_ROOT) at the SAME pinned checkout so + # the catalog is reproducibly equivalent to that commit. The source-tree + # headers wrap PostgreSQL types in stubs conditionally, so struct field + # layouts are approximate here; the build path below derives them exactly. - name: Generate the catalog id: catalog + if: inputs.build-libmeos != 'true' shell: bash run: | REPO_ROOT="$(cd "$GITHUB_ACTION_PATH/../../.." && pwd)" @@ -133,6 +136,38 @@ runs: -DPOSTGRESQL_PG_CONFIG=/usr/lib/postgresql/17/bin/pg_config \ .. 2>&1 | tee "$RUNNER_TEMP/cmake-configure.log" make -j "$(nproc)" + # Stage the MEOS headers alone into a clean prefix for the catalog parse + # BEFORE the shared install: /usr/local/include on the runner carries + # unrelated system headers (OpenSSL, PostgreSQL, …) that run.py's header + # glob would otherwise pull into the catalog; this prefix holds only MEOS + # headers. Runs before `sudo make install` so the build directory (where + # cmake writes install_manifest.txt) is still writable by the runner. + cmake --install . --prefix "$RUNNER_TEMP/meos-headers" + echo "installed MEOS headers under $RUNNER_TEMP/meos-headers/include" sudo make install echo "libmeos-prefix=/usr/local" >> "$GITHUB_OUTPUT" echo "libmeos installed under /usr/local" + + # Build path: derive the catalog from the INSTALLED headers. `make install` + # writes the generated, self-contained headers (meos_export.h spliced with + # postgres_ext_defs.in.h in place of the source's `#include `), + # so libclang resolves every struct field to its real C type and byte + # offset — the FFI-accurate layout the native bindings' `#[repr(C)]` structs + # and cgo/cffi field access require. MDB_SRC_ROOT stays the source checkout + # for the @sqlfn/@ingroup maps. + - name: Generate the catalog from installed headers + id: catalog_installed + if: inputs.build-libmeos == 'true' + shell: bash + run: | + REPO_ROOT="$(cd "$GITHUB_ACTION_PATH/../../.." && pwd)" + cd "$REPO_ROOT" + MDB_SRC_ROOT="$GITHUB_WORKSPACE/_mobilitydb_src" \ + python3 run.py "$RUNNER_TEMP/meos-headers/include" + CATALOG="$REPO_ROOT/output/meos-idl.json" + if [ ! -s "$CATALOG" ]; then + echo "::error::catalog not generated or empty at $CATALOG" + exit 1 + fi + echo "catalog-path=$CATALOG" >> "$GITHUB_OUTPUT" + echo "Catalog written from installed headers: $CATALOG ($(wc -c < "$CATALOG") bytes)" diff --git a/.github/workflows/pytest.yml b/.github/workflows/pytest.yml index a447492..56f57e8 100644 --- a/.github/workflows/pytest.yml +++ b/.github/workflows/pytest.yml @@ -2,12 +2,11 @@ name: tests # Dogfoods the reusable provision-meos composite action, the single step every # binding uses to obtain the MEOS artifacts from one pinned MobilityDB commit. -# Job 1 derives output/meos-idl.json through the action and runs the pytest -# suite (type recovery + portable alias parity) against it — the same catalog -# correctness signal master had before, now produced by the shared action. -# Job 2 exercises the action's optional libmeos path (all families, incl. -# pgPointCloud via PGDG PG17) so the whole action is validated before any -# binding depends on it. +# The action builds and installs all-families libmeos (incl. pgPointCloud via +# PGDG PG17) and derives output/meos-idl.json from the installed, self-contained +# headers, then the pytest suite (type recovery + struct layout + portable +# alias parity) runs against that catalog — the same catalog correctness signal +# master had before, now produced by the shared action and FFI-accurate. on: pull_request: @@ -17,19 +16,26 @@ on: jobs: tests: - name: Regenerate catalog + pytest + name: Build libmeos + regenerate catalog + pytest runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 - # Derive output/meos-idl.json from MobilityDB master via the shared - # action — the exact step a downstream binding runs. - - name: Provision the MEOS catalog + # Build + install all-families libmeos and derive the catalog from the + # installed headers — the exact step a native/FFI binding runs. + - name: Provision the MEOS catalog and libmeos id: provision uses: ./.github/actions/provision-meos with: mobilitydb-ref: master - build-libmeos: "false" + build-libmeos: "true" + + - name: Assert libmeos.so is installed and print the family summary + run: | + test -f "${{ steps.provision.outputs.libmeos-prefix }}/lib/libmeos.so" + echo "libmeos installed at ${{ steps.provision.outputs.libmeos-prefix }}/lib/libmeos.so" + echo "----- cmake family summary -----" + grep -E 'Including|Excluding' "$RUNNER_TEMP/cmake-configure.log" || true - name: Install pytest run: pip install pytest @@ -48,24 +54,3 @@ jobs: name: meos-idl path: output/meos-idl.json if-no-files-found: error - - native: - name: Build libmeos (all families) - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v4 - - # Same action, optional libmeos path: build + install all families. - - name: Provision the MEOS catalog and libmeos - id: provision - uses: ./.github/actions/provision-meos - with: - mobilitydb-ref: master - build-libmeos: "true" - - - name: Assert libmeos.so is installed and print the family summary - run: | - test -f "${{ steps.provision.outputs.libmeos-prefix }}/lib/libmeos.so" - echo "libmeos installed at ${{ steps.provision.outputs.libmeos-prefix }}/lib/libmeos.so" - echo "----- cmake family summary -----" - grep -E 'Including|Excluding' "$RUNNER_TEMP/cmake-configure.log" || true diff --git a/parser/extractors.py b/parser/extractors.py index 12950c9..17f0ee6 100644 --- a/parser/extractors.py +++ b/parser/extractors.py @@ -64,16 +64,21 @@ def find_unlisted_foreign_structs(idl) -> list: return sorted(elaborated - bare - set(_EXTERNAL_OPAQUE_STRUCTS)) +def _bool_norm(spelling: str) -> str: + # Normalise clang's ``_Bool`` keyword to the ``bool`` spelling the catalog + # uses, token-wise so pointer/const forms (``_Bool *``, ``const _Bool *``) + # normalise too — clang spells the pointee's underlying keyword, not the + # ``bool`` macro. + return re.sub(r"\b_Bool\b", "bool", spelling) + + def _c_spelling(ty) -> str: # Return the declared C spelling, with ``_Bool`` normalised to ``"bool"``. # Two bool representations arise depending on which postgres_int_defs.h is # in play: # - PostgreSQL headers: ``typedef char bool`` -> spelling already ``"bool"`` # - Stub header: ``#define bool _Bool`` -> spelling is ``"_Bool"`` - spelling = ty.spelling - if spelling == "_Bool": - return "bool" - return _demote_external_opaque(spelling) + return _demote_external_opaque(_bool_norm(ty.spelling)) # Canonical spellings of plain C scalars/builtins. @@ -131,8 +136,8 @@ def _canonical_c_spelling(ty) -> str: return "bool" preserved = _preserved_opaque(ty) if preserved is not None: - return _demote_external_opaque(preserved) - return _demote_external_opaque(_canonical_spelling(ty)) + return _bool_norm(_demote_external_opaque(preserved)) + return _bool_norm(_demote_external_opaque(_canonical_spelling(ty))) # ----------------------------------------------------------------------------- @@ -217,7 +222,7 @@ def extract_struct(node) -> dict: "fields": [ { "name": f.spelling, - "cType": f.type.spelling, + "cType": _c_spelling(f.type), "offset_bits": node.type.get_offset(f.spelling), } for f in node.get_children() diff --git a/parser/parser.py b/parser/parser.py index 3d9def3..bd6a198 100644 --- a/parser/parser.py +++ b/parser/parser.py @@ -1,6 +1,7 @@ import re import clang.cindex import json +import subprocess import tempfile import os from pathlib import Path @@ -8,6 +9,74 @@ from parser.extractors import extract_function, extract_struct, extract_enum from parser.type_resolver import resolve_idl_types + +def _compiler_system_includes() -> list[str]: + """The C compiler's system header search path (builtin resource dir, + ``/usr/local/include``, ``/usr/include/``, ``/usr/include``). + + The pip ``libclang`` wheel ships the shared library but *not* a configured + system include path, so the amalgamation fails to find ```` / + ```` / ``size_t`` and every ``bool`` / ``uint8`` / ``Datum`` field + degrades to ``int`` with ``get_offset`` returning ``-1``. ``provision-meos`` + installs ``clang``, so ask the installed compiler for its own search list + (``clang -E -v``) rather than guessing paths; every entry there is added as + ``-isystem`` so a real MEOS compile and this parse see the same headers. + """ + for cc in ("clang", "cc", "gcc"): + try: + out = subprocess.run( + [cc, "-E", "-x", "c", "-v", os.devnull], + capture_output=True, text=True, check=True).stderr + except Exception: + continue + dirs, capture = [], False + for line in out.splitlines(): + if line.startswith("#include <...> search starts here:"): + capture = True + continue + if line.startswith("End of search list."): + break + if capture: + d = line.strip() + if d and os.path.isdir(d): + dirs.append(d) + if dirs: + return dirs + return [] + + +def _clang_extra_args() -> list[str]: + """Include paths clang needs to resolve MEOS types to their real C types. + + Without a configured search path the MEOS headers' standard-library + includes (```` / ```` / ``size_t``) fail, degrading + every struct field to ``int`` at offset ``-1`` — unusable for the FFI + bindings (``#[repr(C)]`` structs, cgo/cffi field access). Add the + compiler's own system search path, and the external family (H3 / GDAL / + PROJ) headers when present. ``size_t`` is force-included via + ``-include stddef.h`` because ``meos.h`` uses it without an explicit + ``#include``. + + The catalog is derived from the *installed* MEOS headers (the generated + ``meos_export.h`` written by ``make install``), which splice + ``postgres_ext_defs.in.h`` in place of the source tree's + ``#include `` — so they are self-contained and no PostgreSQL + server headers are needed here. + """ + args: list[str] = [] + + for d in _compiler_system_includes(): + args.append(f"-isystem{d}") + if args: + args += ["-include", "stddef.h"] + + for d in ("/usr/include/h3", "/usr/include/gdal", "/usr/include/proj"): + if os.path.isdir(d): + args.append(f"-isystem{d}") + + return args + + def merge_meta(idl: dict, meta_path: Path) -> dict: with open(meta_path) as f: meta = json.load(f) @@ -36,7 +105,7 @@ def parse_meos(entry: Path, include_dir: Path) -> dict: # it, and an undefined ``UNUSED`` makes clang error on the declarator and # silently drop the remaining parameters of that prototype. "-DUNUSED=__attribute__((unused))", - ]) + ] + _clang_extra_args()) # Collect all .h files belonging to the project own_files = {str(p.resolve()) for p in include_dir.glob("**/*.h")} diff --git a/tests/test_struct_layout.py b/tests/test_struct_layout.py new file mode 100644 index 0000000..eef2fa2 --- /dev/null +++ b/tests/test_struct_layout.py @@ -0,0 +1,84 @@ +"""Regression tests for FFI-accurate struct layouts in the IDL. + +The amalgamation parse must resolve struct field types to their real C types +(``uint8`` / ``bool`` / ``Datum`` — not the ``int`` the preprocessor degrades +them to when ```` is unreachable) and must compute real byte offsets +(not ``-1``). Both depend on ``parser.parser._clang_extra_args`` putting clang's +builtin resource dir (``stdbool.h`` / ``stddef.h`` / ``stdint.h``) on the parse +path. If that regresses, every field collapses to ``int`` with ``offset_bits == +-1`` and the FFI bindings (``#[repr(C)]`` structs, cgo/cffi field access) lose +their layout — so these assertions fail loudly. + +Field types are also normalised to the catalog's ``bool`` spelling (clang spells +the underlying ``_Bool`` keyword) and offsets are in bits. + +The IDL is generated, not committed; run ``python run.py`` first. +""" +import json +import unittest +from pathlib import Path + +IDL = Path(__file__).resolve().parents[1] / "output" / "meos-idl.json" + + +class StructLayoutTests(unittest.TestCase): + def setUp(self): + if not IDL.exists(): + self.skipTest(f"{IDL} not generated; run `python run.py` first") + idl = json.loads(IDL.read_text()) + self.by_name = {s["name"]: s for s in idl["structs"]} + + def _fields(self, name): + self.assertIn(name, self.by_name, f"{name} missing from IDL structs") + return {f["name"]: f for f in self.by_name[name]["fields"]} + + def test_span_layout_resolved(self): + # Pre-fix every field was ``int`` at offset ``-1``. + f = self._fields("Span") + self.assertEqual((f["spantype"]["cType"], f["spantype"]["offset_bits"]), + ("uint8", 0)) + self.assertEqual((f["basetype"]["cType"], f["basetype"]["offset_bits"]), + ("uint8", 8)) + self.assertEqual(f["lower_inc"]["cType"], "bool") + self.assertEqual(f["upper_inc"]["cType"], "bool") + self.assertEqual((f["lower"]["cType"], f["lower"]["offset_bits"]), + ("Datum", 64)) + self.assertEqual((f["upper"]["cType"], f["upper"]["offset_bits"]), + ("Datum", 128)) + + def test_stbox_layout_resolved(self): + f = self._fields("STBox") + self.assertEqual(f["period"]["cType"], "Span") + self.assertEqual(f["period"]["offset_bits"], 0) + self.assertEqual((f["xmin"]["cType"], f["xmin"]["offset_bits"]), + ("double", 192)) + + def test_tinstant_layout_resolved(self): + f = self._fields("TInstant") + self.assertEqual((f["temptype"]["cType"], f["temptype"]["offset_bits"]), + ("uint8", 32)) + self.assertEqual((f["t"]["cType"], f["t"]["offset_bits"]), + ("TimestampTz", 64)) + self.assertEqual((f["value"]["cType"], f["value"]["offset_bits"]), + ("Datum", 128)) + + def test_no_bool_keyword_left_as_underscore_Bool(self): + # Clang spells the keyword ``_Bool``; the catalog normalises to ``bool``. + for s in self.by_name.values(): + for fld in s["fields"]: + self.assertNotIn("_Bool", fld["cType"], + f"{s['name']}.{fld['name']} not normalised") + + def test_core_structs_have_real_offsets(self): + # No CORE-family struct field may keep the degraded ``-1`` offset. + for s in self.by_name.values(): + if s.get("family") != "CORE": + continue + for fld in s["fields"]: + self.assertGreaterEqual( + fld["offset_bits"], 0, + f"{s['name']}.{fld['name']} has unresolved offset") + + +if __name__ == "__main__": + unittest.main()