FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

tools: add double-quotes to make-v8.sh by Trott · Pull Request #41257 · nodejs/node · GitHub

/ node Public

tools: add double-quotes to make-v8.sh - #41257

Merged
nodejs-github-bot merged 1 commit into
nodejs:masterfrom
Trott:v8-sh
Dec 22, 2021
ConversationCommits (1)ChecksFiles changed
Merged

tools: add double-quotes to make-v8.sh#41257
nodejs-github-bot merged 1 commit into
nodejs:masterfrom
Trott:v8-sh

Conversation

Trott commented Dec 21, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

Now that we maintain make-v8.sh rather than pull it from upstream, we
can make it conform to our shellcheck lint checks.

This is not being flagged in CI because (I'm guessing) shellcheck in CI
is 0.7.0 but latest is 0.8.0.

nodejs-github-bot added needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency. labels Dec 21, 2021

richardlau commented Dec 21, 2021

Copy link
Copy Markdown
Member

Now that we maintain make-v8.sh rather than pull it from upstream, we can make it conform to our shellcheck lint checks.

FYI This file didn’t come from upstream.

richardlau approved these changes Dec 21, 2021
View reviewed changes

richardlau left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks okay. This file is used by the V8 CI job so it would be a good idea to run that with this given previous breakage caused by quoting shell variables in this file (#36099).

Trott commented Dec 21, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

FYI This file didn’t come from upstream.

I based the comment on a (quick) reading of #39222 which suggested to me that it used to be maintained by an upstream project: "The files are not maintained nor used upstream anymore." I guess my interpretation was not quite right.

Trott commented Dec 21, 2021

Copy link
Copy Markdown
Member Author

This file is used by the V8 CI job so it would be a good idea to run that with this

https://ci.nodejs.org/job/node-test-commit-v8-linux/4439/

richardlau commented Dec 21, 2021

Copy link
Copy Markdown
Member

FYI This file didn’t come from upstream.

I based the comment on a (quick) reading of #39222 which suggested to me that it used to be maintained by an upstream project: "The files are not maintained nor used upstream anymore."

That comment is referring to these two files from that PR:

which did use to come from upstream (dropped by https://chromium-review.googlesource.com/c/v8/v8/+/3000966).

tools/make_v8.sh was updated in that PR to point to the new location for fetch_deps.py but it has always been a file that we have maintained in this repository.

tools: add double-quotes to make-v8.sh
d8bf0e3 
This is not being flagged in CI because (I'm guessing) shellcheck in CI
is 0.7.0 but latest is 0.8.0.

Trott commented Dec 21, 2021

Copy link
Copy Markdown
Member Author

it has always been a file that we have maintained in this repository.

Got it. I've removed that sentence from the commit message.

Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 21, 2021

Trott commented Dec 21, 2021

Copy link
Copy Markdown
Member Author

This file is used by the V8 CI job so it would be a good idea to run that with this

https://ci.nodejs.org/job/node-test-commit-v8-linux/4439/

V8 CI is green.

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 21, 2021

nodejs-github-bot commented Dec 21, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/41583/

jasnell added the fast-track PRs that do not need to wait for 48 hours to land. label Dec 21, 2021

github-actions Bot commented Dec 21, 2021

Copy link
Copy Markdown
Contributor

Fast-track has been requested by @jasnell. Please 👍 to approve.

nodejs-github-bot commented Dec 21, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/41590/

Trott added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 21, 2021
github-actions Bot mentioned this pull request Dec 22, 2021
Open
21 tasks
Trott added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 22, 2021
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 22, 2021
nodejs-github-bot merged commit 9e1a080 into nodejs:master Dec 22, 2021

nodejs-github-bot commented Dec 22, 2021

Copy link
Copy Markdown
Collaborator

Landed in 9e1a080

Trott deleted the v8-sh branch December 22, 2021 04:26
This was referenced Dec 23, 2021
Open
Open
Open
Open
Open
Open
Open
github-actions Bot mentioned this pull request Dec 30, 2021
Open
27 tasks
targos pushed a commit that referenced this pull request Jan 14, 2022
tools: add double-quotes to make-v8.sh
e2df381 
This is not being flagged in CI because (I'm guessing) shellcheck in CI
is 0.7.0 but latest is 0.8.0.

PR-URL: #41257
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos mentioned this pull request Jan 16, 2022
Merged
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
tools: add double-quotes to make-v8.sh
12cbded 
This is not being flagged in CI because (I'm guessing) shellcheck in CI
is 0.7.0 but latest is 0.8.0.

PR-URL: #41257
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
tools: add double-quotes to make-v8.sh
1523aa5 
This is not being flagged in CI because (I'm guessing) shellcheck in CI
is 0.7.0 but latest is 0.8.0.

PR-URL: #41257
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams mentioned this pull request Feb 1, 2022
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
lpinca lpinca approved these changes
cjihrig cjihrig approved these changes
richardlau richardlau approved these changes
+1 more reviewer
targos targos approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

fast-track PRs that do not need to wait for 48 hours to land. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL