-
-
Notifications
You must be signed in to change notification settings - Fork 717
ENH: Use BUILD_INTERFACE generator expression for Python includes #5757
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
When system include directories are within the source directory (e.g., .pixi/envs/python/include), they should only be added to BUILD_INTERFACE, not INSTALL_INTERFACE. CMake does not allow INTERFACE properties to contain paths prefixed in the source directory. This fix checks each system include directory using file(RELATIVE_PATH) and only adds it to INSTALL_INTERFACE if it is outside the source directory. The implementation: - Normalizes paths with REALPATH to handle symlinks consistently - Handles both Unix (/) and Windows (\) path separators - Checks for empty relative paths (directory equals source directory) - Identifies parent directory references (../) to detect external paths
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.
This does not feel like the right solution. Leaving it up to Brad.
blowekamp
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.
This is the incorrect approach.
|
@copilot apply suggestions from comment #5756 (comment) |
I've applied the correct fix in commit 91973dc. The previous approach of modifying ITKModuleMacros.cmake was indeed incorrect. The fix is now localized to the ITKBridgeNumPy module's CMakeLists.txt, conditionally setting SYSTEM_INCLUDE_DIRS based on whether Python is inside or outside the source directory. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Conditionally set SYSTEM_INCLUDE_DIRS based on Python location. When Python is in source directory (e.g., .pixi/envs), use include_directories() for build-time only instead of SYSTEM_INCLUDE_DIRS which would add to INSTALL_INTERFACE and violate CMake policy. This fixes the CMake error: Target 'ITKBridgeNumPyModule' INTERFACE_INCLUDE_DIRECTORIES property contains path which is prefixed in the source directory. The previous approach of modifying ITKModuleMacros.cmake was incorrect as it changed behavior for all modules. The fix should be localized to the ITKBridgeNumPy module where the issue originates. Handles edge cases: - Empty relative path (directory equals source directory) - Absolute paths on different drives (Windows-specific) - Parent directory references (../)
|
@copilot Use the $<BUILD_INTERFACE: > generator expression in the Numpy module, not checking whether pixi environment is created in the source directory. |
The comment appears to be incomplete. Could you please provide the full suggestion? Are you referring to using a different CMake variable or generator expression? |
|
@copilot Apply this comment: #5756 (comment) by using the $<BUILD_INTERFACE: > generator expression in the Numpy module, not by checking whether pixi environment is created in the source directory. |
Add Python includes directly to ITKBridgeNumPyModule target using $<BUILD_INTERFACE:...> generator expression after itk_module_impl(). This prevents CMake errors about source-prefixed paths in INSTALL_INTERFACE while keeping the includes available during build. Reverts incorrect changes to ITKModuleMacros.cmake that affected all modules. Addresses reviewer request to use $<BUILD_INTERFACE:...> generator expressions.
Applied in commit daebeaa. Now using |
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.
This seems wrong too. I am giving up on trying to get copilot to fix this.
|
Super needed by #5765 |
CMake configuration fails when Python include directories are within the source tree (e.g.,
.pixi/envs/python/include):CMake prohibits source-prefixed paths in INTERFACE properties as they leak build internals to consumers.
Changes
Modified
Modules/Bridge/NumPy/CMakeLists.txtto add Python includes directly to theITKBridgeNumPyModuletarget using$<BUILD_INTERFACE:...>generator expression:itk_module_impl()creates the targettarget_include_directories()with$<BUILD_INTERFACE:${Python3_INCLUDE_DIRS}>This localizes the fix to the ITKBridgeNumPy module where the issue originates, without modifying the general module system in
CMake/ITKModuleMacros.cmake.Implementation:
The
$<BUILD_INTERFACE:...>generator expression is the modern CMake approach for handling build-time only include directories that should not be exported to consumers.PR Checklist
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.