Merge #2771

2771: ci: run pyo3-ffi-check r=davidhewitt a=messense



Co-authored-by: messense <messense@icloud.com>
Co-authored-by: David Hewitt <1939362+davidhewitt@users.noreply.github.com>

.github/workflows/build.yml

@@ -131,6 +131,29 @@ jobs:
           nox -s test-py
         env:
           CARGO_TARGET_DIR: ${{ github.workspace }}/target
+      
+      - uses: dorny/paths-filter@v2
+        # pypy 3.7 and 3.8 are not PEP 3123 compliant so fail checks here
+        if: ${{ inputs.rust == 'stable' && inputs.python-version != 'pypy-3.7' && inputs.python-version != 'pypy-3.8' }}
+        id: ffi-changes
+        with:
+          filters: |
+            changed:
+              - 'pyo3-ffi/**'
+              - 'pyo3-ffi-check/**'
+              - '.github/workflows/ci.yml'
+              - '.github/workflows/build.yml'
+      
+      - run: cargo doc -p pyo3-ffi
+        working-directory: pyo3-ffi-check
+        if: ${{ steps.ffi-changes.outputs.changed == 'true' && inputs.rust == 'stable' && inputs.python-version != 'pypy-3.7' && inputs.python-version != 'pypy-3.8' }}
+
+      - name: Run pyo3-ffi-check
+        run: cargo run
+        working-directory: pyo3-ffi-check
+        # Allow failure on PyPy for now
+        continue-on-error: ${{ startsWith(inputs.python-version, 'pypy') }}
+        if: ${{ steps.ffi-changes.outputs.changed == 'true' && inputs.rust == 'stable' && inputs.python-version != 'pypy-3.7' && inputs.python-version != 'pypy-3.8' }}
 
       - name: Test cross compilation
         if: ${{ inputs.os == 'ubuntu-latest' && inputs.python-version == '3.9' }}
@@ -177,6 +200,7 @@ jobs:
           export PYO3_CROSS_PYTHON_VERSION=3.9
           cargo build --manifest-path examples/maturin-starter/Cargo.toml --features generate-import-lib --target x86_64-pc-windows-gnu
           cargo xwin build --manifest-path examples/maturin-starter/Cargo.toml --features generate-import-lib --target x86_64-pc-windows-msvc
+
       - name: Test cross compile to Windows with maturin
         if: ${{ inputs.os == 'ubuntu-latest' && inputs.python-version == '3.8' }}
         uses: PyO3/maturin-action@v1

pyo3-ffi-check/Cargo.toml

@@ -0,0 +1,22 @@
+[package]
+name = "pyo3-ffi-check"
+version = "0.1.0"
+edition = "2021"
+publish = false
+
+[dependencies]
+pyo3-ffi-check-macro = { path = "./macro" }
+memoffset = "0.7.0"
+
+[dependencies.pyo3-ffi]
+path = "../pyo3-ffi"
+features = ["extension-module"]  # A lazy way of skipping linking in most cases (as we don't use any runtime symbols)
+
+[workspace]
+members = [
+    "macro"
+]
+
+[build-dependencies]
+bindgen = "0.63.0"
+pyo3-build-config = { path = "../pyo3-build-config" }

pyo3-ffi-check/README.md

@@ -0,0 +1,7 @@
+# pyo3-ffi-check
+
+This is a simple program which compares ffi definitions from `pyo3-ffi` against those produced by `bindgen`.
+
+If any differ in size, these are printed to stdout and a the process will exit nonzero.
+
+The main purpose of this program is to run a scheduled weekly job in Github actions to catch possible errors in PyO3's ffi definitions.

pyo3-ffi-check/build.rs

@@ -0,0 +1,37 @@
+use std::env;
+use std::path::PathBuf;
+
+fn main() {
+    let config = pyo3_build_config::get();
+    let python_include_dir = config
+        .run_python_script(
+            "import sysconfig; print(sysconfig.get_config_var('INCLUDEPY'), end='');",
+        )
+        .expect("failed to get lib dir");
+
+    println!("cargo:rerun-if-changed=wrapper.h");
+    dbg!(format!("-I{python_include_dir}"));
+
+    let bindings = bindgen::Builder::default()
+        .header("wrapper.h")
+        .clang_arg(format!("-I{python_include_dir}"))
+        .parse_callbacks(Box::new(bindgen::CargoCallbacks))
+        // blocklist some values which apparently have conflicting definitions on unix
+        .blocklist_item("FP_NORMAL")
+        .blocklist_item("FP_SUBNORMAL")
+        .blocklist_item("FP_NAN")
+        .blocklist_item("FP_INFINITE")
+        .blocklist_item("FP_INT_UPWARD")
+        .blocklist_item("FP_INT_DOWNWARD")
+        .blocklist_item("FP_INT_TOWARDZERO")
+        .blocklist_item("FP_INT_TONEARESTFROMZERO")
+        .blocklist_item("FP_INT_TONEAREST")
+        .blocklist_item("FP_ZERO")
+        .generate()
+        .expect("Unable to generate bindings");
+
+    let out_path = PathBuf::from(env::var("OUT_DIR").unwrap());
+    bindings
+        .write_to_file(out_path.join("bindings.rs"))
+        .expect("Couldn't write bindings!");
+}

pyo3-ffi-check/macro/Cargo.toml

@@ -0,0 +1,13 @@
+[package]
+name = "pyo3-ffi-check-macro"
+version = "0.1.0"
+edition = "2021"
+
+[lib]
+proc-macro = true
+
+[dependencies]
+glob = "0.3"
+quote = "1"
+proc-macro2 = "1"
+scraper = "0.13"

pyo3-ffi-check/macro/src/lib.rs

@@ -0,0 +1,146 @@
+use std::{env, fs, path::PathBuf};
+
+use proc_macro2::{Ident, Span, TokenStream, TokenTree};
+use quote::quote;
+
+/// Macro which expands to multiple macro calls, one per pyo3-ffi struct.
+#[proc_macro]
+pub fn for_all_structs(input: proc_macro::TokenStream) -> proc_macro::TokenStream {
+    let input: TokenStream = input.into();
+    let mut input = input.into_iter();
+
+    let macro_name = match input.next() {
+        Some(TokenTree::Ident(i)) => i,
+        _ => {
+            return quote!(compile_error!(
+                "for_all_structs!() takes only a single ident as input"
+            ))
+            .into()
+        }
+    };
+
+    if !input.next().is_none() {
+        return quote!(compile_error!(
+            "for_all_structs!() takes only a single ident as input"
+        ))
+        .into();
+    }
+
+    let doc_dir = get_doc_dir();
+    let structs_glob = format!("{}/doc/pyo3_ffi/struct.*.html", doc_dir.display());
+
+    let mut output = TokenStream::new();
+
+    for entry in glob::glob(&structs_glob).expect("Failed to read glob pattern") {
+        let entry = entry
+            .unwrap()
+            .file_name()
+            .unwrap()
+            .to_string_lossy()
+            .into_owned();
+        let struct_name = entry
+            .strip_prefix("struct.")
+            .unwrap()
+            .strip_suffix(".html")
+            .unwrap();
+        let struct_ident = Ident::new(struct_name, Span::call_site());
+        output.extend(quote!(#macro_name!(#struct_ident);));
+    }
+
+    if output.is_empty() {
+        quote!(compile_error!(concat!(
+            "No files found at `",
+            #structs_glob,
+            "`, try running `cargo doc -p pyo3-ffi` first."
+        )))
+    } else {
+        output
+    }
+    .into()
+}
+
+fn get_doc_dir() -> PathBuf {
+    let path = PathBuf::from(env::var_os("OUT_DIR").unwrap());
+    path.parent()
+        .unwrap()
+        .parent()
+        .unwrap()
+        .parent()
+        .unwrap()
+        .parent()
+        .unwrap()
+        .to_owned()
+}
+
+/// Macro which expands to multiple macro calls, one per field in a pyo3-ffi
+/// struct.
+#[proc_macro]
+pub fn for_all_fields(input: proc_macro::TokenStream) -> proc_macro::TokenStream {
+    let input: TokenStream = input.into();
+    let mut input = input.into_iter();
+
+    let struct_name = match input.next() {
+        Some(TokenTree::Ident(i)) => i,
+        _ => {
+            return quote!(compile_error!(
+                "for_all_fields!() takes exactly two idents as input"
+            ))
+            .into()
+        }
+    };
+
+    match input.next() {
+        Some(TokenTree::Punct(punct)) if punct.as_char() == ',' => (),
+        _ => {
+            return quote!(compile_error!(
+                "for_all_fields!() takes exactly two idents as input"
+            ))
+            .into()
+        }
+    };
+
+    let macro_name = match input.next() {
+        Some(TokenTree::Ident(i)) => i,
+        _ => {
+            return quote!(compile_error!(
+                "for_all_fields!() takes exactly two idents as input"
+            ))
+            .into()
+        }
+    };
+
+    if !input.next().is_none() {
+        return quote!(compile_error!(
+            "for_all_fields!() takes exactly two idents as input"
+        ))
+        .into();
+    }
+
+    let doc_dir = get_doc_dir();
+    let struct_file = fs::read_to_string(format!(
+        "{}/doc/pyo3_ffi/struct.{}.html",
+        doc_dir.display(),
+        struct_name
+    ))
+    .unwrap();
+
+    let html = scraper::Html::parse_document(&struct_file);
+    let selector = scraper::Selector::parse("span.structfield").unwrap();
+
+    let mut output = TokenStream::new();
+
+    for el in html.select(&selector) {
+        let id = el
+            .value()
+            .id()
+            .unwrap()
+            .strip_prefix("structfield.")
+            .unwrap();
+
+        let field_ident = Ident::new(id, Span::call_site());
+
+        output.extend(quote!(#macro_name!(#struct_name, #field_ident);));
+    }
+
+    output.into()
+}

pyo3-ffi-check/src/main.rs

@@ -0,0 +1,78 @@
+use std::process::exit;
+
+fn main() {
+    let mut failed = false;
+
+    macro_rules! check_struct {
+        ($name:ident) => {{
+            let pyo3_ffi_size = std::mem::size_of::<pyo3_ffi::$name>();
+            let bindgen_size = std::mem::size_of::<bindings::$name>();
+
+            let pyo3_ffi_align = std::mem::align_of::<pyo3_ffi::$name>();
+            let bindgen_align = std::mem::align_of::<bindings::$name>();
+
+            // Check if sizes differ, but ignore zero-sized types (probably "opaque" in pyo3-ffi)
+            if pyo3_ffi_size == 0 {
+                println!(
+                    "warning: ignoring zero-sized pyo3_ffi type {}",
+                    stringify!($name),
+                );
+            } else if pyo3_ffi_size != bindgen_size {
+                failed = true;
+                println!(
+                    "error: size of {} differs between pyo3_ffi ({}) and bindgen ({})",
+                    stringify!($name),
+                    pyo3_ffi_size,
+                    bindgen_size
+                );
+            } else if pyo3_ffi_align != bindgen_align {
+                failed = true;
+                println!(
+                    "error: alignment of {} differs between pyo3_ffi ({}) and bindgen ({})",
+                    stringify!($name),
+                    pyo3_ffi_align,
+                    bindgen_align
+                );
+            } else {
+                pyo3_ffi_check_macro::for_all_fields!($name, check_field);
+            }
+        }};
+    }
+
+    macro_rules! check_field {
+        ($struct_name:ident, $field:ident) => {{
+            let pyo3_ffi_offset = memoffset::offset_of!(pyo3_ffi::$struct_name, $field);
+            let bindgen_offset = memoffset::offset_of!(bindings::$struct_name, $field);
+
+            if pyo3_ffi_offset != bindgen_offset {
+                failed = true;
+                println!(
+                    "error: field offset of {}.{} differs between pyo3_ffi ({}) and bindgen ({})",
+                    stringify!($struct_name),
+                    stringify!($field),
+                    pyo3_ffi_offset,
+                    bindgen_offset
+                );
+            }
+        }};
+    }
+
+    pyo3_ffi_check_macro::for_all_structs!(check_struct);
+
+    if failed {
+        exit(1);
+    } else {
+        exit(0);
+    }
+}
+
+#[allow(
+    non_snake_case,
+    non_camel_case_types,
+    non_upper_case_globals,
+    dead_code,
+    improper_ctypes
+)]
+mod bindings {
+    include!(concat!(env!("OUT_DIR"), "/bindings.rs"));
+}

pyo3-ffi-check/wrapper.h

@@ -0,0 +1,3 @@
+#include "Python.h"
+#include "datetime.h"
+#include "frameobject.h"

pytests/Cargo.toml

@@ -4,6 +4,7 @@ name = "pyo3-pytests"
 version = "0.1.0"
 description = "Python-based tests for PyO3"
 edition = "2018"
+publish = false
 
 [dependencies]
 pyo3 = { path = "../", features = ["extension-module"] }