Skip to content

[math] migrate old Makefiles to CMake and reenable non-covered tests#21890

Open
ferdymercury wants to merge 15 commits intoroot-project:masterfrom
ferdymercury:smatrix
Open

[math] migrate old Makefiles to CMake and reenable non-covered tests#21890
ferdymercury wants to merge 15 commits intoroot-project:masterfrom
ferdymercury:smatrix

Conversation

@ferdymercury
Copy link
Copy Markdown
Collaborator

Split-off #21820 as requested by @pcanal and @linev

@ferdymercury ferdymercury requested a review from bellenot as a code owner April 12, 2026 07:26
@ferdymercury ferdymercury changed the title [math] migrate from Makefile to CMake and reenable non-covered tests [math] migrate old Makefiles to CMake and reenable non-covered tests Apr 12, 2026
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 12, 2026

Test Results

    22 files      22 suites   3d 4h 39m 13s ⏱️
 3 841 tests  3 837 ✅  1 💤  3 ❌
75 821 runs  75 781 ✅ 18 💤 22 ❌

For more details on these failures, see this check.

Results for commit 5fd8c35.

♻️ This comment has been updated with latest results.

Co-authored-by: ferdymercury <ferdymercury@users.noreply.github.com>
Copy link
Copy Markdown
Member

@linev linev left a comment

Choose a reason for hiding this comment

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

There are three points in smatrix/test/ subdirectory:

  1. Do we need test reflex dictionaries
  2. Is linking with CLHEP library crucial, do it properly
  3. How to use old ROOT files stored in the directory

target_link_directories(${testname} PRIVATE "${CLHEPBASE}/lib")
endif()
endif()
ROOT_ADD_TEST(smatrix-${testname} COMMAND ${testname} LABELS ${${testname}_LABELS})
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There are two ROOT files in the directory which were generated by the tests.
One should try use these files for tests.
Most probably one need to make custom ROOT_ADD_TEST commands providing file name as argument

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

you mean checking that the result generated by the test is compatible with that reference?

https://github.com/ferdymercury/root/blob/5ce524b96321eb747d38c3c7358980ee2ef0976d/math/smatrix/test/testOperations.cxx#L776

they get recreated at every run. so i guess that reference was done on an old SLC3 and gcc3

@ferdymercury
Copy link
Copy Markdown
Collaborator Author

There are three points in smatrix/test/ subdirectory:

1. Do we need test reflex dictionaries

2. Is linking with CLHEP library crucial, do it properly

3. How to use old ROOT files stored in the directory

No idea whether those things are relevant, I just reenabled something that was there. If not relevant, we should just delete the whole test :)

Co-authored-by: ferdymercury <ferdymercury@users.noreply.github.com>
Comment on lines +22 to +26
if (DEFINED ENV{USE_REFLEX})
REFLEX_GENERATE_DICTIONARY(GVTrackDict Track.h SELECTION TrackDict.xml)
else()
ROOT_GENERATE_DICTIONARY(GVTrackDict Track.h LINKDEF TrackLinkDef.h OPTIONS -inlineInputHeader)
endif()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would either drop one or always run both.

Suggested change
if (DEFINED ENV{USE_REFLEX})
REFLEX_GENERATE_DICTIONARY(GVTrackDict Track.h SELECTION TrackDict.xml)
else()
ROOT_GENERATE_DICTIONARY(GVTrackDict Track.h LINKDEF TrackLinkDef.h OPTIONS -inlineInputHeader)
endif()
REFLEX_GENERATE_DICTIONARY(GVTrackDictXml Track.h SELECTION TrackDict.xml)
ROOT_GENERATE_DICTIONARY(GVTrackDict Track.h LINKDEF TrackLinkDef.h OPTIONS -inlineInputHeader)

and also duplicate the test accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants