[lint] Move shellcheck to its own step (#58623)

Summary:
Pull Request resolved: https://github.com/pytorch/pytorch/pull/58623

This splits out everything shellcheck related into its own job that generates and checks GHA workflows, then shellchecks those + jenkins scripts. This PR also integrates shellcheck into the changed-only stuff in `actions_local_runner.py` so that shellcheck won't do anything unless someone edits a shell script in their local checkout. This is the final piece to clean up the output of `make quicklint` and speeds it up by a good bit (before it was shellchecking everything which took a few seconds):

```
$ make quicklint -j $(nproc)
✓ quick-checks: Ensure no unqualified noqa
✓ quick-checks: Ensure canonical include
✓ quick-checks: Ensure no unqualified type ignore
✓ quick-checks: Ensure no direct cub include
✓ quick-checks: Ensure no tabs
✓ quick-checks: Ensure no non-breaking spaces
✓ shellcheck: Regenerate workflows
✓ quick-checks: Ensure no versionless Python shebangs
✓ quick-checks: Ensure correct trailing newlines
✓ shellcheck: Assert that regenerating the workflows didn't change them
✓ mypy (skipped typestub generation)
✓ cmakelint: Run cmakelint
✓ quick-checks: Ensure no trailing spaces
✓ flake8
✓ shellcheck: Extract scripts from GitHub Actions workflows
✓ shellcheck: Run Shellcheck
real 0.92
user 6.12
sys 2.45
```

Test Plan: Imported from OSS

Reviewed By: nikithamalgifb

Differential Revision: D28617293

Pulled By: driazati

fbshipit-source-id: af960ed441db797d07697bfb8292aff5010ca45b

.github/scripts/report_git_status.sh

@@ -1,5 +1,5 @@
 #!/usr/bin/env bash
-CHANGES=$(git status --porcelain)
+CHANGES=$(git status --porcelain "$1")
 echo "$CHANGES"
-git diff
+git diff "$1"
 [ -z "$CHANGES" ]

.github/workflows/lint.yml

@@ -35,27 +35,6 @@ jobs:
         run: |
           pip install ruamel.yaml==0.17.4
           .github/scripts/lint_native_functions.py
-      - name: Extract scripts from GitHub Actions workflows
-        if: always() && steps.requirements.outcome == 'success'
-        run: |
-          # For local lints, remove the .extracted_scripts folder if it was already there
-          rm -rf .extracted_scripts
-          tools/extract_scripts.py --out=.extracted_scripts
-      - name: Install ShellCheck
-        id: install_shellcheck
-        if: always()
-        # https://github.com/koalaman/shellcheck/tree/v0.7.2#installing-a-pre-compiled-binary
-        run: |
-          set -x
-          scversion="v0.7.2"
-          wget -qO- "https://github.com/koalaman/shellcheck/releases/download/${scversion?}/shellcheck-${scversion?}.linux.x86_64.tar.xz" | tar -xJv
-          sudo cp "shellcheck-${scversion}/shellcheck" /usr/bin/
-          rm -r "shellcheck-${scversion}"
-          shellcheck --version
-      - name: Run ShellCheck
-        if: always() && steps.install_shellcheck.outcome == 'success'
-        run: |
-          tools/run_shellcheck.sh .jenkins/pytorch .extracted_scripts
       - name: Ensure correct trailing newlines
         if: always() && steps.requirements.outcome == 'success'
         run: |
@@ -129,7 +108,7 @@ jobs:
         run: |
           python2 setup.py | grep "Python 2 has reached end-of-life and is no longer supported by PyTorch."
 
-  templates:
+  shellcheck:
     runs-on: ubuntu-18.04
     steps:
       - name: Setup Python
@@ -137,14 +116,42 @@ jobs:
         with:
           python-version: 3.x
           architecture: x64
+      - name: Checkout PyTorch
+        uses: actions/checkout@v2
+      - name: Install requirements
+        id: requirements
+        run: |
+          pip install -r requirements.txt
       - name: Install Jinja2
-        run: pip install Jinja2
+        run: |
+          pip install Jinja2==3.0.1
       - name: Checkout PyTorch
         uses: actions/checkout@v2
       - name: Regenerate workflows
         run: .github/scripts/generate_ci_workflows.py
       - name: Assert that regenerating the workflows didn't change them
-        run: .github/scripts/report_git_status.sh
+        run: .github/scripts/report_git_status.sh .github/workflows
+      - name: Install ShellCheck
+        id: install_shellcheck
+        if: always()
+        # https://github.com/koalaman/shellcheck/tree/v0.7.2#installing-a-pre-compiled-binary
+        run: |
+          set -x
+          scversion="v0.7.2"
+          wget -qO- "https://github.com/koalaman/shellcheck/releases/download/${scversion?}/shellcheck-${scversion?}.linux.x86_64.tar.xz" | tar -xJv
+          sudo cp "shellcheck-${scversion}/shellcheck" /usr/bin/
+          rm -r "shellcheck-${scversion}"
+          shellcheck --version
+      - name: Extract scripts from GitHub Actions workflows
+        if: always() && steps.install_shellcheck.outcome == 'success'
+        run: |
+          # For local lints, remove the .extracted_scripts folder if it was already there
+          rm -rf .extracted_scripts
+          tools/extract_scripts.py --out=.extracted_scripts
+      - name: Run ShellCheck
+        if: always() && steps.install_shellcheck.outcome == 'success'
+        run: |
+          tools/run_shellcheck.sh .extracted_scripts .jenkins/pytorch
 
   toc:
     runs-on: ubuntu-18.04

Makefile

@@ -32,13 +32,33 @@ generate-gha-workflows:
 	.github/scripts/generate_ci_workflows.py
 	$(MAKE) shellcheck-gha
 
+shellcheck:
+	@$(PYTHON) tools/actions_local_runner.py \
+		--file .github/workflows/lint.yml \
+		--job 'shellcheck' \
+		--step "Regenerate workflows"
+	@$(PYTHON) tools/actions_local_runner.py \
+		--file .github/workflows/lint.yml \
+		--job 'shellcheck' \
+		--step "Assert that regenerating the workflows didn't change them"
+	@$(PYTHON) tools/actions_local_runner.py \
+		--file .github/workflows/lint.yml \
+		--job 'shellcheck' \
+		--step 'Extract scripts from GitHub Actions workflows'
+	@$(PYTHON) tools/actions_local_runner.py \
+		--file-filter '.sh' \
+		$(CHANGED_ONLY) \
+		--job 'shellcheck'
+
 setup_lint:
 	$(PYTHON) tools/actions_local_runner.py --file .github/workflows/lint.yml \
-	 	--job 'flake8-py3' --step 'Install dependencies' --no-quiet
+		--job 'flake8-py3' --step 'Install dependencies' --no-quiet
 	$(PYTHON) tools/actions_local_runner.py --file .github/workflows/lint.yml \
-	 	--job 'cmakelint' --step 'Install dependencies' --no-quiet
+		--job 'cmakelint' --step 'Install dependencies' --no-quiet
 	$(PYTHON) tools/actions_local_runner.py --file .github/workflows/lint.yml \
-	 	--job 'mypy' --step 'Install dependencies' --no-quiet
+		--job 'mypy' --step 'Install dependencies' --no-quiet
+	$(PYTHON) tools/actions_local_runner.py --file .github/workflows/lint.yml \
+		--job 'shellcheck' --step 'Install Jinja2' --no-quiet
 
 	@if [ "$$(uname)" = "Darwin" ]; then \
 		if [ -z "$$(which brew)" ]; then \
@@ -48,16 +68,11 @@ setup_lint:
 		brew install shellcheck; \
 	else \
 		$(PYTHON) tools/actions_local_runner.py --file .github/workflows/lint.yml \
-		--job 'quick-checks' --step 'Install ShellCheck' --no-quiet; \
+		--job 'shellcheck' --step 'Install ShellCheck' --no-quiet; \
 	fi
 	pip install jinja2
 
 quick_checks:
-	@$(PYTHON) tools/actions_local_runner.py \
-		--file .github/workflows/lint.yml \
-		--job 'quick-checks' \
-		--step 'Extract scripts from GitHub Actions workflows'
-
 # TODO: This is broken when 'git config submodule.recurse' is 'true' since the
 # lints will descend into third_party submodules
 	@$(PYTHON) tools/actions_local_runner.py \
@@ -71,7 +86,6 @@ quick_checks:
 		--step 'Ensure no unqualified noqa' \
 		--step 'Ensure no unqualified type ignore' \
 		--step 'Ensure no direct cub include' \
-		--step 'Run ShellCheck' \
 		--step 'Ensure correct trailing newlines'
 
 flake8:
@@ -102,7 +116,7 @@ toc:
 		--job 'toc' \
 		--step "Regenerate ToCs and check that they didn't change"
 
-lint: flake8 mypy quick_checks cmakelint generate-gha-workflows
+lint: flake8 mypy quick_checks cmakelint shellcheck
 
 quicklint: CHANGED_ONLY=--changed-only
-quicklint: mypy flake8 mypy quick_checks cmakelint generate-gha-workflows
+quicklint: mypy flake8 mypy quick_checks cmakelint shellcheck

tools/actions_local_runner.py

@@ -223,6 +223,39 @@ async def run_mypy(files: Optional[List[str]], quiet: bool) -> bool:
     return passed
 
 
+
+async def run_shellcheck(files: Optional[List[str]], quiet: bool) -> bool:
+    if files is not None:
+        # The files list should already be filtered by '--file-filter ".sh"' when
+        # calling this script
+        passed, stdout, stderr = await shell_cmd(
+            ["tools/run_shellcheck.sh"] + [
+                os.path.join(REPO_ROOT, f) for f in files
+            ],
+        )
+        print_results("shellcheck: Run ShellCheck", passed, [
+            stdout + "\n",
+            stderr + "\n",
+        ])
+        return passed
+
+    # Not running quicklint, so use lint.yml
+    passed, _, _ = await shell_cmd(
+        [
+            sys.executable,
+            "tools/actions_local_runner.py",
+            "--job",
+            "shellcheck",
+            "--file",
+            ".github/workflows/lint.yml",
+            "--step",
+            "Run ShellCheck",
+        ],
+        redirect=False,
+    )
+    return passed
+
+
 async def run_step(
     step: Dict[str, Any], job_name: str, files: Optional[List[str]], quiet: bool
 ) -> bool:
@@ -238,7 +271,10 @@ async def run_step(
     name = f'{job_name}: {step["name"]}'
 
     passed, stderr, stdout = await shell_cmd(script, env=env)
-    print_results(name, passed, [stdout, stderr])
+    if not passed:
+        print_results(name, passed, [stdout, stderr])
+    else:
+        print_results(name, passed, [])
 
     return passed
 
@@ -285,7 +321,7 @@ def grab_specific_steps(
                 break
 
     if len(relevant_steps) != len(steps_to_grab):
-        raise RuntimeError("Missing steps")
+        raise RuntimeError(f"Missing steps:\n{relevant_steps}\n{steps_to_grab}")
 
     return relevant_steps
 
@@ -355,6 +391,7 @@ def main() -> None:
 ad_hoc_steps = {
     "mypy": run_mypy,
     "flake8-py3": run_flake8,
+    "shellcheck": run_shellcheck,
 }
 
 if __name__ == "__main__":

tools/test/test_actions_local_runner.py

@@ -59,11 +59,9 @@ if sys.version_info >= (3, 8):
 
             result = f.getvalue()
             self.assertIn("say hello", result)
-            self.assertIn("hi", result)
 
     class TestEndToEnd(unittest.TestCase):
         expected = [
-            "quick-checks: Extract scripts from GitHub Actions workflows",
             "cmakelint: Run cmakelint",
             "quick-checks: Ensure no direct cub include",
             "quick-checks: Ensure no unqualified type ignore",
@@ -74,7 +72,10 @@ if sys.version_info >= (3, 8):
             "flake8",
             "quick-checks: Ensure correct trailing newlines",
             "quick-checks: Ensure no trailing spaces",
-            "quick-checks: Run ShellCheck",
+            "shellcheck: Regenerate workflows",
+            "shellcheck: Assert that regenerating the workflows didn't change them",
+            "shellcheck: Extract scripts from GitHub Actions workflows",
+            "shellcheck: Run ShellCheck",
         ]
 
         def test_lint(self):
@@ -105,7 +106,10 @@ if sys.version_info >= (3, 8):
             os.path.join("torch", "some_cool_file.py"),
             os.path.join("aten", "some_cool_file.py"),
             os.path.join("torch", "some_stubs.pyi"),
+            os.path.join("test.sh"),
         ]
+        test_py_files = [f for f in test_files if f.endswith(".py") or f.endswith(".pyi")]
+        test_sh_files = [f for f in test_files if f.endswith(".sh")]
         maxDiff = None
 
         def setUp(self, *args, **kwargs):
@@ -131,7 +135,7 @@ if sys.version_info >= (3, 8):
         async def test_flake8(self):
             f = io.StringIO()
             with contextlib.redirect_stdout(f):
-                await actions_local_runner.run_flake8(self.test_files, True)
+                await actions_local_runner.run_flake8(self.test_py_files, True)
 
             # Should exclude the caffe2/ file
             expected = textwrap.dedent("""
@@ -141,6 +145,14 @@ if sys.version_info >= (3, 8):
             """).lstrip("\n")
             self.assertEqual(expected, f.getvalue())
 
+        async def test_shellcheck(self):
+            f = io.StringIO()
+            with contextlib.redirect_stdout(f):
+                await actions_local_runner.run_shellcheck(self.test_sh_files, True)
+
+            self.assertIn("SC2148: Tips depend on target shell", f.getvalue())
+            self.assertIn("SC2283: Remove spaces around = to assign", f.getvalue())
+
         async def test_mypy(self):
             self.maxDiff = None
             f = io.StringIO()
@@ -161,7 +173,7 @@ if sys.version_info >= (3, 8):
                     redirect=True,
                 )
 
-                await actions_local_runner.run_mypy(self.test_files, True)
+                await actions_local_runner.run_mypy(self.test_py_files, True)
 
 
             # Should exclude the aten/ file; also, apparently mypy