Skip to content

Use normal cmake variables for output directories #5691

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.

Sign up for GitHub

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

Jump to bottom
Merged
hjmjohnson merged 2 commits into from
Dec 29, 2025
Merged

Use normal cmake variables for output directories #5691

hjmjohnson merged 2 commits into from
Dec 29, 2025
+22 −133

Conversation

@blowekamp
Copy link
Member

@blowekamp blowekamp commented Dec 15, 2025

PR Checklist

  • No API changes were made (or the changes have been approved)
  • No major design changes were made (or the changes have been approved)
  • Added test (or behavior not changed)
  • Updated API documentation (or API not changed)
  • Added license to new files (if any)
  • Added Python wrapping to new files (if any) as described in ITK Software Guide Section 9.5
  • Added ITK examples for all new major features (if any)

Refer to the ITK Software Guide for
further development details if necessary.

@github-actions github-actions bot added the type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots label Dec 15, 2025
dzenanz
dzenanz reviewed Dec 15, 2025
View reviewed changes
Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

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

LGTM, but Hans and/or Matt should ideally review.

@blowekamp blowekamp force-pushed the no_cache_output_directory branch 2 times, most recently from a144b3a to a447bbe Compare December 16, 2025 13:39
thewtex
thewtex reviewed Dec 16, 2025
View reviewed changes
blowekamp
blowekamp commented Dec 16, 2025
View reviewed changes
@blowekamp blowekamp force-pushed the no_cache_output_directory branch from a447bbe to a0f17a3 Compare December 16, 2025 18:07
@blowekamp
Copy link
Member Author

blowekamp commented Dec 16, 2025

@thewtex I removed the code consolidation and only have the variable type change. Please let me know if this PR is now more amendable.

@blowekamp blowekamp marked this pull request as ready for review December 17, 2025 15:33
@blowekamp
Copy link
Member Author

blowekamp commented Dec 17, 2025

@thewtex The current behavior in main when doing a clean build with wrapping ITK_WRAP_PYTHON enable. The ITK unit test executable are being placed Wrapping/Generators/Python/itk/

Regarding:

1.) was not the case before. Should be checked:

Build without wrapping. List file locations.
Enable wrapping. Build.
Disable wrapping. Build and List file locations.
The file locations in 1.) and 3.) are the same.

What files need to be checked? libraries and unit test executables?

@thewtex
Copy link
Member

thewtex commented Dec 17, 2025

What files need to be checked? libraries and unit test executables?

Yes, the libraries and executables.

@hjmjohnson should also review

@blowekamp
Copy link
Member Author

blowekamp commented Dec 17, 2025

@thewtex What about current behavior ( before my change) of the ITK Unit tests being placed in Wrapping/Generators/Python/itk/? The does not seem right to start.

@thewtex
Copy link
Member

thewtex commented Dec 17, 2025

@blowekamp if it is possible to pull unrelated unit tests without more complexity or breaking things, yes, that would be nice.

@blowekamp blowekamp force-pushed the no_cache_output_directory branch from a0f17a3 to c960531 Compare December 18, 2025 16:18
@blowekamp
Copy link
Member Author

blowekamp commented Dec 18, 2025

@blowekamp if it is possible to pull unrelated unit tests without more complexity or breaking things, yes, that would be nice.

That is not controllable by these changes as both libraries and executables are controlled but CMAKE_RUNTIME_OUTPUT_DIRECTORY.

I made more aggressive changes to remove the NO_WRAP cache variables as well. These are not needed with the described desired behavior and simplified logic. It can be summarized as:

If wrapping is enabled always set the cmake output variables. Other wise if the cmake output variable is not set ( by user provided cache variable ) then set the default.

I went through numerous configurations and reconfigurations comparing the location where libraries and executables. Including the sequence described. The build outputs were consistent.

There were some inconsistencies in some temporary CMake files, addressed here: #5700

Additionally, GDCM archive output path is always the runtime directory, so GDCM static libraries are always in a different place. This is related to old code for executable_output_path and library_output_path. But that is another issue.

@blowekamp
Copy link
Member Author

blowekamp commented Dec 18, 2025

/azp run ITK.Linux

1 similar comment
@blowekamp
Copy link
Member Author

blowekamp commented Dec 21, 2025

/azp run ITK.Linux

@blowekamp blowekamp requested review from dzenanz and thewtex December 22, 2025 13:34
@blowekamp
Copy link
Member Author

blowekamp commented Dec 22, 2025

@thewtex @hjmjohnson This PR should be ready to go.

Please note, that switching to ITK before this PR and then after will have the old OUTPUT_DIRECTORY CMake variables in the cache. After building before this change with wrapping, then building after this change the OUTPUT_DIRECTORY CMake variable may need to be removed from the cache, or a clean configuration done when switching back to building without wrapping.

Here is the PR I have for SimpleITK to make use of these changed with using ITK with FetchContent.
SimpleITK/SimpleITK#2475

Having the ITK and the SimpleITK targets tightly integrated is a very nice feature when making changes in ITK source code and only rebuilding the ITK needed by the requested SimpleITK targets.

dzenanz
dzenanz reviewed Dec 22, 2025
View reviewed changes
Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

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

Looks good on a glance. Deferring to Hans and Matt.

CMake/ITKModuleExternal.cmake Outdated
# When Wrapping is enabled the CMAKE_*_OUTPUT_DIRECTORY(s) will be explicitly set
include(WrappingConfigCommon)

# Only set default is not prior configured by wrapping or the user
Copy link
Member

@dzenanz dzenanz Dec 22, 2025

Choose a reason for hiding this comment

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

is -> if?

@blowekamp
COMP: Set CMake output directories as normal variables
cfca436 
Normal CMake variable take priority over cache variables.

The implemented logic is if ITK_WRAP is enabled then always set the
output directories to known paths.

Otherwise, if not user provided cache variable exists set the normal
cmake variable to the default. This ignores user settings when
wrapping is enabled, but restores then with wrapping is disabled.

Remove cache variable setting of the following:
 - CMAKE_RUNTIME_OUTPUT_DIRECTORY
 - CMAKE_LIBRARY_OUTPUT_DIRECTORY
 - CMAKE_ARCHIVE_OUTPUT_DIRECTORY
 - NO_WRAP_CMAKE_RUNTIME_OUTPUT_DIRECTORY
 - NO_WRAP_CMAKE_LIBRARY_OUTPUT_DIRECTORY

Not force setting cmake output directories enables better usage with
ITK as a FetchContent library.
@blowekamp
ENH: Remove legacy and unused Wrapping EXECUTE_OUTPUT_PATH
ffaa0c9 
CMake document this as "Old executable location variable". And the
RUNTIME_OUTPUT_DIRECTORY target property supersedes this variable.

Additionally the CXX_TEST_PATH variable was not used.
@blowekamp blowekamp force-pushed the no_cache_output_directory branch from c960531 to ffaa0c9 Compare December 22, 2025 18:51
@blowekamp
Copy link
Member Author

blowekamp commented Dec 23, 2025

@thewtex @hjmjohnson This PR is ready to be reviewed and merged. Thank you for your time.

thewtex
thewtex approved these changes Dec 23, 2025
View reviewed changes
Copy link
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

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

LGTM but @hjmjohnson should review

@hjmjohnson hjmjohnson merged commit 160dbfd into InsightSoftwareConsortium:main Dec 29, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dzenanz dzenanz dzenanz left review comments

@thewtex thewtex thewtex approved these changes

@hjmjohnson hjmjohnson hjmjohnson approved these changes

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@blowekamp @thewtex @hjmjohnson @dzenanz