Use official `llvm-project` monorepo, expand LLDB versions check #389

trxcllnt · 2021-04-30T17:16:16Z

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 · 2021-06-28T10:00:02Z

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

trxcllnt · 2021-06-28T15:03:03Z

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

No9 · 2021-09-24T22:14:50Z

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

mhdawson

LGTM.

mhdawson · 2021-09-27T22:09:23Z

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

No9 · 2021-10-16T15:54:46Z

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 · 2021-10-16T23:04:19Z

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 · 2021-11-13T12:51:59Z

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 · 2021-11-15T00:13:10Z

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 · 2021-11-16T23:34:37Z

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 · 2021-11-18T15:28:23Z

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

Will do!

codecov-commenter · 2021-11-18T15:34:20Z

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 · 2022-06-28T22:16:34Z

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 · 2022-06-30T00:52:26Z

@No9 Sure, I'd be happy to!

trxcllnt · 2022-06-30T00:59:33Z

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 · 2022-06-30T08:08:02Z

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 · 2022-06-30T19:29:32Z

@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.

…node versions matrix

No9 · 2022-08-13T23:42:10Z

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 · 2022-08-15T17:59:39Z

@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.

Signed-off-by: Anton Whalley <[email protected]>

No9 · 2022-08-17T22:22:48Z

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]

Old versions of Ubuntu (and likely other LTS distros) have `git` versions which do not support `git-sparse-checkout`. `git-sparse-checkout` is used as of nodejs#389. Dynamically check if `git sparse-checkout` is supported, and if not then fallback to cloning the whole repo. Cloning the whole repo is _slow_, but at least it works. I tested this patch on Ubuntu 18.04 without any lldb headers installed, and it worked successfully.

Old versions of Ubuntu (and likely other LTS distros) have `git` versions which do not support `git-sparse-checkout`. `git-sparse-checkout` is used as of #389. Dynamically check if `git sparse-checkout` is supported, and if not then fallback to cloning the whole repo. Cloning the whole repo is _slow_, but at least it works. I tested this patch on Ubuntu 18.04 without any lldb headers installed, and it worked successfully.

mhdawson approved these changes Sep 27, 2021

View reviewed changes

No9 mentioned this pull request Nov 9, 2021

llnode failed to install #396

Closed

trxcllnt added 3 commits June 29, 2022 17:53

Check for LLDB up to version 20 in Linux

397a02a

Fetch headers from official LLVM github monorepo

3161e9f

add llvm 10-13 to github actions matrix in push.yml

46bc04d

trxcllnt force-pushed the use-llvm-project-monorepo branch from 138cabc to 46bc04d Compare June 30, 2022 00:53

trxcllnt added 4 commits June 30, 2022 12:46

install lldb from llvm apt repos when not packaged by Ubuntu, update …

5872b49

…node versions matrix

fix bash whitespace

d5144f3

install lldb without prefix when using llvm apt repos

3577f85

fix typos

c3a980f

trxcllnt force-pushed the use-llvm-project-monorepo branch from 3afd655 to c3a980f Compare June 30, 2022 20:02

also symlink llvm-config binary

d65c0cf

No9 mentioned this pull request Aug 14, 2022

Install Fails for LLDB-10 #350

Closed

This was referenced Aug 14, 2022

docs: update Ubuntu install instructions for 20.04 LTS #379

Open

Upgrade GitHub Actions #403

Closed

cclauss mentioned this pull request Aug 14, 2022

changes to support Node.js v16 & v18 #404

Merged

No9 added 4 commits August 17, 2022 20:14

test: update test for multi-line error

1113cee

Signed-off-by: Anton Whalley <[email protected]>

test: remove 16 18 nightly

67d560b

Signed-off-by: Anton Whalley <[email protected]>

test: fix lib/lib mapping issue

acbdb18

Signed-off-by: Anton Whalley <[email protected]>

test: bump timeouts due to later lldbs

5ea886a

Signed-off-by: Anton Whalley <[email protected]>

No9 merged commit ff75da7 into nodejs:main Aug 17, 2022

kvakil mentioned this pull request Oct 1, 2022

scripts: installation without sparse-checkout #425

Merged

trxcllnt mentioned this pull request Mar 7, 2024

Support llvm dev releases #433

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use official `llvm-project` monorepo, expand LLDB versions check #389

Use official `llvm-project` monorepo, expand LLDB versions check #389

trxcllnt commented Apr 30, 2021 •

edited

Loading

simllll commented Jun 28, 2021

trxcllnt commented Jun 28, 2021 •

edited

Loading

No9 commented Sep 24, 2021

mhdawson left a comment

mhdawson commented Sep 27, 2021

No9 commented Oct 16, 2021

mmarchini commented Oct 16, 2021

No9 commented Nov 13, 2021

No9 commented Nov 15, 2021

No9 commented Nov 16, 2021

trxcllnt commented Nov 18, 2021

codecov-commenter commented Nov 18, 2021 •

edited

Loading

No9 commented Jun 28, 2022

trxcllnt commented Jun 30, 2022

trxcllnt commented Jun 30, 2022

No9 commented Jun 30, 2022

trxcllnt commented Jun 30, 2022

No9 commented Aug 13, 2022

trxcllnt commented Aug 15, 2022

No9 commented Aug 17, 2022

Use official llvm-project monorepo, expand LLDB versions check #389

Use official llvm-project monorepo, expand LLDB versions check #389

Conversation

trxcllnt commented Apr 30, 2021 • edited Loading

simllll commented Jun 28, 2021

trxcllnt commented Jun 28, 2021 • edited Loading

No9 commented Sep 24, 2021

mhdawson left a comment

Choose a reason for hiding this comment

mhdawson commented Sep 27, 2021

No9 commented Oct 16, 2021

mmarchini commented Oct 16, 2021

No9 commented Nov 13, 2021

No9 commented Nov 15, 2021

No9 commented Nov 16, 2021

trxcllnt commented Nov 18, 2021

codecov-commenter commented Nov 18, 2021 • edited Loading

Codecov Report

No9 commented Jun 28, 2022

trxcllnt commented Jun 30, 2022

trxcllnt commented Jun 30, 2022

No9 commented Jun 30, 2022

trxcllnt commented Jun 30, 2022

No9 commented Aug 13, 2022

trxcllnt commented Aug 15, 2022

No9 commented Aug 17, 2022

Use official `llvm-project` monorepo, expand LLDB versions check #389

Use official `llvm-project` monorepo, expand LLDB versions check #389

trxcllnt commented Apr 30, 2021 •

edited

Loading

trxcllnt commented Jun 28, 2021 •

edited

Loading

codecov-commenter commented Nov 18, 2021 •

edited

Loading