Skip to content
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

Re-enable fastcloningeventtree tests #1059

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

vepadulano
Copy link
Member

@vepadulano vepadulano self-assigned this Feb 13, 2024
@phsft-bot
Copy link

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default
How to customize builds

@phsft-bot
Copy link

Build failed on ROOT-ubuntu2004/python3.
Running on root-ubuntu-2004-1.cern.ch:/home/sftnight/build/workspace/roottest-pullrequests-build
See console output.

Failing tests:

@phsft-bot
Copy link

Build failed on windows10/default.
Running on null:C:\build\workspace\roottest-pullrequests-build
See console output.

Failing tests:

@dpiparo
Copy link
Member

dpiparo commented Feb 16, 2024

I think we are almost there with this fix. It is just necessary to disable this test on Windows.

@pcanal
Copy link
Member

pcanal commented Feb 16, 2024

@bellenot Is there an easy way to fix this test on Windows (seem a question of access a local Event.lib file, maybe it was not copied when the .dll was copied).?

@bellenot
Copy link
Member

bellenot commented Feb 16, 2024

@bellenot Is there an easy way to fix this test on Windows (seem a question of access a local Event.lib file, maybe it was not copied when the .dll was copied).?

I already took a look and explained to Vincenzo. The Event.lib (and dll) are generated using a gnu makefile, hence not compatible with Windows. One should port that to CMake...
I.e in root\tree\fastcloningeventtree\CMakeLists.txt there is set(ROOT_EVENT_DIR ${ROOTTEST_DIR}/root/treeformula/event/), and in ${ROOTTEST_DIR}/root/treeformula/event/ there is ROOTTEST_ADD_OLDTEST(), which is not compatible with Windows

@phsft-bot
Copy link

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default
How to customize builds

@vepadulano
Copy link
Member Author

It turns out these files were already ported to cmake previously, then the change was reverted #716

I re-reverted the change, make the needed adjustments afterwards using fixtures. On my machine everything works again now, let's see the Windows CI

@phsft-bot
Copy link

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default
How to customize builds

@phsft-bot
Copy link

Build failed on mac12arm/cxx20.
Running on 194.12.161.128:/Users/sftnight/build/workspace/roottest-pullrequests-build
See console output.

Warnings:

  • [2024-02-20T00:11:45.950Z] /Users/sftnight/build/workspace/roottest-pullrequests-build/roottest/root/treeformula/event/TreeFormulaEvent.cxx:146:3: warning: 'sprintf' is deprecated: This function is provided for compatibility reasons only. Due to security concerns inherent in the design of sprintf(3), it is highly recommended that you use snprintf(3) instead. [-Wdeprecated-declarations]
  • [2024-02-20T00:11:45.950Z] /Users/sftnight/build/workspace/roottest-pullrequests-build/roottest/root/treeformula/event/TreeFormulaEvent.cxx:147:3: warning: 'sprintf' is deprecated: This function is provided for compatibility reasons only. Due to security concerns inherent in the design of sprintf(3), it is highly recommended that you use snprintf(3) instead. [-Wdeprecated-declarations]

Failing tests:

@phsft-bot
Copy link

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default
How to customize builds

@phsft-bot
Copy link

Build failed on ROOT-ubuntu2204/nortcxxmod.
Running on root-ubuntu-2204-1.cern.ch:/home/sftnight/build/workspace/roottest-pullrequests-build
See console output.

Failing tests:

@phsft-bot
Copy link

Build failed on ROOT-performance-centos8-multicore/soversion.
Running on olbdw-01.cern.ch:/data/sftnight/workspace/roottest-pullrequests-build
See console output.

Failing tests:

@phsft-bot
Copy link

Build failed on mac12arm/cxx20.
Running on 194.12.161.128:/Users/sftnight/build/workspace/roottest-pullrequests-build
See console output.

Warnings:

  • [2024-02-20T19:27:02.773Z] /Users/sftnight/build/workspace/roottest-pullrequests-build/roottest/root/treeformula/event/TreeFormulaEvent.cxx:146:3: warning: 'sprintf' is deprecated: This function is provided for compatibility reasons only. Due to security concerns inherent in the design of sprintf(3), it is highly recommended that you use snprintf(3) instead. [-Wdeprecated-declarations]
  • [2024-02-20T19:27:02.773Z] /Users/sftnight/build/workspace/roottest-pullrequests-build/roottest/root/treeformula/event/TreeFormulaEvent.cxx:147:3: warning: 'sprintf' is deprecated: This function is provided for compatibility reasons only. Due to security concerns inherent in the design of sprintf(3), it is highly recommended that you use snprintf(3) instead. [-Wdeprecated-declarations]

* The `fastcloningeventtree` folder is updated to using cmake instead of make.
* The `ctrans.C` macro now properly loads the shared library built at the same
  directory of the input file
* Relationship between `fastcloningeventtree` and `event` is made cleared. The
  first depends on the latter. They are thus placed in the same roottest subfolder
@phsft-bot
Copy link

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default
How to customize builds

@phsft-bot
Copy link

Build failed on ROOT-performance-centos8-multicore/soversion.
Running on olbdw-01.cern.ch:/data/sftnight/workspace/roottest-pullrequests-build
See console output.

Failing tests:

@vepadulano
Copy link
Member Author

@pcanal this is now ready for review

@vepadulano
Copy link
Member Author

@phsft-bot build

@phsft-bot
Copy link

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default
How to customize builds

@phsft-bot
Copy link

Build failed on ROOT-ubuntu2204/nortcxxmod.
Running on root-ubuntu-2204-2.cern.ch:/home/sftnight/build/workspace/roottest-pullrequests-build
See console output.

Failing tests:

@phsft-bot
Copy link

Build failed on ROOT-performance-centos8-multicore/soversion.
Running on olbdw-01.cern.ch:/data/sftnight/workspace/roottest-pullrequests-build
See console output.

Failing tests:

@phsft-bot
Copy link

Build failed on ROOT-ubuntu2004/python3.
Running on root-ubuntu-2004-1.cern.ch:/home/sftnight/build/workspace/roottest-pullrequests-build
See console output.

Failing tests:

@pcanal
Copy link
Member

pcanal commented Feb 23, 2024

@phsft-bot build with flags -DCTEST_TEST_EXCLUDE_NONE=On

@phsft-bot
Copy link

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default with flags -DCTEST_TEST_EXCLUDE_NONE=On
How to customize builds

ROOTTEST_ADD_TEST("${file_name}-${mode}"
COMMAND ${ROOT_root_CMD} -q -b -l "${CMAKE_CURRENT_SOURCE_DIR}/dt_wrap.C\(\"${file_name}.root\",${mode}\)"
DEPENDS EventGeneration
RUN_SERIAL
Copy link
Member

@pcanal pcanal Feb 23, 2024

Choose a reason for hiding this comment

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

There should be a comment mentioning why this is here.

EVENT_GENERATE_FILE(split-nine ${size} ${comp} 9 ${action} ${tracks})
EVENT_GENERATE_TESTS(split-nine)

EVENT_GENERATE_FILE(stream-old ${size} ${comp} -1 ${action} ${tracks})
Copy link
Member

@pcanal pcanal Feb 23, 2024

Choose a reason for hiding this comment

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

Suggested change
EVENT_GENERATE_FILE(stream-old ${size} ${comp} -1 ${action} ${tracks})
EVENT_GENERATE_FILE(stream-old 20 ${comp} -1 ${action} 30)

was the old behavior ...

#
function(EVENT_GENERATE_TESTS file_name)
foreach(mode 0;1;2;3;4)
ROOTTEST_ADD_TEST("${file_name}-${mode}"
Copy link
Member

@pcanal pcanal Feb 23, 2024

Choose a reason for hiding this comment

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

The essential part of the testing was comparing to the references files (See OUTREF is the old cmake files), are we still doing that and/or what did we replace it with. (dt_RunDrawTest.sh was to cleanup the output so that despite intentional differences one could compare to a single reference file; see the grep -v in there).

This comparison was used to check for unexpected warning or error messages.

@@ -0,0 +1,20 @@
void ctrans(const char *filename)
Copy link
Member

Choose a reason for hiding this comment

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

We may want to use git mv to keep track of the history. (My github display claims it wasn't used).

Copy link
Member

Choose a reason for hiding this comment

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

Can you confirm to git mv was used?

Comment on lines -116 to -117
$(MERGEFILES): %: $(ONEFILES) $(TWOFILES) | merged
$(CMDECHO) $(CALLROOTEXE) -q -b -l 'mtrans.C("$(@F)")' > [email protected] 2>&1
Copy link
Member

Choose a reason for hiding this comment

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

Which targets are covering this set of tests?

Comment on lines -140 to -141
CloneTree.root: make_CloneTree.C
$(CMDECHO) $(CALLROOTEXE) -q -b -l make_CloneTree.C > make_CloneTree.log 2>&1
Copy link
Member

Choose a reason for hiding this comment

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

This test seems to be missing.

Comment on lines -153 to -155
SplitMismatch.clog: runSplitMismatch_C.$(DllSuf)
$(CMDECHO) $(CALLROOTEXE) -q -b -l runSplitMismatch.C+ 2>&1 | sed -e 's:file .*roottest:file .../roottest:' > SplitMismatch.clog

Copy link
Member

Choose a reason for hiding this comment

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

Can you also check into this one?

Comment on lines -159 to -182
Tuple_merge.root: Tuple_1.root Tuple_2.root $(ROOTCORELIBS)
$(CMDECHO) hadd -f Tuple_merge.root Tuple_1.root Tuple_2.root > Tuple_merge.log

outoforder.log: Tuple_merge.root

outoforder: outoforder.log
$(TestDiff)

filemergererror: filemergererror.log
$(TestDiff)

badmix: badmix.log
$(TestDiff)


abstract.root: abstract_C.$(DllSuf)
$(CMDECHO) root.exe -b -l -q 'abstract.C+(0)' > abstract_root.log 2>&1

copy-abstract.root: abstract_C.$(DllSuf) abstract.root
$(CMDECHO) root.exe -b -l -q 'abstract.C+(1)' > abstract_root.log 2>&1

abstract.log: copy-abstract.root

abstract: abstract.log
Copy link
Member

@pcanal pcanal Feb 23, 2024

Choose a reason for hiding this comment

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

Can you also check into these tests? (outoforder, filemergererror, badmix, abstract, copy-abstract)

@phsft-bot
Copy link

Build failed on ROOT-ubuntu2004/python3.
Running on root-ubuntu-2004-1.cern.ch:/home/sftnight/build/workspace/roottest-pullrequests-build
See console output.

Failing tests:

@phsft-bot
Copy link

Build failed on ROOT-performance-centos8-multicore/soversion.
Running on olbdw-01.cern.ch:/data/sftnight/workspace/roottest-pullrequests-build
See console output.

Failing tests:

@dpiparo
Copy link
Member

dpiparo commented May 29, 2024

I propose to merge this PR to close root-project/root#14579 and work on the other tests at a later stage: does it make sense?

@pcanal
Copy link
Member

pcanal commented May 29, 2024

There are several outstanding issue/conversation holding back the merge.

Copy link
Member

@pcanal pcanal left a comment

Choose a reason for hiding this comment

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

See comments.

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.

[ROOT-10190] Excluded fastcloningeventtree test in roottest.git should be re-enabled back
5 participants