COMP: Declare NrrdIO's Threads dependency in ITK's exported config - #6776
Conversation
|
I would be good to a have a small isolated project to check a couple things with this change. If the recommended modern CMake linking interface is the issue still there? Use |
|
Also check if there is missing export code for Threads::Threads in the NRRD third party library. |
I think this is the right place to look. I'll need to do some digging and investigating. NOTE: The current patch fixes the downstream problem I have, but is probably to heavy handed in making it an unconditional find_package rather than binding it to the ITKNrrdIO that needs it. WIll run some tests as suggested. |
452cf94 to
a3488c0
Compare
|
Both answered, and the PR is reworked to your second point. Isolated package attached ( 1. Modern interface libraries — the failure is identical. Two consumer projects against the same ITK build tree, one CMake resolves 2. Missing export code — yes, that was the root cause. One implementation note: the vars have to be set in the module's top-level Verified on macOS arm64 against a build tree. The install-tree path mirrors Happy to file a follow-up issue applying the same pattern to the remaining leaked targets ( Isolated package, inlined
# `find_package(ITK)` + `Threads::Threads` export probe
Minimal external project reproducing the generate-time failure reported in
ITK PR #6776: `ITK::ITKNrrdIO`'s exported link interface names
`Threads::Threads`, but `ITKConfig.cmake` never brings that imported target
into the consumer's scope.
## Run
```bash
./run-probe.sh /path/to/ITK-build # or an ITK install's lib/cmake/ITK-6.0Exit status 0 means both consumer styles configure; non-zero means at least What it coversTwo directories, one per documented consumer style:
Both must link a target. A probe that only calls Expected results
The failure is identical in both styles. Failure text: Configuring the ITK under test
cmake -G Ninja -S ITK -B ITK-build \
-DBUILD_TESTING=OFF -DBUILD_EXAMPLES=OFF \
-DITK_BUILD_DEFAULT_MODULES=OFF -DModule_ITKIONRRD=ONThe probe is a configure-time check, so ITK does not have to be built.
#include "itkImageFileReader.h"
#include "itkImage.h"
int
main()
{
using ImageType = itk::Image<float, 3>;
const auto reader = itk::ImageFileReader<ImageType>::New();
return reader.IsNull() ? 1 : 0;
}
# Legacy consumer style: UseITK + ${ITK_LIBRARIES}
cmake_minimum_required(VERSION 3.22)
project(itk_threads_probe_legacy CXX)
find_package(ITK REQUIRED COMPONENTS ITKIONRRD)
include(${ITK_USE_FILE})
add_executable(probe_legacy ../probe.cxx)
target_link_libraries(probe_legacy PRIVATE ${ITK_LIBRARIES})
# Modern consumer style: no UseITK, ${ITK_INTERFACE_LIBRARIES}
# https://docs.itk.org/en/latest/migration_guides/itk_6_migration_guide.html#modern-cmake-interface-libraries
cmake_minimum_required(VERSION 3.22)
project(itk_threads_probe_modern CXX)
find_package(ITK REQUIRED COMPONENTS ITKIONRRD)
add_executable(probe_modern ../probe.cxx)
target_link_libraries(probe_modern PRIVATE ${ITK_INTERFACE_LIBRARIES})
|
dzenanz
left a comment
There was a problem hiding this comment.
Looks good on a glance.
Trying here and maybe upstreaming later? Or would this fix be irrelevant upstream? Even so, having synchronization would be good.
@dzenanz This is ITK's CMake file, and it does not exist in the NrrdIO upstream. |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as resolved.
This comment was marked as resolved.
ITKNrrdIO's exported link interface names Threads::Threads with nothing bringing that imported target into a consumer's scope, so find_package(ITK) failed at generate time for any UNIX consumer linking an ITK target. ITKTBB is the precedent for re-running find_package(Threads) from the module's own export code.
a3488c0 to
a6ddb92
Compare
|
Force-push is commit-message rewrap only; the diff is unchanged from the version @dzenanz approved. Install-tree path is now exercised, which closes the gap both greptile and the previous description flagged as untested.
|
ITK::ITKNrrdIO's exported interface namesThreads::Threads, which nothing resolves for consumers, sofind_package(ITK)fails at generate time for UNIX consumers linking an ITK target.ITKIONRRDis a default module, somainis affected.Root cause
#6695 added to
Modules/ThirdParty/NrrdIO/src/CMakeLists.txt:which reaches
ITKTargets.cmakeas$<LINK_ONLY:Threads::Threads>with no consumer-side declaration.mainonly;release-5.4is unaffected.ITKNrrdIO_EXPORT_CODE_{BUILD,INSTALL}re-runsfind_package(Threads)on the consumer side, asITKTBBdoes for the same dependency. The variables must be set beforeitk_module_impl()in the module's top-levelCMakeLists.txt;add_subdirectory(src)opens a new scope.Local validation
Isolated consumer project (posted in a comment, two directories:
UseITK+ITK_LIBRARIES, andITK_INTERFACE_LIBRARIESwithoutUseITK):mainBoth styles fail identically without the change:
INTERFACE_LINK_LIBRARIESon an imported target resolves at generate time once anything linksITK::ITKNrrdIOtransitively. A consumer that callsfind_package(ITK)without linking a target passes either way, which matters for any regression test added for this.Build tree and install tree, macOS arm64,
-DITK_BUILD_DEFAULT_MODULES=OFF -DModule_ITKIONRRD=ON -DBUILD_TESTING=ON:Audit of other externally-namespaced targets in ITKTargets.cmake
An earlier revision of this description listed
DCMTK::*,GTest::gtestandMPI::MPI_Cas latent leaks of the same class. Each was checked against a real build tree; that was wrong.Threads::Threads(viaITKNrrdIO)DCMTK::*(19)ITKDCMTKsets export code on both pathsGTest::*VTK::*(10, viaITKVtkGlue)MPI::MPI_C, vendored-HDF5Threads::ThreadsAI assistance
pre-commit run --all-filesclean on the pushed HEAD