Skip to content

[gtest] fix cppinterop testing with builtin_gtest#22469

Open
ferdymercury wants to merge 10 commits into
root-project:masterfrom
ferdymercury:patch-22
Open

[gtest] fix cppinterop testing with builtin_gtest#22469
ferdymercury wants to merge 10 commits into
root-project:masterfrom
ferdymercury:patch-22

Conversation

@ferdymercury
Copy link
Copy Markdown
Collaborator

@ferdymercury ferdymercury commented Jun 3, 2026

@ferdymercury ferdymercury requested a review from dpiparo June 3, 2026 10:15
@linev
Copy link
Copy Markdown
Member

linev commented Jun 3, 2026

Seems to be I got a glitch with incremental builds.

Now when I rebuild everything from scratch - cmake is happy.
Sorry for the noise

@linev linev closed this Jun 3, 2026
@ferdymercury ferdymercury deleted the patch-22 branch June 3, 2026 10:31
@ferdymercury ferdymercury restored the patch-22 branch June 3, 2026 11:21
@ferdymercury ferdymercury reopened this Jun 3, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 3, 2026

Test Results

    21 files      21 suites   3d 8h 26m 7s ⏱️
 3 856 tests  3 856 ✅ 0 💤 0 ❌
73 320 runs  73 320 ✅ 0 💤 0 ❌

Results for commit 9db911d.

♻️ This comment has been updated with latest results.

@ferdymercury ferdymercury requested a review from hageboeck June 3, 2026 11:24
@ferdymercury ferdymercury marked this pull request as ready for review June 3, 2026 11:24
@ferdymercury ferdymercury requested a review from bellenot as a code owner June 3, 2026 11:24
@ferdymercury ferdymercury changed the title [gtest] fix cppinterop testing [gtest] fix cppinterop testing with builtin_gtest Jun 3, 2026
@linev
Copy link
Copy Markdown
Member

linev commented Jun 3, 2026

Probably one need to do this like in ROOT_FIND_REQUIRED_DEP macro.
There error message is generated like:

   message(SEND_ERROR "The required package ${PACKAGE_NAME} was not found. "
   "Please install it in the system (preferred), set the corresponding CMake search variable, "
   "or opt in to downloading it using '-D${BUILTIN_CONFIG_OPTION}=ON'.")

Just let provide same kind of message for enabling builtin_gtest.

One just need to avoid that ffind_package(GTest) is invoked

@ferdymercury
Copy link
Copy Markdown
Collaborator Author

ferdymercury commented Jun 3, 2026

Probably one need to do this like in ROOT_FIND_REQUIRED_DEP macro.

Ok for me to add this, but I think it's a separate issue. There are many builtins that do not have yet this behavior, it's only for

# Request explicit user opt-in for required "easy to self-install" dependencies

Maybe all builtins considered easy should be enabled at one, not just gtest.

@linev
Copy link
Copy Markdown
Member

linev commented Jun 3, 2026

gtest and gmock are really available on all platforms.
So making error like for other common builtins should be ok.

@linev
Copy link
Copy Markdown
Member

linev commented Jun 3, 2026

Your second commit goes in right direction.
But one need to remove related code from lines 1270..1300
And in macro ROOT_FIND_REQUIRED_DEP one should be able to provide version of GTest

@ferdymercury
Copy link
Copy Markdown
Collaborator Author

But one need to remove related code from lines 1270..1300
And in macro ROOT_FIND_REQUIRED_DEP one should be able to provide version of GTest

Done!

@ferdymercury ferdymercury requested a review from linev June 3, 2026 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants