Skip to content

[interop] Fix build errors with -Dtesting=off #17791

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.

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

Merged
merged 3 commits into from
Mar 11, 2025

Conversation

aaronj0
Copy link
Contributor

@aaronj0 aaronj0 commented Feb 21, 2025

This was due to a bug in CppInterOp where option(CPPINTEROP_ENABLE_TESTING "Enables the testing infrastructure." BOOL) was defined twice conditionally. This is incorrect and does not allow users to override the option, leading to testing always being on. Fixed with compiler-research/CppInterOp#506

This also requires ROOT to set the same flag in the interpreter CMake (which did not work earlier due to the underlying bug in InterOp)

cc @bellenot

Copy link

github-actions bot commented Feb 21, 2025

Test Results

    18 files      18 suites   4d 10h 40m 28s ⏱️
 2 725 tests  2 722 ✅ 0 💤 3 ❌
47 351 runs  47 348 ✅ 0 💤 3 ❌

For more details on these failures, see this check.

Results for commit 0f2b341.

♻️ This comment has been updated with latest results.

@aaronj0 aaronj0 force-pushed the fix-interop-cmake branch 2 times, most recently from 3f4d1a9 to e1f3605 Compare February 21, 2025 14:11
@aaronj0 aaronj0 force-pushed the fix-interop-cmake branch 3 times, most recently from 3e1b7e3 to 4e39ea9 Compare March 7, 2025 13:27
@vgvassilev
Copy link
Member

Why do we need this PR I thought everything was already in CppInterOp?

@aaronj0
Copy link
Contributor Author

aaronj0 commented Mar 7, 2025

Why do we need this PR I thought everything was already in CppInterOp?

Yes the fix from compiler-research/CppInterOp#506 is pulled in by using the later commit, but the testing flags are not correctly propagated and CppInterOp does not configure or check the flags when it is not built standalone. For eg: currently with ROOT you can set both USE_CLING and USE_REPL ON. This is fixed by the patch here which will be opened upstream. The changes in ROOT's interpreter cmake is to set the flags in a better way.

Copy link
Contributor

@guitargeek guitargeek left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the PR, and it also fixed the usecase of building ROOT with -Dfail-on-missin=ON -Dtesting=OFF -Dbuiltin_gtest=OFF without internet connection, which was broken by the initial interop PR. Building ROOT without internet connection is important for some experiments need to build ROOT on network-isolated nodes (see also #11603). I also tested that it works again with this PR.

@aaronj0, please merge when you're ready.

@guitargeek guitargeek requested a review from bellenot March 7, 2025 14:13
@guitargeek guitargeek dismissed bellenot’s stale review March 7, 2025 14:13

Comments addressed.

@aaronj0 aaronj0 force-pushed the fix-interop-cmake branch 3 times, most recently from b8d249d to e398db2 Compare March 10, 2025 15:31
aaronj0 added 2 commits March 10, 2025 21:45
Also moves the addition of `CppInterOpUnitTests` to the build chain, from metacling, to the interpreter cmake
@aaronj0 aaronj0 force-pushed the fix-interop-cmake branch from e398db2 to 0f2b341 Compare March 10, 2025 20:46
@aaronj0
Copy link
Contributor Author

aaronj0 commented Mar 11, 2025

The failures look unrelated to this PR, merging

@aaronj0 aaronj0 merged commit 371b096 into root-project:master Mar 11, 2025
18 of 22 checks passed
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.

4 participants