Skip to content

COMP: Emit GoogleTest export code for install-tree consumers - #6777

Closed
hjmjohnson wants to merge 1 commit into
InsightSoftwareConsortium:mainfrom
hjmjohnson:comp-googletest-export-code-install
Closed

COMP: Emit GoogleTest export code for install-tree consumers#6777
hjmjohnson wants to merge 1 commit into
InsightSoftwareConsortium:mainfrom
hjmjohnson:comp-googletest-export-code-install

Conversation

@hjmjohnson

@hjmjohnson hjmjohnson commented Aug 21, 2026

Copy link
Copy Markdown
Member

Follow-on to #6776, same class of defect: a module whose consumer-side dependency is not declared in what ITK exports. ITKGoogleTest set only ITKGoogleTest_EXPORT_CODE_BUILD, so the module file installed with ITK carried no GTest resolution, and a project built against an installed ITK configured with ITK_USE_SYSTEM_GOOGLETEST=ON failed at CreateGoogleTestDriver with GTest::gtest ... but the target was not found.

It is the only asymmetric module: on main, seventeen modules declare export code and the other sixteen set both _BUILD and _INSTALL.

Red/green, verified at configure time

-DITK_BUILD_DEFAULT_MODULES=ON -DBUILD_TESTING=ON -DITK_USE_SYSTEM_GOOGLETEST=ON, comparing the generated install-tree module file Modules/ThirdParty/GoogleTest/CMakeFiles/ITKGoogleTest.cmake:

tail of the install-tree ITKGoogleTest.cmake
before (nothing after the set(ITKGoogleTest_FACTORY_NAMES "") block)
after set(GTEST_ROOT "") then find_package(GTest REQUIRED)

The build-tree variant already carried the equivalent block and is unchanged. Both variants are written at configure time, so this needs no build or install to check.

Audit of the other targets named in #6776 — no change needed

#6776's description claimed DCMTK::*, GTest::gtest and MPI::MPI_C were latent leaks of the same kind. Each was checked against real build trees; that claim was wrong, and #6776's body has been corrected.

target finding
DCMTK::* (19) Already handled. ITKDCMTK sets export code on both the system and FetchContent paths. An external project linking ITK::ITKIODCMTK against a Module_ITKIODCMTK=ON build configures cleanly.
GTest::gtest, GTest::gtest_main Self-contained on the vendored path — ITK exports them as real imported targets in ITKTargets.cmake. The install-tree gap this PR fixes is on the system-GTest path, where they are absent from the export instead.
VTK::* (10, via ITKVtkGlue) Already handled, both variants.
MPI::MPI_C, and Threads::Threads from the vendored HDF5 Unreachable. Both sit inside $<$<BOOL:${HDF5_ENABLE_PARALLEL}>:...>-style genexes, and vendored HDF5 refuses to configure with HDF5_ENABLE_PARALLEL=ON ("Parallel and C++ options are mutually exclusive"), which ITK requires. HDF5_ENABLE_THREADS is referenced by the vendored snapshot but never defined.
Unrelated defect found while testing, not fixed here

-DITK_BUILD_DEFAULT_MODULES=OFF -DModule_ITKIONRRD=ON -DBUILD_TESTING=ON fails to configure: ITKGoogleTest is not enabled, but Modules/IO/NRRD/test/CMakeLists.txt still calls CreateGoogleTestDriver, so GTest::gtest is undefined. That is a module-enablement problem rather than an export problem, and can be filed separately.

ITKGoogleTest set only ITKGoogleTest_EXPORT_CODE_BUILD, so the module file
installed with ITK carried no GTest resolution. A project built against an
installed ITK configured with ITK_USE_SYSTEM_GOOGLETEST=ON failed at
CreateGoogleTestDriver with GTest::gtest not found.

Set ITKGoogleTest_EXPORT_CODE_INSTALL to match, as the other seventeen
modules that declare export code do.
@github-actions github-actions Bot added type:Compiler Compiler support or related warnings type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots area:ThirdParty Issues affecting the ThirdParty module labels Aug 21, 2026
@hjmjohnson
hjmjohnson marked this pull request as ready for review August 21, 2026 17:23
@hjmjohnson
hjmjohnson requested a review from blowekamp August 21, 2026 17:23
@greptile-apps

greptile-apps Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This change adds install-tree GTest package discovery for one system-GTest configuration. When GTEST_ROOT points to a valid GoogleTest source directory, the install export is still omitted, so an installed ITK package cannot provide the GTest:: targets required by downstream projects using CreateGoogleTestDriver. A producer/install/consumer CMake reproduction confirmed the configuration failure and confirmed that emitting the export for this branch resolves it.

Confidence Score: 4/5

Not safe to merge until installed ITK consumers using a system GoogleTest source root can restore the required GTest targets.

The failure was reproduced with an installed producer and an independent CMake consumer. The same consumer configured, built, and ran when the install export was emitted outside the source-root conditional.

Files Needing Attention: Modules/ThirdParty/GoogleTest/CMakeLists.txt

T-Rex T-Rex Logs

What T-Rex did

  • T-Rex reproduced the P1 finding by using a CMake install-tree reproduction harness, coordinated a consumer configure flow, observed an initial failure with a valid GTest source root, and achieved a successful configure and build after relocating the test-only export.
  • T-Rex posted a second P1 finding proof and prepared corresponding review context.
  • T-Rex performed contract validation around GoogleTest integration, identified the affected CMakeLists.txt lines and the nested conditional behavior, confirmed no repository changes were needed, and noted that CMake 3.31.10 was installed locally to run the test.

View all artifacts

T-Rex Ran code and verified through T-Rex

Comments Outside Diff (1)

  1. General comment

    P1 Install-tree GoogleTest export is omitted for a valid system source root

    • Bug
      • An ITK install configured with ITK_USE_SYSTEM_GOOGLETEST=ON and GTEST_ROOT pointing to a source tree containing CMakeLists.txt exports no find_package(GTest) call. A downstream consumer invoking CreateGoogleTestDriver therefore fails to configure because GTest::gtest is absent.
    • Cause
      • ITKGoogleTest_EXPORT_CODE_INSTALL is assigned only inside if(NOT DEFINED GTEST_ROOT OR NOT EXISTS "${GTEST_ROOT}/CMakeLists.txt"); a valid source tree makes that condition false, even though the installed module still needs restored imported GTest targets.
    • Fix
      • Emit the install export code for all ITK_USE_SYSTEM_GOOGLETEST configurations (outside the source-root/no-source conditional), so install-tree consumers call find_package(GTest REQUIRED) before using CreateGoogleTestDriver.

    T-Rex Ran code and verified through T-Rex

Reviews (1): Last reviewed commit: "COMP: Emit GoogleTest export code for in..." | Re-trigger Greptile

Comment on lines +16 to 22
set(
ITKGoogleTest_EXPORT_CODE_INSTALL
"
set(GTEST_ROOT \"${GTEST_ROOT}\")
find_package(GTest REQUIRED)
"
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Install-tree GoogleTest export is omitted for a valid system source root

When ITK_USE_SYSTEM_GOOGLETEST=ON and GTEST_ROOT points to a GoogleTest source directory containing CMakeLists.txt, the condition at line 5 is false and this install export code is never emitted. The installed module then does not restore GTest::gtest and GTest::gtest_main; consequently, downstream projects that use CreateGoogleTestDriver fail to configure because those targets are missing. Emit the install-side find_package(GTest REQUIRED) export for both system-GTest branches.

Artifacts

CMake install-tree reproduction harness source

  • The executable shell harness creates a valid fake GTest source root and package, installs a producer, and configures/builds an independent consumer; it is the exact source used for both captured runs.

Consumer configure failure with valid GTest source root

  • The repository's exact module file was installed and consumed with `GTEST_ROOT` containing `CMakeLists.txt`; CMake failed because `GTest::gtest` was not found, confirming the broken install export path.

Consumer configure and build success after test-only export relocation

  • A temporary test-only variant emits the install export outside the source-root condition; the consumer found GTest, built `consumer_driver`, and exited successfully, demonstrating the repair.

View artifacts

T-Rex Ran code and verified through T-Rex

@blowekamp

Copy link
Copy Markdown
Member

The initial implementation of the google test third party library was that is should not be installed and only available in the build tree.

@hjmjohnson

Copy link
Copy Markdown
Member Author

Closing: rejected by the maintainer. Not vetted beyond a configure-time check.

@hjmjohnson hjmjohnson closed this Aug 21, 2026
@hjmjohnson

Copy link
Copy Markdown
Member Author

Understood — GoogleTest is deliberately build-tree-only, so the missing install-tree export is the design, not a gap. Correcting my closing note above: that, not the verification depth, is why this is wrong.

@hjmjohnson
hjmjohnson deleted the comp-googletest-export-code-install branch August 21, 2026 18:39
hjmjohnson added a commit to hjmjohnson/itk_forest_build_testbed that referenced this pull request Aug 22, 2026
Phase 3 recognised exactly one reviewer, greptile-apps[bot]. Any other
review bot fell through is_bot() into "bot_other", a bucket the skill
documents as "non-blocking, skip unless explicitly asked". On
Project-MONAI/MONAI#9065 that put a genuine actionable CodeRabbit
finding in the ignore pile; it was only acted on because the raw JSON
was read by hand.

The single GREPTILE_LOGIN constant becomes AI_REVIEW_PROVIDERS, keyed by
bot login and carrying what differs per provider: how a review is
requested, how one is forced for an already-reviewed head, and which
in-repo file indicates the provider is configured. Findings are parsed
per provider and normalised to P1/P2/P3, so CodeRabbit's
Critical/Major/Minor maps onto the vocabulary the phase logic already
speaks and one rule covers both.

Two bugs surfaced while testing this against real PRs.

The greptile parser never matched inline findings. Its pattern was

  alt="(P[123])"[^>]*>\s*\*\*([^*]+)\*\*

but the badge is an <img> wrapped in an <a>, so a closing </a> sits
between the badge and the bold title and \s* cannot span it. Most
findings are inline, so Phase 3 has been running "address every P1/P2"
against an empty list. InsightSoftwareConsortium/ITK#6777 reports 0
findings before this change and 2 P1s after.

Provider detection read config files relative to the working directory,
so triaging owner/repo#N from an unrelated checkout reported whatever
that checkout happened to contain. It now queries the target repo.

Unrecognised bots go to a new "bot_unknown" bucket rather than
"bot_other". The two are documented differently on purpose: bot_other is
ignorable, bot_unknown means nobody has classified this bot yet and it
must be read before the phase can be called clean. That is the failure
mode above, closed for the next review bot as well as this one.

phase_3_ai_review also carries CodeRabbit's PR-level signals, merge_risk
and failed_pre_merge_checks, which have no greptile equivalent and no
inline comment to hang off. On Project-MONAI/MONAI#9067 merge_risk was
"High" with zero inline findings — a credential-exposure issue in a
workflow that would otherwise have been reported as Phase 3 clean.

phase_3_greptile is retained as an alias so callers written against the
old report keep working. ghtp_reply.py is untouched: replying and
resolving are provider-agnostic.

Verified against ITK#6714 and ITK#6777 (greptile) and MONAI#9065 and
MONAI#9067 (coderabbit); test_ghtp_workstate.py still passes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:ThirdParty Issues affecting the ThirdParty module type:Compiler Compiler support or related warnings type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants