Fix binding generation bugs, add FreeBSD support, resolve CMake MPI issue#62
Open
elliejs wants to merge 2 commits into
Open
Fix binding generation bugs, add FreeBSD support, resolve CMake MPI issue#62elliejs wants to merge 2 commits into
elliejs wants to merge 2 commits into
Conversation
Several bugs in the binding generator caused incorrect or missing C++
code in the generated pybind11 bindings. These affect all platforms.
header.py — type resolution (_underlying_type):
Nested types defined inside template classes (typedefs like
value_type, enums like BehaviorOnTransform, POD types like
size_type) were emitted as bare names without their enclosing class
qualification. In generated static_cast expressions, the compiler
cannot resolve a bare "value_type" — it needs the fully qualified
"NCollection_Array1<T>::value_type". Three new blocks handle this:
non-POD typedefs (134 types), POD typedefs (3 types), and nested
enums/records (17 types). The non-POD typedef block also fixes a
corruption pattern where the upstream replace() call could produce
"X<T>::typename X<T>::Y", and prevents double typename prefixes.
header.py — default values (_default_value):
Fix a typo in the ArgInfo dataclass field name: "arg_defualt" was
never matching what Jinja2 templates reference as "arg_default".
This silently dropped every default parameter value for every
function on every platform. Also strip "const" from brace-init
defaults — "const gp_Pnt{ }" is a syntax error (8 parameters).
macros.j2 — default value emission:
The argnames macro had {% if d %} where d is undefined in Jinja2
(always falsy), so default values were never emitted even if
header.py produced them. Fixed to {% if arg.arg_default %}.
New default_value macro handles null pointer defaults correctly:
pybind11 expects py::none() for optional pointer arguments, not
static_cast<T*>(0) which was previously emitted (40 parameters).
Fix arg.name → arg.arg_name in init_outputs_byref — the ArgInfo
dataclass field is arg_name. The undefined arg.name produced empty
variable names in generated C++ for by-reference smart pointer
methods.
macros.j2 — template type qualification:
arg_type_with_template_params had a bug where it checked the raw
input string t instead of ns.t, and only handled types starting
with the bare class name (ClassName::member). Types where the class
name already had template params (ClassName<T>::Iterator) were not
qualified with typename, causing compile failures. Added a second
branch for this case. Same fix applied to template_return_type.
New argtypes_for_template macro applies template type qualification
to py::init<> type lists. Without this, template class constructor
argument types were emitted without typename qualification, causing
dependent name lookup failures.
Removed dead inline type-expansion code in argnames_template that
computed a namespace variable ns.t which was never used — the
actual default emission already called arg_type_with_template_params.
macros.j2 — trampoline class deduplication:
Pure virtual method deduplication compared by short name (m.name),
so overloaded virtuals like Values(Vec&, Real&, Vec&) and
Values(Vec&, Real&, Vec&, Mat&) were treated as duplicates. Changed
to use m.full_name (full signature). Also added the missing
methods.append call for private virtuals, which allowed parent-class
loops to re-emit methods already overridden. 20 classes affected.
CMakeLists.j2:
Bump cmake_minimum_required to 3.17. Enable C language (needed when
VTK links against MPI::MPI_C). Add find_package(MPI QUIET) so cmake
can resolve MPI::MPI_C and MPI::MPI_CXX targets on platforms where
VTK is built with MPI support (resolves CadQuery/OCP#179). Add
find_package(fmt) and link fmt::fmt — FreeBSD's OCCT port links
against libfmt externally, while conda builds bundle it statically
so this is a no-op there. Add status messages for Python lib path
and VTK version to aid CI debugging.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
FreeBSD's system OCCT (from ports) installs headers and libraries in different locations than conda, and libclang picks up conflicting system headers unless explicitly told not to. This commit adds the platform-specific handling needed to generate OCP bindings on FreeBSD. translation_unit.py: On FreeBSD, libclang's automatic header search adds system include paths that conflict with the OCCT headers from the ports tree, causing hundreds of type redefinition errors during parsing. To fix this, the FreeBSD codepath disables libclang's automatic search (-nostdinc, -nostdinc++) and re-adds the platform include paths from ocp.toml's [FreeBSD] section as -isystem flags. Duplicate -I flags pointing to those same dirs are stripped to keep the arg list clean. Without this, the generate phase fails entirely on FreeBSD. Also fixes an f-string line break that is valid in Python 3.12+ but a syntax error in 3.11 (which FreeBSD uses). symbols_mangled_freebsd.dat: Symbol manifest extracted from FreeBSD's compiled OCCT shared libraries via dump_symbols.py. Each platform needs its own manifest because name mangling and exported symbol sets differ across compilers. 130,310 symbols from FreeBSD's opencascade port. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
4 tasks
Author
|
Linux fails due to something already found in upstream, so not related to PR This PR has been tested in github actions on my own fork here: https://github.com/elliejs/OCP/actions/workflows/bindings.yml Requires sister PR commit CadQuery/OCP#204 however. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two commits:
Fix binding generation bugs and cross-platform CMake issues — fixes several
bugs that affect all platforms:
arg_defualttypo that silently dropped all default parameter values{% if d %}dead conditional that prevented defaults from ever being emittedarg.name→arg.arg_nameproducing empty variable names in by-ref methodsemitted as bare names instead of fully qualified (
NCollection_Array1<T>::value_type)default_valuemacro: emitpy::none()for null pointer defaults instead ofinvalid
static_cast<T*>(0)arg_type_with_template_paramsfor already-parameterized typesargtypes_for_templateforpy::init<>dependent-type qualificationfind_package(MPI QUIET)(resolvesfix CMake not finding MPI OCP#179), add
fmt::fmtlink, bump cmake minimum to 3.17Add FreeBSD platform support — FreeBSD-specific libclang header handling
(
-nostdinc/-isystem) and symbol manifest (130k symbols from FreeBSD'sopencascade port).
Test plan
import OCPsucceedsCompanion OCP PR: CadQuery/OCP#204
🤖 Generated with Claude Code