tools: lint shell scripts #36099

aduh95 · 2020-11-12T17:54:52Z

This PR introduces ShellCheck to Node.js core.

Usage: tools/lint-sh.js <path> [--fix]

This does not vendor ShellCheck, instead it:

checks the environment for a SHELLCHECK variable.
runs command -v shellcheck in case it has been installed in the PATH.
fallbacks to npx shellcheck (https://npmjs.org/shellcheck) to download it from the internet

On top of ShellChecks checks, it also enforces #!/bin/sh at the top of the file. There were a few scripts that were using either #!/usr/bin/env bash or #!/bin/bash, but it's easier to have all of them using the same shell. I've also open several PRs that make changes to files that were using bashisms and which should land before this PR.

Reasons to prefer sh over bash (copied from a StackOverflow answer):

it is standardized
it is much simpler and easier to learn
it is portable across POSIX systems — even if they happen not to have bash, they are required to have sh

This PR also adds a new GH Actions in linters.yml, and fixes the failing checks on the existing .sh files.

Blocked on:

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

targos · 2020-11-13T09:44:35Z

~~Is there a reason for not using an existing tool like ShellCheck ?~~

Edit: sorry, I didn't pay good enough attention to what your script was doing. There's already a GitHub action in the marketplace, though (I don't know what it's worth): https://github.com/ludeeus/action-shellcheck

aduh95 · 2020-11-13T09:46:38Z

Is there a reason for not using an existing tool like ShellCheck ?

@targos this PR is precisely using ShellCheck ^^ It's just a wrapper script to find all the .sh files and it passes it on to shellcheck.

Edit: sorry, I didn't pay good enough attention to what your script was doing. There's already a GitHub action in the marketplace, though (I don't know what it's worth): https://github.com/ludeeus/action-shellcheck

Yes, I think we can get more value by having our own script so contributors can run the same test at home. Also I've added a --fix option.

tools/lint-sh.js

RaisinTen

I read here that it is better to avoid which and use command -v instead: https://stackoverflow.com/a/677212
🙂

.github/workflows/linters.yml

aduh95 · 2020-11-14T22:18:35Z

@Trott I've tried to add a fallback to use npx when shellcheck is not found on PATH, but I'm getting an ENOENT error. I'm guessing I did something wrong, if you want to take a look on how I did it, that would be awesome.

npm ERR! code ENOENT
npm ERR! syscall open
npm ERR! path …/node/package.json
npm ERR! errno -2
npm ERR! enoent ENOENT: no such file or directory, open '…/node/package.json'
npm ERR! enoent This is related to npm not being able to find a file.
npm ERR! enoent

Trott · 2020-11-17T20:53:16Z

@Trott I've tried to add a fallback to use npx when shellcheck is not found on PATH, but I'm getting an ENOENT error. I'm guessing I did something wrong, if you want to take a look on how I did it, that would be awesome.
npm ERR! code ENOENT
npm ERR! syscall open
npm ERR! path …/node/package.json
npm ERR! errno -2
npm ERR! enoent ENOENT: no such file or directory, open '…/node/package.json'
npm ERR! enoent This is related to npm not being able to find a file.
npm ERR! enoent 

Are you getting that locally? I'm running your tools/lint-sh.js locally and it's falling back to npx shellcheck just fine.

Trott · 2020-11-17T20:53:37Z

@Trott I've tried to add a fallback to use npx when shellcheck is not found on PATH, but I'm getting an ENOENT error. I'm guessing I did something wrong, if you want to take a look on how I did it, that would be awesome.
npm ERR! code ENOENT
npm ERR! syscall open
npm ERR! path …/node/package.json
npm ERR! errno -2
npm ERR! enoent ENOENT: no such file or directory, open '…/node/package.json'
npm ERR! enoent This is related to npm not being able to find a file.
npm ERR! enoent 
Are you getting that locally? I'm running your tools/lint-sh.js locally and it's falling back to npx shellcheck just fine.

By the way, totally OK with me to land this without npx fallback if it comes to that.

aduh95 · 2020-11-17T21:05:16Z

I'm running your tools/lint-sh.js locally and it's falling back to npx shellcheck just fine.

Ah it must be a problem with my installation. Good news :)

aduh95 · 2020-12-04T23:07:17Z

@nodejs/linting This is ready for review if you want to have a look.

aduh95 · 2020-12-07T18:54:06Z

Ping @nodejs/linting.

tools/lint-sh.js

nodejs-github-bot · 2020-12-15T12:59:23Z

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

github-actions · 2020-12-15T17:41:36Z

Commit Queue failed

- Loading data for nodejs/node/pull/36099
✔  Done loading data for nodejs/node/pull/36099
----------------------------------- PR info ------------------------------------
Title      tools: lint shell scripts (#36099)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     aduh95:lint-sh -> nodejs:master
Labels     author ready, meta, tools
Commits    4
 - tools: lint shell scripts
 - fixup! tools: lint shell scripts
 - fixup! tools: lint shell scripts
 - fixup! tools: lint shell scripts
Committers 1
 - Antoine du Hamel 
PR-URL: https://github.com/nodejs/node/pull/36099
Reviewed-By: Rich Trott 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/36099
Reviewed-By: Rich Trott 
--------------------------------------------------------------------------------
   ✔  Last GitHub Actions successful
   ℹ  Last Full PR CI on 2020-12-15T12:59:23Z: https://ci.nodejs.org/job/node-test-pull-request/34949/
- Querying data for job/node-test-pull-request/34949/
✔  Build data downloaded
- Querying failures of job/node-test-commit/42962/
✔  Data downloaded
   ✖  2 failure(s) on the last Jenkins CI run
   ℹ  This PR was created on Thu, 12 Nov 2020 17:54:52 GMT
   ✔  Approvals: 1
   ✔  - Rich Trott (@Trott) (TSC): https://github.com/nodejs/node/pull/36099#pullrequestreview-552383843
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/423801608

nodejs-github-bot · 2020-12-15T18:44:47Z

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

PR-URL: nodejs#36099 Reviewed-By: Rich Trott <rtrott@gmail.com>

aduh95 · 2020-12-16T22:46:08Z

Landed in 0e96dc1

PR-URL: #36099 Reviewed-By: Rich Trott <rtrott@gmail.com>

V8's `tools/dev/v8gen.py` does not like being passed an empty string (`""`). PR-URL: #36594 Refs: #36099 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Coe <bencoe@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com>

PR-URL: #36099 Reviewed-By: Rich Trott <rtrott@gmail.com>

V8's `tools/dev/v8gen.py` does not like being passed an empty string (`""`). PR-URL: #36594 Refs: #36099 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Coe <bencoe@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com>

nodejs-github-bot added meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory. labels Nov 12, 2020

aduh95 marked this pull request as draft November 12, 2020 17:55

richardlau reviewed Nov 13, 2020

View reviewed changes

tools/lint-sh.js Outdated Show resolved Hide resolved

silverwind reviewed Nov 13, 2020

View reviewed changes

tools/lint-sh.js Outdated Show resolved Hide resolved

RaisinTen reviewed Nov 14, 2020

View reviewed changes

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

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

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

This was referenced Nov 14, 2020

tools: remove bashisms from macOS release scripts #36121

Closed

tools: remove bashisms from license builder script #36122

Closed

tools: remove bashisms from release script #36123

Closed

aduh95 force-pushed the lint-sh branch from 1f2bbf6 to 8608354 Compare December 4, 2020 23:06

aduh95 marked this pull request as ready for review December 4, 2020 23:06

aduh95 force-pushed the lint-sh branch from 8608354 to 93ba049 Compare December 7, 2020 18:15

Trott reviewed Dec 10, 2020

View reviewed changes

tools/lint-sh.js Outdated Show resolved Hide resolved

Trott approved these changes Dec 15, 2020

View reviewed changes

aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. request-ci Add this label to start a Jenkins CI on a PR. labels Dec 15, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 15, 2020

github-actions bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 15, 2020

aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Dec 15, 2020

github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 15, 2020

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 16, 2020

github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 16, 2020

aduh95 added a commit to aduh95/node that referenced this pull request Dec 16, 2020

tools: lint shell scripts

0e96dc1

PR-URL: nodejs#36099 Reviewed-By: Rich Trott <rtrott@gmail.com>

aduh95 closed this Dec 16, 2020

aduh95 force-pushed the lint-sh branch from 98a7426 to 0e96dc1 Compare December 16, 2020 22:46

aduh95 deleted the lint-sh branch December 16, 2020 22:46

targos pushed a commit that referenced this pull request Dec 21, 2020

tools: lint shell scripts

7b8d373

PR-URL: #36099 Reviewed-By: Rich Trott <rtrott@gmail.com>

richardlau mentioned this pull request Dec 21, 2020

tools: fix make-v8.sh #36594

Closed

2 tasks

targos mentioned this pull request Dec 22, 2020

v15.5.0 release proposal #36597

Merged

targos pushed a commit that referenced this pull request May 1, 2021

tools: lint shell scripts

9b13ddc

PR-URL: #36099 Reviewed-By: Rich Trott <rtrott@gmail.com>

danielleadams mentioned this pull request May 3, 2021

v14.17.0 proposal #38507

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tools: lint shell scripts #36099

tools: lint shell scripts #36099

aduh95 commented Nov 12, 2020 •

edited

targos commented Nov 13, 2020 •

edited

aduh95 commented Nov 13, 2020 •

edited

RaisinTen left a comment

aduh95 commented Nov 14, 2020

Trott commented Nov 17, 2020

Trott commented Nov 17, 2020

aduh95 commented Nov 17, 2020

aduh95 commented Dec 4, 2020

aduh95 commented Dec 7, 2020

nodejs-github-bot commented Dec 15, 2020

github-actions bot commented Dec 15, 2020

nodejs-github-bot commented Dec 15, 2020

aduh95 commented Dec 16, 2020

tools: lint shell scripts #36099

tools: lint shell scripts #36099

Conversation

aduh95 commented Nov 12, 2020 • edited

Checklist

targos commented Nov 13, 2020 • edited

aduh95 commented Nov 13, 2020 • edited

RaisinTen left a comment

Choose a reason for hiding this comment

aduh95 commented Nov 14, 2020

Trott commented Nov 17, 2020

Trott commented Nov 17, 2020

aduh95 commented Nov 17, 2020

aduh95 commented Dec 4, 2020

aduh95 commented Dec 7, 2020

nodejs-github-bot commented Dec 15, 2020

github-actions bot commented Dec 15, 2020

nodejs-github-bot commented Dec 15, 2020

aduh95 commented Dec 16, 2020

aduh95 commented Nov 12, 2020 •

edited

targos commented Nov 13, 2020 •

edited

aduh95 commented Nov 13, 2020 •

edited