fix bug in CMake preventing user from disabling testing

[aaronj0]

There is a bug in the CMake where
option(CPPINTEROP_ENABLE_TESTING "Enables the testing infrastructure." BOOL)
is conditional and for non-Emscripten builds the default is explicitly set to ON. This is incorrect and does not allow users to override the option, leading to testing always being on.

The option keyword must be defined once with one default, which CMake then overrides from the user's command line option

[anutosh491]

Hi,

This looks good. Just curious if we can improve it this way and remove the use of the extra var introduced ...

1. So below these we introduce the default option we want
   

   CppInterOp/CMakeLists.txt

   Lines 60 to 61 in 9c818c5

   	option(CPPINTEROP_USE_CLING "Use Cling as backend" OFF)
   	option(CPPINTEROP_USE_REPL "Use clang-repl as backend" ON)

option(CPPINTEROP_ENABLE_TESTING "Enable the CppInterOp testing infrastructure." ON)

2. And for the emscripten case, we just set the same var as off (of course till in the future we enable testing for this platform too and can remove this)

set(CPPINTEROP_ENABLE_TESTING OFF)

So its just one line at the top that the user controls.

[aaronj0]

Thanks! Indeed that makes much more sense instead of setting a default variable

[mcbarton]

@aaronj0 @vgvassilev As part of my automated wasm tests PR here #483 the tests are on by default for both situations, so don't believe this PR is needed.

[mcbarton]

Don't believe this change is needed due to automated wasm tests PR. Will wait for comment.

[aaronj0]

This is a patch needed for ROOT today

@aaronj0 @vgvassilev As part of my automated wasm tests PR here #483 the tests are on by default for both situations, so don't believe this PR is needed.

I don't understand what you mean. This is a pretty serious bug, ROOT does not even build with testing off since the current code forces the unittests subdir and then breaks with gtest not found. You can always rebase that PR, but I'm not going to drop this important fix for the sake of that

[mcbarton]

Just realised this PR is about allowing the user to override having the tests enable. Sorry, misread the PR description.

[aaronj0]

Just realised this PR is about allowing the user to override having the tests enable. Sorry, misread the PR description.

Can you please drop your changes requested, although I see no changes being requested.

[mcbarton]

Just realised this PR is about allowing the user to override having the tests enable. Sorry, misread the PR description.

Can you please drop your changes requested, although I see no changes being requested.

Already have as far as I can tell.

[aaronj0]

@mcbarton Please refrain from merging PR's like that. I was waiting for the CI run to pass, and a review from @vgvassilev