Use official llvm-project monorepo, expand LLDB versions check

[trxcllnt]

• Fetch headers from the official llvm-project instead of the deprecated llvm-mirror/lldb repo.
• Check for LLDB versions 20.0 - 1.0.0 on Linux so we don't have to update LLDB versions for a few years.

Fixes #350

[simllll]

Looking forward to this.. can we use this fork in the meantime somehow? is it published somewhere?

[trxcllnt]

@simllll it's not published anywhere, but you can see how we're installing it in our development container here:

• Install the LLDB version you'd like to use
• Clone my fork and manually install llnode

[No9]

Tested this today and it worked as expected
@nodejs/diagnostics what can I do to help land this?

[mhdawson]

LGTM.

[mhdawson]

@mmarchini any chance you can help with the CI issues?

[No9]

Looks like we need this PR to land first mmarchini/setup-node#1
@mmarchini I see you're on vacation at the moment so understand if you can't get to this soon but do you think we should look at pulling your setup-node into this org?

[mmarchini]

The mirror option really should be implemented upstream for setup-node :(

There are way to many conflicts on my fork, let me see if I can get a PR going on setup-node. In the meantime, I'll disable the non-fixed major CIs so this PR should be unblocked.

[No9]

Hey @mmarchini
Did you start working on the PR to actions/setup-node?
If you haven't I can look at picking it up.

From what I can see it's these two commits that need reintegrating but let me know if there is anything else or hidden gotchas.
mmarchini/setup-node@6a399da
mmarchini/setup-node@772e957

If they don't accept it we can kick of this process
https://github.com/nodejs/admin/blob/main/transfer-repo-into-the-org.md

[No9]

Update: I've merged the changes from https://github.com/mmarchini/setup-node/commits/mirror
into https://github.com/No9/setup-node/tree/mirror
I've also applied it to my llnode fork and reapplied the v15 settings.
https://github.com/No9/llnode/runs/4206002518?check_suite_focus=true
The change seems to be working as it's picking up the mirror code and runs the tests.
However the change is failing due to verifyObject no longer working in 14 and I think this is reported here
#375
I'll continue to work on the failing test.

[No9]

Update: I've raised this PR to land a green build #398 that we can use as a base to land this.
This PR should probably also add llvm 10 and 11 to the test definitions
https://github.com/nodejs/llnode/blob/main/.github/workflows/push.yml#L32

[trxcllnt]

@No9 This PR should probably also add llvm 10 and 11 to the test definitions

Will do!

[codecov-commenter]

Codecov Report

Merging #389 (5ea886a) into main (39b38a9) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #389      +/-   ##
==========================================
+ Coverage   77.14%   77.18%   +0.03%     
==========================================
  Files          34       34              
  Lines        5023     5027       +4     
==========================================
+ Hits         3875     3880       +5     
+ Misses       1148     1147       -1     

Impacted Files	Coverage Δ
test/plugin/scan-test.js	98.97% <ø> (ø)
test/common.js	86.84% <100.00%> (+0.28%)	⬆️
test/plugin/workqueue-test.js	100.00% <100.00%> (ø)
src/llv8.cc	71.94% <0.00%> (+0.13%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[No9]

Hey @trxcllnt
I've picked up merge rights this week and pulled in the CI fix that this was dependent on.
I know it's been a while but do you want to rebase and then we can get this merged?
I understand we've lost some momentum but it would be great to get it in as it's an important update.

[trxcllnt]

@No9 Sure, I'd be happy to!

[trxcllnt]

Seems like some LLVM versions aren't packaged by Ubuntu. Should I switch the CI runner to use the LLVM apt repositories listed at https://apt.llvm.org/?

[No9]

My first thought is to add mutually exclusive conditionals based on ${{ matrix.llvm }} here
https://github.com/nodejs/llnode/pull/389/files#diff-f3fc934cf0d89bdf07de358896ff90f1202585df812cf606206d1830a9949811R44
Something along the lines of
if [ "${{ matrix.llvm }}" -lt "10" ]; then use distro llvm
if [ "${{ matrix.llvm }}" -gt "9" ]; then Use llvm repos`

I'm conscious that distros tend to do some magic when they are packaging and as that's the most common use case I think it would be a good idea to continue to catch it.

That said, if I've missed a reason why we should default to all llvm I'm happy to hear it.

[trxcllnt]

@No9 it looks like there's a few specific combos of matrix.os + matrix.llvm that don't have packages in universe, so I'll update the job to only use the LLVM repos for those.

[No9]

Hey @trxcllnt
Took a look at the failing tests tonight.
Looks like later versions of lldb are now returning more lines than expected and these are being truncated.
Also the content of the response has changed.

So lldb 10 to 14 is returning

error: expression failed to parse:
error: <user expression 16>:1:1: use of undeclared identifier 'invalid_expr'
invalid_expr

Instead of

error: error: use of undeclared identifier 'invalid_expr'

This is compounded by the logic in flush truncating lines on carriage returns
https://github.com/nodejs/llnode/blob/main/test/common.js#L60
So for 10-14 the only line that is being returned to the test is the ambiguous

error: expression failed to parse:

I've updated my local version of flush to make an exception for this case

      let line = '';
      let index = 0;

      let inv = buf.indexOf('invalid_expr');
      if (inv !== -1) { 
          line = buf;
          index = buf.length;
        } else {
      
        index = buf.indexOf('\n');

        if (index === -1)
          break;
        
        line = buf.slice(0, index);
      }

      buf = buf.slice(index + 1);

And also updated the test to just match on use of undeclared identifier 'invalid_expr'
https://github.com/nodejs/llnode/blob/main/test/plugin/inspect-test.js#L661
https://github.com/nodejs/llnode/blob/main/test/plugin/scan-test.js#L30
I don't think this impacts the integrity of the tests and seems to work.

Very conscious that you've put a lot of work in to this so wanted to give you the option of applying something similar to this PR.
If you don't have the bandwidth and want me to do it I'm also happy to pick it up.

[trxcllnt]

@No9 feel free to push any fixes directly to this branch! Lemme know if you need access. Whatever we need to do to get this passing.

[No9]

Given this PR has been open for 16 months and the last major change was 6 weeks ago and we have an approval I am going to merge this.
@trxcllnt Thank you so much for all your work if you are ever in Dublin Ireland hit me up for a [insert favourite drink]