Skip to content

Generate the meos-sys FFI from the MEOS-API catalog - #6

Open
estebanzimanyi wants to merge 1 commit into
MobilityDB:mainfrom
estebanzimanyi:tooling/catalog-driven-sys
Open

Generate the meos-sys FFI from the MEOS-API catalog#6
estebanzimanyi wants to merge 1 commit into
MobilityDB:mainfrom
estebanzimanyi:tooling/catalog-driven-sys

Conversation

@estebanzimanyi

Copy link
Copy Markdown
Member

meos-sys is a projection of the MEOS-API catalog (the single source of truth every MobilityDB binding derives from) instead of bindgen: sys/codegen.py emits sys/src/generated.rs from meos-idl.json, and sys/build.rs only locates and links libmeos. wrapper.h and prebuilt-bindings/ are removed.

The FFI derives from the stable-1.3 line the crate pins (the 1.3 family set has no -DALL). A drift-check CI job provisions the catalog and asserts the committed generated.rs matches it, so a MEOS API change surfaces instead of silently rotting the FFI. tools/refresh-from-master.sh regenerates it in one command — the same entry point as GoMEOS, MEOS.NET, and the JVM bindings.

The public meos crate (src/) is unchanged — this touches only the sys/ FFI layer, invisible to users.

meos-sys parsed the MEOS headers with bindgen at build time and carried
committed prebuilt-bindings snapshots. Replace both with a projection of the
MEOS-API catalog, the single source of truth every MobilityDB binding derives
from: sys/codegen.py emits sys/src/generated.rs from meos-idl.json, and
sys/build.rs only locates and links libmeos.

The FFI is derived from the stable-1.3 line the crate pins. A drift-check CI job
provisions the catalog and asserts the committed generated.rs matches it, so a
MEOS API change surfaces instead of silently rotting the FFI.
tools/refresh-from-master.sh regenerates it in one command, the same entry point
as every other MobilityDB binding.

The public meos crate (src/) is unchanged; wrapper.h and prebuilt-bindings/ are
removed.
@Davichet-e

Copy link
Copy Markdown
Member

I've asked an LLM to draft in a message my thoughts. Overall I understand the idea but for the Rust case I fail to see the advantage over the current approach.

Review: Generate the meos-sys FFI from the MEOS-API catalog (#6)

I don't think this PR should be merged in its current form. The concerns below are ordered by importance, and all of them are verifiable from the diff itself.

1. It replaces a well-tested framework (bindgen) with a custom, untested Python script — this is the core problem

The PR swaps rust-bindgen — a mature tool maintained by the Rust project, used by thousands of crates, and battle-tested against every C corner case (varargs, unions, bitfields, flexible array members, per-platform integer widths) — for a bespoke 319-line codegen.py with no tests. Its correctness rests entirely on hand-maintained tables (NAMED_ALIASES, CALLBACK_ALIASES, FORCE_VOID_PTR, INLINE_SCALARS): every new MEOS typedef or callback requires editing the script by hand. The comment in the script even admits the callback signature "lives here in the boundary table" — i.e., a hand-maintained shadow ABI. The concrete bugs listed below are direct consequences of this substitution, and they are exactly the class of bugs bindgen exists to prevent.

The "single source of truth" argument is also circular: meos-idl.json is itself produced by parsing the MEOS headers with libclang (MEOS-API's run.py). Headers are still parsed exactly once per pipeline — this PR just relocates the parse to another repository, inserts a JSON intermediary, and replaces bindgen with a homegrown generator.

2. It introduces concrete UB / ABI bugs

  • meos_error lost its varargs. The old bindgen output declared pub fn meos_error(..., format: *const c_char, ...). The new generated.rs declares it without .... Calling a variadic C function through a non-variadic prototype is undefined behavior per the C standard, and concretely broken on Apple silicon (arm64 macOS passes variadic args on the stack, fixed args in registers). The catalog projection simply has no varargs concept.
  • int64 is hardcoded to c_long (and uint64 to c_ulong) in the boundary table, and functions take ::std::os::raw::c_long for int64 parameters (e.g. bigintset_make, rtree_insert). On Windows (LLP64), c_long is 32 bits. bindgen resolved these per-platform from the actual headers; a static table cannot.
  • text silently became an opaque zero-sized struct. In the old bindings text = varlena with accessible fields. Any sys-level consumer touching vl_len_/vl_dat breaks — and inconsistently, since varlena itself is still emitted with fields. "This touches only the sys layer, invisible to users" is false: the sys crate is a published API surface.

3. It deletes the safety net that makes a raw FFI trustworthy

  • Every bindgen-generated struct carried compile-time layout assertions (size_of, align_of, offset_of for every field). All of them are gone. The new generator emits #[repr(C)] structs by trusting the catalog's field list, with zero verification that Rust layout matches what libmeos was actually compiled with. For a raw FFI crate, layout assertions are the single most important correctness property.
  • The bindgen feature — the escape hatch that guaranteed the FFI matched your installed headers — is removed. Every user must now trust that a committed file generated from someone else's catalog matches whatever libmeos pkg-config finds on their machine. The drift check only verifies generated.rs against the catalog, never against the library actually being linked.

4. It breaks the version features while claiming nothing changes

  • Previously, v1_1/v1_2/v1_3 selected prebuilt bindings matching the installed MEOS version. Now all three features link a single FFI "targeting the MEOS 1.3 API," but the APIs genuinely differ: in 1.1, meos_initialize takes (tz_str, err_handler); the new FFI declares meos_initialize(). A v1_1 user calling that against libmeos 1.1 passes garbage arguments — UB, not a link error. Other 1.1/1.2 users get missing-symbol failures for functions that don't exist in their library. The features are kept but turned into lies.
  • Removing the bindgen cargo feature (and bundled's implication of it) is a breaking change to a crate published on crates.io. Cargo features are public API; shipping this as 0.1.x breaks downstream builds.

5. CI and supply-chain regressions

  • uses: MobilityDB/MEOS-API/.github/actions/provision-meos@master is an unpinned, mutable action reference to another repository. CI behavior can change or break at any time when that repo moves, and mutable action refs are a documented supply-chain risk. It also couples this crate's CI availability to an external repository.
  • The whole change lands as a single ~9,000-line commit (generator + generated file + CI + README + deletions) with one participant and no review — effectively unreviewable as a unit.

6. Why this approach brings little benefit to Rust specifically — and why bindgen's advantages are bigger

The catalog approach was designed to solve a problem Rust doesn't have:

  • The catalog solves "no good header-to-binding tool exists" — Rust is the one ecosystem where that's false. For GoMEOS, MEOS.NET, and the JVM bindings, a shared IDL catalog genuinely fills a gap: cgo is painful, P/Invoke and JNI signatures are hand-written, and none of those ecosystems has a first-class C-header importer. Rust has rust-bindgen — maintained by the Rust project, used by thousands of sys crates, built on the same libclang that MEOS-API's run.py uses to produce the catalog in the first place. For Rust, the catalog doesn't remove a header parse; it replaces a native, purpose-built one with a JSON re-encoding of the same parse done elsewhere. The marginal benefit is roughly zero.
  • Rust's FFI is stricter than the other bindings', so the catalog's lossy type model costs Rust the most. Go, .NET, and JVM bindings call through marshalling layers (cgo, P/Invoke, JNI) that tolerate an approximate view of the C API. Rust raw FFI is direct: the declared signature is the ABI contract, with no runtime marshalling to absorb mistakes. That's why bindgen emits target-dependent integer widths, varargs, typed function pointers, and layout assertions. The catalog — a lowest-common-denominator JSON of C type spellings — cannot express varargs (hence the meos_error bug), platform-dependent widths (hence int64 = c_long), or verified layouts. The language that most needs the fidelity is the one this projection serves worst.
  • It goes against sys-crate convention — including this crate's own dependencies. meos-sys depends on geos-sys and proj-sys, both bindgen-based. Following the ecosystem norm means any Rust contributor can maintain the bindings; a bespoke Python generator means only people who learn this repo's tooling can. The claimed benefit — "the same entry point as every other MobilityDB binding" — is uniformity for the upstream maintainer's workflow, paid for by the Rust maintainers and users.
  • The Python generator is a permanent maintenance burden, and a brittle one. codegen.py parses C type spellings with regexes, and its failure mode is silence: an unknown named type becomes an opaque struct, a bare pointer typedef must be manually listed in FORCE_VOID_PTR or it miscompiles, and "anything exotic left" is degraded to void — the generator's own comment says so. Keeping it correct means hand-updating four boundary tables (NAMED_ALIASES, CALLBACK_ALIASES, FORCE_VOID_PTR, INLINE_SCALARS) every time MEOS adds a typedef or callback, with no tests to catch a mistake and no compiler error when one slips through — just a wrong FFI. This repo would own that liability forever, whereas bindgen is somebody else's well-maintained problem.
  • The remaining advertised benefits are achievable with bindgen anyway. Drift detection: regenerate with pinned bindgen in CI and git diff --exit-code. Simpler builds: the crate already shipped prebuilt bindings, so normal builds never invoked bindgen or needed libclang. Once you subtract what bindgen already provides, the PR's unique contribution to Rust users is a new external repo dependency and a hand-maintained boundary table.

Weighing the two sides: bindgen gives ABI fidelity (varargs, unions, function pointer types), per-target correctness, compile-time layout verification, an escape hatch against the user's actual installed headers, ecosystem familiarity, and upstream maintenance by the Rust project — automatically, with no code owned by this repo. The catalog gives workflow uniformity with the other MobilityDB bindings and drift detection, the latter of which bindgen can also provide. The trade is heavily lopsided against this PR.

@Davichet-e

Davichet-e commented Jul 31, 2026

Copy link
Copy Markdown
Member

How meos-rs could leverage MEOS-API without replacing bindgen

To be clear, I think the MEOS-API catalog is valuable — the objection is only to using it as the generator of the Rust ABI. The split that works is: the catalog is the authority on what the MEOS API surface is; bindgen stays the authority on how it maps to ABI-correct Rust. Concretely, here is how we could use MEOS-API and capture every benefit this PR claims:

1. Catalog as bindgen's allowlist

The most legitimate gripe with the current prebuilt bindings is the glibc noise (fopen, _IO_FILE, INT8_MIN, ...) that leaks in through an unfiltered wrapper.h. Instead of hand-writing allowlist regexes, derive allowlist_function / allowlist_type / allowlist_var entries from the functions / structs / enums / macros arrays in meos-idl.json and feed them to bindgen. The result is a clean, MEOS-only generated.rs with all of bindgen's fidelity intact — varargs, layout assertions, per-target integer widths, typed callbacks. This is a ~30-line script instead of a 319-line generator, and its failure mode is safe: a bug produces a missing binding (a compile error downstream), never a wrong one.

2. Catalog as a drift oracle in CI

Keep the drift-check job, but instead of regenerating the FFI from the catalog, cross-check the committed bindgen output against it: functions present in the bindings must match the catalog's signatures (name, arity), and stale extras are flagged. Since the bindings don't yet cover the full catalog (e.g. the cbuffer/npoint/pose families), the check can start one-directional — "nothing in the bindings drifts from the catalog" as a hard failure, "catalog functions missing from the bindings" as a report — and tighten to bidirectional once coverage catches up. A MEOS API change still surfaces as a CI failure — the exact property this PR advertises — with zero ABI risk.

3. provision-meos for the CI version matrix

The provision-meos action is useful infrastructure regardless of how the bindings are generated. Pinned to a SHA or tag (not @master), it can build libmeos 1.1, 1.2, and 1.3 in CI; we then regenerate each version's prebuilt bindings with a pinned bindgen against the provisioned headers and assert git diff --exit-code. That fixes binding rot for all three supported versions — the current PR only addresses 1.3 and silently breaks the other two features.

4. refresh-binding.sh with bindgen as the last leg

The shared refresh driver takes a per-binding BUILD_CMD via refresh.conf — nothing forces that command to be codegen.py. Point it at "run pinned bindgen with catalog-derived allowlists, build, clippy, test." Upstream keeps its one-command-per-binding refresh uniformity across GoMEOS / MEOS.NET / JVM / meos-rs; Rust keeps its native toolchain.

5. Catalog for API-coverage tracking (bonus)

Given that neither the sys bindings nor the high-level meos crate cover 100% of the MEOS API yet, a small script comparing catalog functions against what each layer exposes turns the catalog into a roadmap: it quantifies the gap, shows which function families are missing (cbuffer, npoint, pose, ...), and lets us prioritize — and compare completeness against GoMEOS / MEOS.NET. Purely additive, and a use of the catalog that plays to its strengths.

Summary

Options 1–4 deliver everything this PR sets out to achieve — clean MEOS-only output, drift detection in CI, a single documented refresh entry point, and workflow uniformity with the other bindings — while keeping bindgen's ABI guarantees, layout assertions, cross-platform correctness, and per-version bindings. I'd be happy to help implement this variant.

@estebanzimanyi

Copy link
Copy Markdown
Member Author

Dear David ! Many thanks for this great analysis ! It would be great that you implement the approach you suggested. Looking forward to it !
Esteban

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants