Skip to content

[ci] build with clingtest flag active if interpreter folder was touched#16917

Closed
ferdymercury wants to merge 3 commits intoroot-project:masterfrom
ferdymercury:clingtest
Closed

[ci] build with clingtest flag active if interpreter folder was touched#16917
ferdymercury wants to merge 3 commits intoroot-project:masterfrom
ferdymercury:clingtest

Conversation

@ferdymercury
Copy link
Copy Markdown
Collaborator

@ferdymercury ferdymercury commented Nov 13, 2024

This Pull request:

Changes or fixes:

Fixes #15230
Fixes #6957

Sibling PR in root-project/roottest#1232

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

@ferdymercury ferdymercury marked this pull request as draft November 13, 2024 09:45
@dpiparo dpiparo self-assigned this Nov 15, 2024
@dpiparo dpiparo marked this pull request as ready for review November 30, 2024 11:28
@dpiparo dpiparo closed this Nov 30, 2024
@dpiparo dpiparo reopened this Nov 30, 2024
Copy link
Copy Markdown
Member

@dpiparo dpiparo left a comment

Choose a reason for hiding this comment

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

This PR is a great attempt to introduce a useful new feature. It seems some information is still needed before starting to test it thoroughly.

Comment thread .github/workflows/root-ci.yml Outdated
Comment thread .github/workflows/root-ci.yml Outdated
@dpiparo
Copy link
Copy Markdown
Member

dpiparo commented Nov 30, 2024

@vgvassilev

Comment thread .github/workflows/root-ci.yml Outdated
Comment thread .github/workflows/root-ci.yml Outdated
@vgvassilev
Copy link
Copy Markdown
Member

These changes are too advanced for me. Maybe @mcbarton could take a look...

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Nov 30, 2024

Test Results

    18 files      18 suites   4d 5h 33m 48s ⏱️
 2 690 tests  2 665 ✅ 0 💤 25 ❌
46 567 runs  46 519 ✅ 0 💤 48 ❌

For more details on these failures, see this check.

Results for commit 3db45e0.

♻️ This comment has been updated with latest results.

Comment thread interpreter/cling/CMakeLists.txt Outdated
@ferdymercury
Copy link
Copy Markdown
Collaborator Author

ferdymercury commented Nov 30, 2024

It's working now.

Should we also:

  • set LLVM_BUILD_TYPE=Debug ?
  • add more platforms ? Right now: both windows, one mac, one ubuntu, one linux

@ferdymercury ferdymercury force-pushed the clingtest branch 2 times, most recently from 40b9857 to 6d5e3a8 Compare November 30, 2024 21:56
@ferdymercury ferdymercury requested a review from hahnjo November 30, 2024 22:00
Copy link
Copy Markdown
Member

@hahnjo hahnjo left a comment

Choose a reason for hiding this comment

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

This looks really (too) complicated and IMHO is not what we actually want:

  • check-cling doesn't seem to work in-tree on Windows.
  • On Apple Silicon, we (still) cannot catch exceptions from interpreted code and therefore many (all?) NullDeref tests fail. We probably want to start with an x86 machine.
  • For Linux, as I predicted in #6957 (comment), exposing the LLVM symbols from libCling interferes with other libraries, in this case TensorFlow (but I think we have seen problems with Mesa in the past as well).
  • roottest-root-meta-countIncludePaths checks the number of include paths, which is larger with clingtest=ON to make the tests pass.

In general, as was discussed before, we still want to run all PR builds because this is what matters for ROOT's functionality. On top of this, we want extra builds with clingtest that only execute check-cling. This can be accomplished much easier with a separate workflow that triggers on pull_request with paths.

@ferdymercury
Copy link
Copy Markdown
Collaborator Author

ferdymercury commented Dec 2, 2024

* On Apple Silicon, we (still) cannot catch exceptions from interpreted code and therefore many (all?) `NullDeref` tests fail. We probably want to start with an x86 machine.

Changed.

* `roottest-root-meta-countIncludePaths` checks the number of include paths, which is larger with `clingtest=ON` to make the tests pass.

This should be fixed in my opinion in the CMakeLists using an if(clingtest) --> pass argument of extra include paths. (build_root.py should forward this flag to roottest as -Dclingtest when cmaking) and adapt the ctest limit.

This can be accomplished much easier with a separate workflow that triggers on pull_request with paths.

I hope someone can implement that variant soon ;) and I will close this PR then.

Fixes root-project#15230

[ci] move clingtest to a non-apple-silicon machine

hahnjo: On Apple Silicon, we (still) cannot catch exceptions from interpreted code and therefore many (all?) NullDeref tests fail.
@ferdymercury
Copy link
Copy Markdown
Collaborator Author

Closing in favour of #18127

@ferdymercury ferdymercury deleted the clingtest branch March 26, 2025 14:34
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.

[CI] clingtest=ON when cling touched Enable cling-test for nightly, PR builds

5 participants