From 84b6c629d3ed0aff3dea7d3e0c2fdb50fc46d8ab Mon Sep 17 00:00:00 2001 From: driazati Date: Fri, 21 May 2021 18:21:57 -0700 Subject: [PATCH] [lint] Move shellcheck to its own step (#58623) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 4 +- .github/workflows/lint.yml | 55 ++++++++++++++----------- Makefile | 38 +++++++++++------ tools/actions_local_runner.py | 41 +++++++++++++++++- tools/test/test_actions_local_runner.py | 22 +++++++--- 5 files changed, 115 insertions(+), 45 deletions(-) diff --git a/.github/scripts/report_git_status.sh b/.github/scripts/report_git_status.sh index 357bacfecb24..738fbcfd1451 100755 --- a/.github/scripts/report_git_status.sh +++ b/.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" ] diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index d6928c82bc79..5151adc0c3b9 100644 --- a/.github/workflows/lint.yml +++ b/.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 diff --git a/Makefile b/Makefile index 320e77cba27b..f328d07b2756 100644 --- a/Makefile +++ b/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 diff --git a/tools/actions_local_runner.py b/tools/actions_local_runner.py index e0bbab5f3100..4082abda527b 100755 --- a/tools/actions_local_runner.py +++ b/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__": diff --git a/tools/test/test_actions_local_runner.py b/tools/test/test_actions_local_runner.py index 874e7c5bbe6a..2b2c26dbc378 100644 --- a/tools/test/test_actions_local_runner.py +++ b/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