Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tools: lint shell scripts #36099

Closed
wants to merge 0 commits into from
Closed

tools: lint shell scripts #36099

wants to merge 0 commits into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Nov 12, 2020

This PR introduces ShellCheck to Node.js core.

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

This does not vendor ShellCheck, instead it:

  1. checks the environment for a SHELLCHECK variable.
  2. runs command -v shellcheck in case it has been installed in the PATH.
  3. 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

@nodejs-github-bot 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 aduh95 marked this pull request as draft November 12, 2020 17:55
@targos
Copy link
Member

targos commented Nov 13, 2020

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
Copy link
Contributor Author

aduh95 commented Nov 13, 2020

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 Outdated Show resolved Hide resolved
tools/lint-sh.js Outdated Show resolved Hide resolved
Copy link
Contributor

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

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

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

.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
@aduh95
Copy link
Contributor Author

aduh95 commented Nov 14, 2020

@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
Copy link
Member

Trott commented Nov 17, 2020

@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
Copy link
Member

Trott commented Nov 17, 2020

@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
Copy link
Contributor Author

aduh95 commented Nov 17, 2020

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 aduh95 marked this pull request as ready for review December 4, 2020 23:06
@aduh95
Copy link
Contributor Author

aduh95 commented Dec 4, 2020

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

@aduh95
Copy link
Contributor Author

aduh95 commented Dec 7, 2020

Ping @nodejs/linting.

tools/lint-sh.js Outdated Show resolved Hide resolved
@aduh95 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 github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 15, 2020
@nodejs-github-bot
Copy link
Collaborator

@github-actions 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
@github-actions
Copy link
Contributor

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
Copy link
Collaborator

@aduh95 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 github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 15, 2020
@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 16, 2020
@github-actions 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
PR-URL: nodejs#36099
Reviewed-By: Rich Trott <rtrott@gmail.com>
@aduh95 aduh95 closed this Dec 16, 2020
@aduh95
Copy link
Contributor Author

aduh95 commented Dec 16, 2020

Landed in 0e96dc1

@aduh95 aduh95 deleted the lint-sh branch December 16, 2020 22:46
targos pushed a commit that referenced this pull request Dec 21, 2020
PR-URL: #36099
Reviewed-By: Rich Trott <rtrott@gmail.com>
@richardlau richardlau mentioned this pull request Dec 21, 2020
2 tasks
nodejs-github-bot pushed a commit that referenced this pull request Dec 22, 2020
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>
targos pushed a commit that referenced this pull request Dec 22, 2020
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>
targos pushed a commit that referenced this pull request May 1, 2021
PR-URL: #36099
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request May 1, 2021
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>
@danielleadams danielleadams mentioned this pull request May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants