Skip to content

COMP: Backport IO modules publicly depend on ImageIOBase#6068

Closed
hjmjohnson wants to merge 2 commits intoInsightSoftwareConsortium:release-5.4from
hjmjohnson:backport-vtkimageio-depends
Closed

COMP: Backport IO modules publicly depend on ImageIOBase#6068
hjmjohnson wants to merge 2 commits intoInsightSoftwareConsortium:release-5.4from
hjmjohnson:backport-vtkimageio-depends

Conversation

@hjmjohnson
Copy link
Copy Markdown
Member

@hjmjohnson hjmjohnson commented Apr 15, 2026

Move ITKIOImageBase from PRIVATE_DEPENDS to DEPENDS for all 6 IO modules that incorrectly kept it private on release-5.4: VTK, CSV, DCMTK, GE, LSM, and the Philips REC module.

IO modules whose public headers include ITKIOImageBase headers need DEPENDS (public/transitive) so downstream consumers receive the include paths and link targets automatically. With PRIVATE_DEPENDS, external projects that link against these IO modules may fail to find ITKIOImageBase headers.

Scope and verification

Origin: Brad's PR #5850 on main fixed VTK only. This PR extends the same fix to the 5 remaining modules that have the identical issue on both release-5.4 and main.

Modules changed:

Module Change
ITKIOVTK PRIVATE_DEPENDS to DEPENDS (cherry-pick of 12be7405)
ITKIOCSV PRIVATE_DEPENDS to DEPENDS
ITKIODCMTK Moved from PRIVATE_DEPENDS to new DEPENDS section
ITKIOGE Moved from PRIVATE_DEPENDS into existing DEPENDS
ITKIOLSM Moved from PRIVATE_DEPENDS into existing DEPENDS
ITKIOPhilipsREC PRIVATE_DEPENDS to DEPENDS

Local build verification (macOS, CMake 3.26.6, Ninja, Release):

  • Configure: passed, module dependency graph resolves cleanly
  • Build: 5896/5896 targets, 0 errors
  • Tests: 77/77 IO tests passed
  • Generated module configs verified: ITKIOImageBase appears in PUBLIC_DEPENDS and TRANSITIVE_DEPENDS

CMake version compatibility: The itk_module() macro maps DEPENDS/PRIVATE_DEPENDS to target_link_libraries(... LINK_PUBLIC/LINK_PRIVATE ...), available since CMake 2.8.12. ITK release-5.4 minimum is 3.16.3. No version concern.

Change ITKIOImageBase from PRIVATE_DEPENDS to DEPENDS for the
ITKIOVTK module. External projects linking against ITKIOVTK need
the ImageIOBase headers and link targets transitively.

Cherry-pick of 12be740 from main.
@github-actions github-actions bot added type:Compiler Compiler support or related warnings area:IO Issues affecting the IO module labels Apr 15, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 15, 2026

Greptile Summary

This PR cherry-picks commit 12be7405 from main onto release-5.4, changing ITKIOImageBase from PRIVATE_DEPENDS to DEPENDS in ITKIOVTK's module declaration so that downstream/external projects receive the transitive CMake link target automatically.

The fix is correct: itkVTKImageIO.h (a public header) includes itkStreamingImageIOBase.h from ITKIOImageBase, making ITKIOImageBase a genuine public dependency. The change is also consistent with every other comparable IO module (JPEG, PNG, NIFTI, etc.), all of which already list ITKIOImageBase under DEPENDS.

Confidence Score: 5/5

Safe to merge — single-line build-system fix with no logic changes.

The change is minimal (one keyword swap in a CMake module declaration), technically correct (public header transitively requires ITKIOImageBase), consistent with all peer IO modules, and carries no runtime or ABI risk.

No files require special attention.

Important Files Changed

Filename Overview
Modules/IO/VTK/itk-module.cmake Moves ITKIOImageBase from PRIVATE_DEPENDS to DEPENDS — correct because itkVTKImageIO.h publicly includes headers from ITKIOImageBase; consistent with all other comparable IO module declarations.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["External Project\n(find_package ITKIOVTK)"] -->|"DEPENDS (public)"| B["ITKIOVTK"]
    B -->|"DEPENDS (public)"| C["ITKIOImageBase"]
    C -->|"DEPENDS (public)"| D["ITKCommon"]
    B -.->|"Before: PRIVATE_DEPENDS\n(not propagated)"| C
    style B fill:#4a90d9,color:#fff
    style C fill:#27ae60,color:#fff
Loading

Reviews (1): Last reviewed commit: "COMP: VTKImageIO publicly depends on Ima..." | Re-trigger Greptile

@blowekamp
Copy link
Copy Markdown
Member

blowekamp commented Apr 15, 2026

Why was this one change in DEPENDS selected? That were probably a dozen similar changes to other modules that occurred during the implementation of CMake Interfaces. I am suspicious this was just selected by AI due to minimal context windows and no real logic on why it needs to be backported compared to the other similar changes.

CSV, DCMTK, GE, LSM, and PhilipsREC still had ITKIOImageBase under
PRIVATE_DEPENDS. This is incorrect because their public headers include
ITKIOImageBase headers, so downstream projects need the transitive
dependency. The same issue exists on main and has not yet been fixed
there either.
@hjmjohnson hjmjohnson changed the title COMP: Backport VTKImageIO publicly depends on ImageIOBase COMP: Backport IO modules publicly depend on ImageIOBase Apr 15, 2026
@hjmjohnson hjmjohnson marked this pull request as draft April 15, 2026 15:10
@hjmjohnson
Copy link
Copy Markdown
Member Author

@blowekamp Fair question — here's the context:

The original cherry-pick was faithful to your commit 5bf3d24301 on main, which changed only ITKIOVTK. The other IO modules were not changed in that commit.

On closer investigation, 5 additional IO modules on release-5.4 have the same PRIVATE_DEPENDS issue: CSV, DCMTK, GE, LSM, and PhilipsREC. Notably, these same 5 are also still PRIVATE_DEPENDS on main — they were not part of your VTK fix or the subsequent gersemi reformatting.

I've expanded this PR to fix all 6 modules and converted it to draft — the broader scope may be out of bounds for a patch release backport. Happy to:

  1. Keep expanded — fix all 6 on release-5.4 if the maintainers agree it's safe for a patch release
  2. Revert to VTK-only — cherry-pick scope matching your original commit
  3. Close this and instead fix all 6 on main first, then backport

What's your preference?

@blowekamp
Copy link
Copy Markdown
Member

@blowekamp Fair question — here's the context:

The original cherry-pick was faithful to your commit 5bf3d24301 on main, which changed only ITKIOVTK. The other IO modules were not changed in that commit.

On closer investigation, 5 additional IO modules on release-5.4 have the same PRIVATE_DEPENDS issue: CSV, DCMTK, GE, LSM, and PhilipsREC. Notably, these same 5 are also still PRIVATE_DEPENDS on main — they were not part of your VTK fix or the subsequent gersemi reformatting.

I've expanded this PR to fix all 6 modules and converted it to draft — the broader scope may be out of bounds for a patch release backport. Happy to:

  1. Keep expanded — fix all 6 on release-5.4 if the maintainers agree it's safe for a patch release
  2. Revert to VTK-only — cherry-pick scope matching your original commit
  3. Close this and instead fix all 6 on main first, then backport

What's your preference?

I don't know who or what am taking to...

What was the reason of this patch to be included? Was there an issue in release-5.4 or was this original PR just marked as a BUG fix and it was included?

@hjmjohnson
Copy link
Copy Markdown
Member Author

@blowekamp This is Hans. I used Claude Code as a tool to help identify and prepare backport candidates from the main..release-5.4 diff (methodology described in #6051). The tool flagged your commit 12be7405 because it was tagged COMP: and touched only 1 file — meeting the low-risk cherry-pick criteria. I reviewed and approved the selection before opening the PR.

To your second question: there was no specific bug report. The original inclusion logic was:

  1. Scan main commits since 2025-12-01 tagged BUG: or COMP: with <=3 files changed
  2. Your commit 5bf3d24301 (PR COMP: VTKImageIO publicly depends on ImageIOBase #5850) was COMP: + 1 file — fits the "low-risk build fix" tier
  3. I cherry-picked it as part of a batch of backport candidates for 5.4.6

Your concern was valid — if VTK needed fixing, the other modules with the same issue should be addressed too. I've expanded the PR to fix all 6 affected modules (CSV, DCMTK, GE, LSM, the Philips module, and VTK). The same 5 non-VTK modules are also still PRIVATE_DEPENDS on main.

That said, since there's no reported downstream breakage from this on release-5.4, this is a correctness fix rather than a bug fix. If you think it doesn't belong in a patch release, I'm happy to close this and instead open a PR on main to fix the remaining 5 modules there.

@hjmjohnson hjmjohnson self-assigned this Apr 15, 2026
@hjmjohnson
Copy link
Copy Markdown
Member Author

Closing, as it feels like this is out of scope for a backport and should not have been considered. I'll update the related Issue #6051 to reflect the rejection of the backport.

@hjmjohnson hjmjohnson closed this Apr 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:IO Issues affecting the IO module type:Compiler Compiler support or related warnings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants