-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[roottest] Fix roottest trying to invoke /usr/bin/root.exe in tests. #20818
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
base: master
Are you sure you want to change the base?
Conversation
7ccf1e0 to
c55369f
Compare
Test Results 22 files 22 suites 3d 16h 23m 1s ⏱️ For more details on these failures, see this check. Results for commit 58a965d. ♻️ This comment has been updated with latest results. |
f523bff to
98e28dd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much, great initiative!
There is however something that needs to happen at the same time as a corollary. The find_package(ROOT) sets these ROOT feature variables like ROOT_pyroot_FOUND, which are extensively used in the tests (for example here).
Now that these variables are not set, the features are not detected and many tests are skipped, like the linked roottest-python-pythonizations-pythonizations. You can compare your CI runs with the nightlies and count the number of tests, and then the extent of this gets clear. For example, in this PR, 3706 test ran on alma10, and in the last nightlies there were 3742.
What you need to do is the same PR (or maybe another one that we merge before) is to replace all these feature variables with the CMake variables that ROOT sets at build time, like pyroot.
"message(ERROR" is not valid CMake.
The test was enabled based on whether ROOT_cintex_FOUND was set. This, however, will never be set, so the test was always disabled.
98e28dd to
4d45862
Compare
I raise to 3746. I noticed this also locally, since there were subfolders where the (previously plain and local) CMake variables didn't get propagated correctly. Now, since the ifs in rootest rely on global CACHE variables, they are visible in all subdirectories. |
The build options called ROOT_xxx_FOUND originate from find_package(ROOT). Given that roottest is built as part of the ROOT build, one can now directly use the build option flags from ROOT's CMake. This will allow to remove the find_package(ROOT) step in the future. This change enabled the following test, which was disabled because ROOT_mathmore_FOUND wasn't set correctly: +gtest-roofit-roofit-vectorisedPDFs-testLegendre
When ROOT is installed in the system, roottest would try invoking /usr/bin/root.exe, leading to failures when trying to combine .pcms from two different ROOT builds. This was caused by a find_package(ROOT) and find_program(root-config), which find the system ROOT before the ROOT being configured. With roottest being part of the ROOT build, any find_package(ROOT) and discovery of root-config and ROOTSYS could be removed, since these variables are anyway known already in the running CMake configuration. This enabled the following tests, which were disabled because the ROOT_xxx_FOUND flags weren't propagated correctly to the subdirectory: +roottest-python-JupyROOT-cppcompleter_doctest +roottest-python-JupyROOT-handlers_doctest +roottest-python-JupyROOT-utils_doctest
4d45862 to
58a965d
Compare
guitargeek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks for this! Looks good now
OK, needs one more iteration, though. The following ifs don't do what we expected on Windows: root/roottest/python/cling/CMakeLists.txt Lines 11 to 18 in 6ecf2d7
It's understood now. The logic will be updated in #20830 |
b6fbeef to
58a965d
Compare
When ROOT is installed in the system, roottest would try invoking /usr/bin/root.exe, leading to failures when trying to combine .pcms from two different ROOT builds.
This was caused by a find_package(ROOT) and find_program(root-config) at the start of the roottest config, which find the system ROOT before the ROOT being configured. With roottest being part of the ROOT build, any find_package(ROOT) and discovery of root-config and ROOTSYS could be removed, since these variables are anyway known already in the running CMake configuration.
This entailed a move from
if(ROOT_<component>_FOUND)toif(<component>), since the former is only set byfind_package(ROOT)whereas the latter always available. As a side effect of this move, 4 tests enabled themselves, because theROOT_xxx_FOUNDapparently didn't propagate correctly to all locations in roottest. These are:roottest-python-JupyROOT-cppcompleter_doctest
roottest-python-JupyROOT-handlers_doctest
roottest-python-JupyROOT-utils_doctest
gtest-roofit-roofit-vectorisedPDFs-testLegendre
Wait for [cmake] Simplify logic disabling runPyClassTest.C on Windows #20830 to be merged