Skip to content

Fail fast for system SWIG 4.4+ with Python limited API#6544

Open
SID-6921 wants to merge 1 commit into
InsightSoftwareConsortium:mainfrom
SID-6921:fix-system-swig-44-limited-api-6479
Open

Fail fast for system SWIG 4.4+ with Python limited API#6544
SID-6921 wants to merge 1 commit into
InsightSoftwareConsortium:mainfrom
SID-6921:fix-system-swig-44-limited-api-6479

Conversation

@SID-6921

@SID-6921 SID-6921 commented Jul 3, 2026

Copy link
Copy Markdown

Summary

  • add a configure-time guard for the known-broken combination:
    • ITK_USE_SYSTEM_SWIG=ON
    • ITK_WRAP_PYTHON=ON
    • ITK_USE_PYTHON_LIMITED_API=ON
    • SWIG_VERSION >= 4.4.0
  • emit a clear actionable error instead of allowing a build that fails later at Python import time

Why

Issue #6479 documents that this combination can compile/link but then fails at import with missing delete_SwigPyIterator from the generated limited-API wrapper module. This change prevents users from reaching that late failure mode.

User guidance in error

The message recommends one of:

  • use system SWIG 4.3.x
  • set ITK_USE_PYTHON_LIMITED_API=OFF
  • or set ITK_USE_SYSTEM_SWIG=OFF

Testing

  • CMake configure logic change only (no runtime behavior change outside guarded configuration path)
  • patch validated by source inspection and targeted diff.

Copilot AI review requested due to automatic review settings July 3, 2026 05:47
@github-actions github-actions Bot added the type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots label Jul 3, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for contributing a pull request! 🙏

Welcome to the ITK community! 🤗👋☀️

We are glad you are here and appreciate your contribution. Please keep in mind our community participation guidelines. 📜
More support and guidance on the contribution process can be found in our contributing guide. 📖

This is an automatic message. Allow for time for the ITK community to be able to read the pull request and comment
on it.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a CMake configure-time guard to fail fast when configuring ITK Python wrapping with the Python Limited API while using a system SWIG version known to generate wrappers that later fail at import time (SWIG ≥ 4.4.0). This prevents late runtime import errors by providing an early, actionable configuration error.

Changes:

  • Add a FATAL_ERROR configure-time check for ITK_USE_SYSTEM_SWIG=ON + ITK_WRAP_PYTHON=ON + ITK_USE_PYTHON_LIMITED_API=ON when SWIG_VERSION >= 4.4.0.
  • Provide user guidance in the error text with suggested configuration alternatives (use SWIG 4.3.x, disable Limited API, or disable system SWIG).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +193 to +200
# Known-broken combination: system SWIG 4.4.x with Python Limited API emits
# wrappers that fail at import time (missing delete_SwigPyIterator on moduledef).
if(ITK_WRAP_PYTHON AND ITK_USE_PYTHON_LIMITED_API AND SWIG_VERSION VERSION_GREATER_EQUAL 4.4.0)
message(
FATAL_ERROR
"System SWIG ${SWIG_VERSION} with ITK_USE_PYTHON_LIMITED_API=ON is currently unsupported due to import-time wrapper failures. Use system SWIG 4.3.x, set ITK_USE_PYTHON_LIMITED_API=OFF, or set ITK_USE_SYSTEM_SWIG=OFF."
)
endif()
@greptile-apps

greptile-apps Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds a configure-time guard for a known-broken SWIG and Python limited API setup. The main changes are:

  • Rejects system SWIG when Python wrapping and the limited API are enabled with SWIG_VERSION >= 4.4.0.
  • Emits a clear CMake fatal error before the build reaches import-time wrapper failures.
  • Suggests using system SWIG 4.3.x, disabling the Python limited API, or using ITK-managed SWIG.

Confidence Score: 4/5

Safe to merge after the minor comment mismatch is addressed.

The functional CMake guard is isolated to configure-time validation and matches the PR intent. The only finding is non-blocking documentation drift in the new in-source comment.

Wrapping/Generators/SwigInterface/CMakeLists.txt

T-Rex T-Rex Logs

What T-Rex did

  • Ran the base CMake configure to verify the SWIG limited API guard; it found fake SWIG 4.4.0 and completed with exit code 0.
  • Ran the head CMake configure for the SWIG limited API guard; it found fake SWIG 4.4.0 and failed at Wrapping/Generators/SwigInterface/CMakeLists.txt:196, with the documented guidance, and the included negative control completed with exit code 0 when limited API was OFF.

View all artifacts

T-Rex Ran code and verified through T-Rex

Important Files Changed

Filename Overview
Wrapping/Generators/SwigInterface/CMakeLists.txt Adds a configure-time fatal error for system SWIG 4.4+ with Python limited API; only issue found is a comment that says 4.4.x while the guard covers 4.4+.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Configure SwigInterface] --> B{ITK_USE_SYSTEM_SWIG}
B -->|ON| C[find_package SWIG]
C --> D{SWIG_VERSION < minimum}
D -->|yes| E[FATAL_ERROR: newer SWIG required]
D -->|no| F{Python wrapping + limited API + SWIG >= 4.4.0}
F -->|yes| G[FATAL_ERROR: unsupported combination]
F -->|no| H[Continue configure]
B -->|OFF and prebuilt| I[Use prebuilt SWIG path]
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[Configure SwigInterface] --> B{ITK_USE_SYSTEM_SWIG}
B -->|ON| C[find_package SWIG]
C --> D{SWIG_VERSION < minimum}
D -->|yes| E[FATAL_ERROR: newer SWIG required]
D -->|no| F{Python wrapping + limited API + SWIG >= 4.4.0}
F -->|yes| G[FATAL_ERROR: unsupported combination]
F -->|no| H[Continue configure]
B -->|OFF and prebuilt| I[Use prebuilt SWIG path]
Loading

Reviews (1): Last reviewed commit: "Fail fast for system SWIG 4.4+ with Pyth..." | Re-trigger Greptile

Comment on lines +193 to +194
# Known-broken combination: system SWIG 4.4.x with Python Limited API emits
# wrappers that fail at import time (missing delete_SwigPyIterator on moduledef).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Clarify blocked versions
The guard blocks every SWIG_VERSION VERSION_GREATER_EQUAL 4.4.0, but the comment narrows the rationale to 4.4.x. That mismatch can mislead future maintainers into thinking 4.5.0 and later should not be rejected even though the condition intentionally fails fast for them.

Context Used: AGENTS.md (source)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@dzenanz dzenanz requested a review from hjmjohnson July 3, 2026 13:08
@SID-6921 SID-6921 force-pushed the fix-system-swig-44-limited-api-6479 branch from 14df2a3 to 526aa0d Compare July 3, 2026 15:48
@SID-6921

SID-6921 commented Jul 3, 2026

Copy link
Copy Markdown
Author

Applied the wording nit from review: updated the in-file note to say SWIG 4.4+ (matching the VERSION_GREATER_EQUAL 4.4.0 guard) in commit 4b11d44.

@thewtex thewtex left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SID-6921 thanks for the contribution! 🙏

@dzenanz

dzenanz commented Jul 3, 2026

Copy link
Copy Markdown
Member

Preferably squash upon merge.

@dzenanz

dzenanz commented Jul 3, 2026

Copy link
Copy Markdown
Member

Or even better, squash into one commit now.

Signed-off-by: SID <nandasiddhardha@gmail.com>
@SID-6921 SID-6921 force-pushed the fix-system-swig-44-limited-api-6479 branch from 4b11d44 to 3eccfe7 Compare July 3, 2026 19:56
@SID-6921

SID-6921 commented Jul 3, 2026

Copy link
Copy Markdown
Author

Done. I squashed the branch into a single commit as requested and force-pushed: 3eccfe7.

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

Labels

type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants