Skip to content

scripts: installation without sparse-checkout#425

Merged
No9 merged 1 commit into
nodejs:mainfrom
kvakil:fix-ubuntu1804-install
Nov 6, 2022
Merged

scripts: installation without sparse-checkout#425
No9 merged 1 commit into
nodejs:mainfrom
kvakil:fix-ubuntu1804-install

Conversation

@kvakil

@kvakil kvakil commented Oct 1, 2022

Copy link
Copy Markdown
Contributor

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.

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

codecov-commenter commented Oct 1, 2022

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.57%. Comparing base (eb969e1) to head (05ba77a).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #425      +/-   ##
==========================================
- Coverage   73.65%   73.57%   -0.09%     
==========================================
  Files          34       34              
  Lines        4995     4995              
==========================================
- Hits         3679     3675       -4     
- Misses       1316     1320       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
Comment thread scripts/lldb.js
'--filter=blob:none',
'--sparse',
'--branch', lldbHeadersBranch,
'https://github.com/llvm/llvm-project.git',

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe could we put that one into a variable ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hey @tony-go I'm looking to merge this.
Can you be a bit more specific with the point about the variable?
I'm not 100% on what your ask is.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think the ask is to put the Github URL into a variable. I don't feel particularly strongly about it; feel free to do it before merging if you'd like.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

OK this has been around for a while with no updates.
@tony-go I'm also not strongly pushed about changing the implementation.
I'll merge this and please feel free to PR your suggested change if you want.

@No9 No9 merged commit de1f01d into nodejs:main Nov 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants