Update cling in ci to version 1.1 using roots llvm 16 #358

mcbarton · 2024-11-29T19:42:37Z

This PR will update cling in the ci to version 1.1 using root llvm 16

aaronj0 · 2024-11-29T20:05:39Z

@mcbarton this is already ongoing in #333, rebasing that PR should do the job. The reason interop should build now is because the offsets to obtain the JIT was changed in #330 . I just never rebased the CI pr after that

mcbarton · 2024-11-29T20:23:56Z

@mcbarton this is already ongoing in #333, rebasing that PR should do the job. The reason interop should build now is because the offsets to obtain the JIT was changed in #330 . I just never rebased the CI pr after that

@vgvassilev @aaronj0 #333 has issues for me that are easier to resolve by just starting a new PR. We shouldn't have the cling version set to 'refs/heads/master' . This will just cause the cache is fill up quickly. The names have been made inconsistent by dropping the compiler from the job name on osx. Its just brought back lots of jobs that were removed from the ci.

aaronj0 · 2024-11-29T20:28:35Z

@vgvassilev @aaronj0 #333 has issues for me that are easier to resolve by just starting a new PR. We shouldn't have the cling version set to 'refs/heads/master' . This will just cause the cache is fill up quickly. The names have been made inconsistent by dropping the compiler from the job name on osx. Its just brought back lots of jobs that were removed from the ci.

It's not ready yet... The reason you see the old jobs is because it's many commits behind, but that is relatively easy to resolve which I am doing right now. I don't think opening a fresh PR is the solution.

We shouldn't have the cling version set to 'refs/heads/master'. This will just cause the cache is fill up quickly

As for 'refs/head/master', cling with llvm18 doesn't have a release tag yet. The purpose of this is to have a real-time test of upstream cling

mcbarton · 2024-11-29T20:36:34Z

@vgvassilev @aaronj0 #333 has issues for me that are easier to resolve by just starting a new PR. We shouldn't have the cling version set to 'refs/heads/master' . This will just cause the cache is fill up quickly. The names have been made inconsistent by dropping the compiler from the job name on osx. Its just brought back lots of jobs that were removed from the ci.

It's not ready yet... The reason you see the old jobs is because it's many commits behind, but that is relatively easy to resolve which I am doing right now. I don't think opening a fresh PR is the solution.

We shouldn't have the cling version set to 'refs/heads/master'. This will just cause the cache is fill up quickly

As for 'refs/head/master', cling with llvm18 doesn't have a release tag yet. The purpose of this is to have a real-time test of upstream cling

@aaronj0 We can't have a real time test of the cling based on llvm 18 in the ci as it would be constantly be rebuilding the cache, overwhelming the little space we have very quickly. If you drop this from the PR and make it specifically about cling 1.1 and llvm 16, and revert the names back to be consistent then I'll drop this PR.

codecov · 2024-11-29T21:56:09Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.87%. Comparing base (f1899af) to head (27b6a14).
Report is 90 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #358   +/-   ##
=======================================
  Coverage   70.87%   70.87%           
=======================================
  Files           9        9           
  Lines        3533     3533           
=======================================
  Hits         2504     2504           
  Misses       1029     1029

github-actions · 2024-11-29T22:05:59Z

clang-tidy review says "All clean, LGTM! 👍"

aaronj0 · 2024-11-29T23:03:08Z

@vgvassilev @aaronj0 #333 has issues for me that are easier to resolve by just starting a new PR. We shouldn't have the cling version set to 'refs/heads/master' . This will just cause the cache is fill up quickly. The names have been made inconsistent by dropping the compiler from the job name on osx. Its just brought back lots of jobs that were removed from the ci.

It's not ready yet... The reason you see the old jobs is because it's many commits behind, but that is relatively easy to resolve which I am doing right now. I don't think opening a fresh PR is the solution.

We shouldn't have the cling version set to 'refs/heads/master'. This will just cause the cache is fill up quickly

As for 'refs/head/master', cling with llvm18 doesn't have a release tag yet. The purpose of this is to have a real-time test of upstream cling

@aaronj0 We can't have a real time test of the cling based on llvm 18 in the ci as it would be constantly be rebuilding the cache, overwhelming the little space we have very quickly. If you drop this from the PR and make it specifically about cling 1.1 and llvm 16, and revert the names back to be consistent then I'll drop this PR.

We will cut a tag for it, or use a commit. Either way I don't see the point of duplicating the previous PR

github-actions · 2024-11-29T23:04:18Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2024-11-29T23:13:53Z

clang-tidy review says "All clean, LGTM! 👍"

Vipul-Cariappa

LGTM! Maybe we should also update the README with updated versions for build instructions.

I spent some time debugging the failure. It is caused when there is an error in the code we are executing (source).

Stack trace goes through

https://github.com/root-project/cling/blob/b5e06a2a7b0832f034d7e405f42f0770364b607f/lib/Interpreter/IncrementalParser.cpp#L914

Destructor of llvm::Expected https://github.com/root-project/llvm-project/blob/cling-llvm16/llvm/include/llvm/Support/Error.h#L551-L552

https://github.com/root-project/llvm-project/blob/cling-llvm16/llvm/include/llvm/Support/Error.h#L714-L715

The logic looks valid in my eyes.

The error msg is

Expected<T> must be checked before access or destruction.
Unchecked Expected<T> contained error

With the following patch for cling the CppInterOp tests pass

diff --git a/lib/Interpreter/IncrementalParser.cpp b/lib/Interpreter/IncrementalParser.cpp
index 89cd78d..2aea40d 100644
--- a/lib/Interpreter/IncrementalParser.cpp
+++ b/lib/Interpreter/IncrementalParser.cpp
@@ -911,8 +911,11 @@ namespace cling {
     PP.EnterSourceFile(FID, /*DirLookup*/nullptr, NewLoc);
     m_Consumer->getTransaction()->setBufferFID(FID);
 
-    if (!ParseOrWrapTopLevelDecl())
+    llvm::Expected<bool> res = ParseOrWrapTopLevelDecl();
+    if (!res) {
+      llvm::consumeError(std::move(res.takeError()));
       return kFailed;
+    }
 
     if (PP.getLangOpts().DelayedTemplateParsing) {
       // Microsoft-specific:

@vgvassilev may have to take a look once.

vgvassilev · 2024-11-30T06:35:22Z

LGTM! Maybe we should also update the README with updated versions for build instructions.

I spent some time debugging the failure. It is caused when there is an error in the code we are executing (source).

Stack trace goes through

https://github.com/root-project/cling/blob/b5e06a2a7b0832f034d7e405f42f0770364b607f/lib/Interpreter/IncrementalParser.cpp#L914

Destructor of llvm::Expected https://github.com/root-project/llvm-project/blob/cling-llvm16/llvm/include/llvm/Support/Error.h#L551-L552

https://github.com/root-project/llvm-project/blob/cling-llvm16/llvm/include/llvm/Support/Error.h#L714-L715

The logic looks valid in my eyes.

The error msg is
Expected<T> must be checked before access or destruction.
Unchecked Expected<T> contained error
With the following patch for cling the CppInterOp tests pass
diff --git a/lib/Interpreter/IncrementalParser.cpp b/lib/Interpreter/IncrementalParser.cpp
index 89cd78d..2aea40d 100644
--- a/lib/Interpreter/IncrementalParser.cpp
+++ b/lib/Interpreter/IncrementalParser.cpp
@@ -911,8 +911,11 @@ namespace cling {
     PP.EnterSourceFile(FID, /*DirLookup*/nullptr, NewLoc);
     m_Consumer->getTransaction()->setBufferFID(FID);
 
-    if (!ParseOrWrapTopLevelDecl())
+    llvm::Expected<bool> res = ParseOrWrapTopLevelDecl();
+    if (!res) {
+      llvm::consumeError(std::move(res.takeError()));
       return kFailed;
+    }
 
     if (PP.getLangOpts().DelayedTemplateParsing) {
       // Microsoft-specific:
@vgvassilev may have to take a look once.

Makes sense. Please open a pr.

Vipul-Cariappa · 2024-11-30T06:36:32Z

Makes sense. Please open a pr.

PR at cling?

vgvassilev · 2024-11-30T07:15:55Z

Makes sense. Please open a pr.

PR at cling?

Yes.

Vipul-Cariappa · 2024-11-30T13:24:26Z

Hi @vgvassilev I have opened the PR at root-project/cling#538.

I was thinking... after merging the PR at Cling, we will have to create a new release, v1.2, before we can use that fix here?
Is it better to just include the fix as a patch file in patches directory in this repo and apply it?

FYI, with this patch, CppInterOp tests pass, but cppyy tests fail. I will start looking into it.

Any thoughts @mcbarton

vgvassilev · 2024-11-30T16:08:42Z

Hi @vgvassilev I have opened the PR at root-project/cling#538.

I was thinking... after merging the PR at Cling, we will have to create a new release, v1.2, before we can use that fix here? Is it better to just include the fix as a patch file in patches directory in this repo and apply it?

FYI, with this patch, CppInterOp tests pass, but cppyy tests fail. I will start looking into it.

Any thoughts @mcbarton

Yeah, I could release cling...

mcbarton · 2024-11-30T23:18:05Z

@Vipul-Cariappa I have added your patch to the workflow to see it working. I have disabled building cppyy, until you determine what is the issue stopping cppyy from passing with cling 1.1. If this patch works then I will update the documentation.

github-actions · 2024-11-30T23:18:34Z

clang-tidy review says "All clean, LGTM! 👍"

mcbarton · 2024-12-01T09:25:23Z

@Vipul-Cariappa any idea how to fix the failing osx jobs? The errror is the same as what you fixed for Ubuntu.

github-actions · 2024-12-01T22:03:51Z

clang-tidy review says "All clean, LGTM! 👍"

Vipul-Cariappa · 2024-12-02T03:56:02Z

Debugging OSX failures at #359.

Looks like Cpp::CreateInterpreter fails.
Backtrace:

I will need to rebuild cling and llvm in debug. It is going to be time consuming.

Vipul-Cariappa · 2024-12-02T04:52:13Z

Hi @mcbarton.

If I run git diff on cling, I do not see my patch applied.
I guess my patch is not applied?

Do we need to rebuild the cache?

Or update the logic of retrieving the cache?
I see some complicated key in the retrieval step

path: |
          llvm-project
          ${{ matrix.cling=='On' && 'cling' || '' }}
key: ${{ env.CLING_HASH }}-${{ runner.os }}-${{ matrix.os }}-${{ matrix.compiler }}-clang-${{ matrix.clang-runtime }}.x-patch-${{ hashFiles(format('patches/llvm/clang{0}-*.patch', matrix.clang-runtime)) || 'none' }}

I am coming to this conclusion by looking at the CI run of dbc5d38 (Update cling16-1-Error-Handling.patch to add blank line) commit. I see that ubuntu-cling still fails for the same reason.
With that patch Ubuntu-Cling should pass, at least it passes in my local machine.

mcbarton · 2024-12-02T07:45:23Z

@Vipul-Cariappa The Ubuntu job passes with your patch. See https://github.com/compiler-research/CppInterOp/actions/runs/12100202323/job/33748369555

Vipul-Cariappa · 2024-12-02T08:28:25Z

Weird. Looks like the OSX failure may not be caused in this round of CI run.
I am speculating here., as OSX-arm passed. Waiting for OSX-X86 and Ubuntu to finish.

@Vipul-Cariappa The Ubuntu job passes with your patch. See https://github.com/compiler-research/CppInterOp/actions/runs/12100202323/job/33748369555

Which commit is this?

Any idea why Ubuntu-Cling fails in my PR https://github.com/compiler-research/CppInterOp/actions/runs/12112309439/job/33766010006?pr=359? It fails with the same error, my patch is supposed to fix.

.github/workflows/ci.yml

github-actions · 2024-12-05T15:48:07Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2024-12-05T15:55:34Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2024-12-20T08:26:25Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2024-12-20T08:40:00Z

clang-tidy review says "All clean, LGTM! 👍"

mcbarton · 2025-02-22T09:41:46Z

Closing this as the commit of this PR isbahdd to follow, and now workflow files have gone through substantial changes since this PR was created. Will recreate as a fresh PR with clean commit history.

Vipul-Cariappa · 2025-05-16T07:24:12Z

Hi @mcbarton, could you please reopen these changes. We are now interested to get this merged quickly (I guess, at least I am).

@aaronj0, @vgvassilev can we skip cling v1.1 and directly try using v1.2?

mcbarton · 2025-05-16T07:29:23Z

Hi @mcbarton, could you please reopen these changes. We are now interested to get this merged quickly (I guess, at least I am).

@aaronj0, @vgvassilev can we skip cling v1.1 and directly try using v1.2?

@Vipul-Cariappa I will look into this today. I'll make a fresh PR which adds cling 1.1 for linux to the ci (since we know that works, and the issues is macos) and i'll open up a separate PR for cling 1.2

Vipul-Cariappa · 2025-05-16T07:32:13Z

I'll make a fresh PR which adds cling 1.1 for linux to the ci (since we know that works, and the issues is macos) and i'll open up a separate PR for cling 1.2

Just 1.2 should be sufficient, I don't want to deal with old versions of LLVM. Unless Aaron or Vassil wants to support 1.1.

mcbarton · 2025-05-16T07:37:56Z

I'll make a fresh PR which adds cling 1.1 for linux to the ci (since we know that works, and the issues is macos) and i'll open up a separate PR for cling 1.2

Just 1.2 should be sufficient, I don't want to deal with old versions of LLVM. Unless Aaron or Vassil wants to support 1.1.

I think its best if we have the 1.1 job in the ci for linux. It would look strange if we support 1.0 and 1.2, but not 1.1 at some level at least (with at least meaning just linux). If Aaron and Vassil agree with you though that's fine. We would probably need some sort of note to that effect though in the documentation, if that was the case.

Vipul-Cariappa · 2025-05-16T07:42:44Z

I think its best if we have the 1.1 job in the ci for linux. It would look strange if we support 1.0 and 1.2, but not 1.1 at some level at least (with at least meaning just linux).

Once we support 1.2, we plan on dropping 1.0. So it should be ok if we don't support 1.1.
Maybe we can have this discussion later, once we have something.
Please note that I will be prioritizing 1.2 for now.

mcbarton · 2025-05-16T07:44:39Z

I'll make a fresh PR which adds cling 1.1 for linux to the ci (since we know that works, and the issues is macos) and i'll open up a separate PR for cling 1.2

Just 1.2 should be sufficient, I don't want to deal with old versions of LLVM. Unless Aaron or Vassil wants to support 1.1.

It should be noted that for clang repl, where we couldn't get a feature working for an older llvm, we just skip over the test. The point of having the old llvms in the ci is more to check that an existing feature that already works isn't broken by a patch

vgvassilev · 2025-05-16T09:11:21Z

I think supporting the latest released cling is fine.

mcbarton · 2025-05-16T09:31:07Z

@Vipul-Cariappa I have made a PR here making the cling ci jobs 1.2 instead of 1.0 #580 . I will make a separate PR on Monday which drops cling 1.0 support from CppInterOp, rather than dropping support for it in the same PR which adds 1.2 support, since you say you want this change to the ci soon.

Update cling in ci to version 1.1 using root llvm 16

e92dc4c

mcbarton changed the title ~~Update cling in ci to version 1.1 using root llvm 16~~ Update cling in ci to version 1.1 using roots llvm 16 Nov 29, 2024

vgvassilev requested review from aaronj0 and Vipul-Cariappa November 29, 2024 20:20

Only build gtest_main for cling llvm=13 ci

bb10c03

mcbarton requested review from alexander-penev and removed request for aaronj0 November 29, 2024 20:51

Disable CUDA tests for Cling

05f2544

mcbarton force-pushed the upgrade-cling-version-ci branch from 930768c to 05f2544 Compare November 29, 2024 23:09

Vipul-Cariappa reviewed Nov 30, 2024

View reviewed changes

mcbarton added 2 commits November 30, 2024 23:12

Apply patch needed to fix error handling for cling version 1.1

0d9e21d

Update cling16-1-Error-Handling.patch to add blank line

dbc5d38

Update job names

1055a9c

alexander-penev approved these changes Dec 3, 2024

View reviewed changes

vgvassilev requested changes Dec 3, 2024

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

mcbarton added 2 commits December 5, 2024 15:42

Merge branch 'main' into upgrade-cling-version-ci

c1c9467

Update ci.yml to bring back cling 1.0 jobs

1d49cfb

Update ci.yml fix names to add -cppyy to cling 1.0 jobs

138bdf7

mcbarton added 3 commits December 20, 2024 08:19

Revert changes to allow conflicts to be fixed

8dab1ec

Merge branch 'main' into upgrade-cling-version-ci

d7950f7

Bring back changes that fixing conflicts removed

27b6a14

mcbarton closed this Feb 22, 2025

Vipul-Cariappa mentioned this pull request Feb 22, 2025

tmp/cling upgrade v1.1 #359

Closed

Update cling in ci to version 1.1 using roots llvm 16 #358

Update cling in ci to version 1.1 using roots llvm 16 #358

Uh oh!

Conversation

mcbarton commented Nov 29, 2024

Uh oh!

aaronj0 commented Nov 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcbarton commented Nov 29, 2024

Uh oh!

aaronj0 commented Nov 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcbarton commented Nov 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Nov 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Nov 29, 2024

Uh oh!

aaronj0 commented Nov 29, 2024

Uh oh!

github-actions bot commented Nov 29, 2024

Uh oh!

github-actions bot commented Nov 29, 2024

Uh oh!

Vipul-Cariappa left a comment

Choose a reason for hiding this comment

Uh oh!

vgvassilev commented Nov 30, 2024

Uh oh!

Vipul-Cariappa commented Nov 30, 2024

Uh oh!

vgvassilev commented Nov 30, 2024

Uh oh!

Vipul-Cariappa commented Nov 30, 2024

Uh oh!

vgvassilev commented Nov 30, 2024

Uh oh!

mcbarton commented Nov 30, 2024

Uh oh!

github-actions bot commented Nov 30, 2024

Uh oh!

mcbarton commented Dec 1, 2024

Uh oh!

github-actions bot commented Dec 1, 2024

Uh oh!

Vipul-Cariappa commented Dec 2, 2024

Uh oh!

Vipul-Cariappa commented Dec 2, 2024

Uh oh!

mcbarton commented Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Vipul-Cariappa commented Dec 2, 2024

Uh oh!

Uh oh!

github-actions bot commented Dec 5, 2024

Uh oh!

github-actions bot commented Dec 5, 2024

Uh oh!

github-actions bot commented Dec 20, 2024

Uh oh!

github-actions bot commented Dec 20, 2024

Uh oh!

mcbarton commented Feb 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Vipul-Cariappa commented May 16, 2025

Uh oh!

mcbarton commented May 16, 2025

Uh oh!

Vipul-Cariappa commented May 16, 2025

Uh oh!

mcbarton commented May 16, 2025

Uh oh!

Vipul-Cariappa commented May 16, 2025

aaronj0 commented Nov 29, 2024 •

edited

Loading

aaronj0 commented Nov 29, 2024 •

edited

Loading

mcbarton commented Nov 29, 2024 •

edited

Loading

codecov bot commented Nov 29, 2024 •

edited

Loading

mcbarton commented Dec 2, 2024 •

edited

Loading

mcbarton commented Feb 22, 2025 •

edited

Loading