-
-
Notifications
You must be signed in to change notification settings - Fork 712
COMP: Modernize Python3 module build #5594
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
dzenanz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this Hans!
8c4e211 to
0187c52
Compare
|
It looks like you accidentally deleted |
0187c52 to
2691336
Compare
Yes. This is no longer needed. The Python3_add_module() performs the necessary steps in 3.20 and greater. This correct behavior testing from 2015 predates cmake support. This is now redundant and no longer used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A number of the removed features are not testing in CI, so we need to check:
- ITKPythonPackage builds and passes tests with this branch (especially on Windows)
- @SimonRit or @LucasGandel verify that this does not break their modules, e.g. the ITKCudaCommon, that may reference external shared libraries, especially on Windows
ef993ac to
acdb6a2
Compare
|
@thewtex Thanks for pointing me in the direction of the most difficult builds. I have a windows VM that I am trying to setup to do these tests. If you have any other pointers to documentation of how to setup and test windows python I would really appreciate it. |
|
@hjmjohnson great, thanks for testing. More on Windows builds is here: And for remote modules: |
|
Finding a windows computer that can build ITK with the environments that need testing is more challenging than expected. I'm working on it. |
acdb6a2 to
c385b1b
Compare
c385b1b to
318642e
Compare
0fcc3a2 to
15925b4
Compare
Sorry, I missed your message. I tested the full compilation on Ubuntu of the wrappings with the main branches of CudaCommon and RTK. It seems to work. I'll try to do the same on Windows. Anything else I should try? |
|
Thanks, Simon. I am working on building packages using the ITKPythonPackage system as well. I appreciate your assistance with checking. FYI: During setting up a Windows system, I was sidetracked with other Windows build issues that I have been trying to address. Hans |
15925b4 to
5306de0
Compare
5306de0 to
e978470
Compare
e978470 to
924feef
Compare
924feef to
0fbcb01
Compare
Visual Studio 2022 warns of ignored flags. In Visual Studio 2012 and later versions, SSE2 is enabled by default. https://learn.microsoft.com/en-us/cpp/build/reference/arch-x86?view=msvc-170 Originally prompted by 1000's of warnings: cl : Command line warning D9025 : overriding '/arch:SSE' with '/arch:SSE2' For all modern Linux packaging workflows (wheels, C++ libs, distros): x86_64: do NOT specify -msse2 — wasteful and redundant. Portability is retained, the x86-64 baseline includes SSE2
Corrected the Python wrapping `foreach` loop logic to properly append file paths to `ITK_WRAP_PYTHON_FILES` and improve code consistency.
Python3.11 + defines c++ compiles with extern "C", so it does not need to be re-stated as extern "C".
Removed unnecessary `link_directories` usage for MSVC in Python wrapping logic as it relies on outdated CMake features and is no longer required. Simplified the code for maintainability.
Deleted redundant ITKCommon_SYSTEM_INCLUDE_DIRS and ITKCommon_SYSTEM_LIBRARY_DIRS setup for Python when ITK_WRAP_PYTHON is enabled. These variable are no longer used anywhere in ITK: ITKCommon_SYSTEM_INCLUDE_DIRS ITKCommon_SYSTEM_LIBRARY_DIRS
Deleted the unused `itkTargetLinkLibrariesWithDynamicLookup.cmake` module to simplify codebase and improve maintainability. This module is no longer relevant or referenced in the current build system. It was previously needed for building python on Mac, but the functionality is now included in python3_add_library() from find_package(Python3 ...)
- Relocate Python LIMITED_API logic for modularity
o Python configuration should be done in ITKSetPytnon3Vars.cmake.
It was previously spread around in different parts of the ITK
package which made tracking of behaviors difficult to monitor.
- Simplify logic for ITK_USE_PYTHON_LIMITED_API
o Avoid using a temporary variable use_python_limited_api_default to
set the value of ITK_USE_PYTHON_LIMITED_API.
o Prefer to set ITK_USE_PYTHON_LIMITED_API directly.
o Moved Python LIMITED_API and package component logic to
`ITKSetPython3Vars.cmake` for improved organization and maintainability.
- Ensure single call to find_package( Python3 ) is used to simplify
the logic and testing. Manually identify required Python3 COMPONENTS
and give more descriptive error messages to help with tracking down
the failing condition. This is particularly helpful when multiple
versions of Python are installed (i.e. python3.13 from homebrew,
python3.13 from pixi, python3.13 in standard OS install, etc).
- Refactored Python LIMITED_API setup for improved modularity and reusability.
- Adjusted `itk_end_wrap_module.cmake` to properly handle LIMITED_API settings.
- Updated `ITKSetPython3Vars.cmake` to streamline Python version selection and configuration.
- Improved maintainability by centralizing Python-related configuration and ensuring consistency across CMake files.
- Fix linking of ITKCommon when including python support
- Added a check in `ITKSetPython3Vars.cmake` to ensure CMake version is >= 3.26 when `ITK_USE_PYTHON_LIMITED_API` is enabled.
- Provides clear error messaging to guide users to update CMake or disable the limited API configuration.
- Updated `ITKSetPython3Vars.cmake` to append `Python3_FIND_ABI` values only for Linux (non-Apple, non-MSVC) systems.
- Unset `Python3_FIND_ABI` for non-Linux platforms to prevent configuration issues.
- Updated `ITKSetPython3Vars.cmake` to restrict `PYTHON_VERSION_MIN` and `PYTHON_VERSION_MAX` to the version of the specified `Python3_EXECUTABLE`.
- Ensures compatibility and proper handling of Python LIMITED_API in wrapping process.
Set include directories for Python wrapping from Python3::Module target. Changed Python header inclusion to use angle brackets for consistency. Added include directory properties for `itkPyCommand.cxx` to improve Python wrapping configuration. Add Python linking in ITKCommon when ITK_WRAP_PYTHON - Updated `ITKCommon` to link against appropriate Python libraries when Python wrapping is enabled. - Differentiated linking logic for LIMITED_API and non-LIMITED_API configurations. Update Python linking and remove unused development component Ensure `ITKCommon` links to `Python3::Module` instead of `Python3::Python`. Adjust Python3 component configuration to exclude unused `Development`, addressing limitations in dockcross build environments.
0fbcb01 to
4784176
Compare
|
@thewtex This builds with ITKPythonPackage (after changing ITKPythonPackage to allow for building an untagged version of ITK). It has taken me a long time to figure out how to get ITKPythonPackage to not use the itk6.0b01 tag and use my git tag for testing, but once I got ITKPythonPackage to compile my branch, all worked without a problem. |
dzenanz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not tried using this. Code mostly looks good.
|
OK I am now trying to build this locally. |
|
Building locally was successful, though I only tried a few configurations. |
Relocate Python LIMITED_API logic for modularity
o Python configuration should be done in ITKSetPytnon3Vars.cmake.
It was previously spread around in different parts of the ITK
package which made tracking of behaviors difficult to monitor.
Simplify logic for ITK_USE_PYTHON_LIMITED_API
o Avoid using a temporary variable use_python_limited_api_default to
set the value of ITK_USE_PYTHON_LIMITED_API.
o Prefer to set ITK_USE_PYTHON_LIMITED_API directly.
0 Moved Python LIMITED_API and package component logic to
ITKSetPython3Vars.cmakefor improved organization and maintainability.Ensure subsequent find Python3 use same version
o When calling find_package(Python3 multiple times
ensure that the same PYTHON_VERSION is used
for each call.
Changed Python header inclusion to use angle brackets for consistency.
o Added include directory properties for
itkPyCommand.cxxto improveo Python wrapping configuration.
Remove unused cmake variables
o Deleted redundant ITKCommon_SYSTEM_INCLUDE_DIRS and
o ITKCommon_SYSTEM_LIBRARY_DIRS setup for Python when ITK_WRAP_PYTHON is
enabled.
These variable are no longer used anywhere in ITK:
o ITKCommon_SYSTEM_INCLUDE_DIRS
o ITKCommon_SYSTEM_LIBRARY_DIRS
Remove redundant MSVC Python linking logic
o Removed unnecessary
link_directoriesusage for MSVC in Python wrappinglogic as it relies on outdated CMake features and is no longer required.
Simplified the code for maintainability.
PR Checklist