Page MenuHomePhabricator

[Chore] Make shellcheck happy
ClosedPublic

Authored by jon on Aug 11 2022, 5:04 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Nov 3, 1:49 PM
Unknown Object (File)
Wed, Oct 23, 7:14 AM
Unknown Object (File)
Mon, Oct 7, 11:30 PM
Unknown Object (File)
Mon, Oct 7, 11:30 PM
Unknown Object (File)
Mon, Oct 7, 11:29 PM
Unknown Object (File)
Mon, Oct 7, 11:25 PM
Unknown Object (File)
Mon, Oct 7, 12:34 AM
Unknown Object (File)
Mon, Oct 7, 12:34 AM

Details

Summary

Shellcheck is failing on GitHub

Feel like I should have known this, as we use command -v in nixpkgs as well. Guess I just overlooked it because I wanted to follow the upstream patch as much as possible.

Test Plan

shellcheck CI

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Aug 11 2022, 5:18 PM
Harbormaster failed remote builds in B11327: Diff 15586!
abosh added a reviewer: atul.

Actually just saw this on GitHub, was going to send you a message about it, and saw this diff! Thanks for catching this.

For the sake of clarity, ShellCheck was complaining that which is non-standard. Use builtin command -v instead. The wiki also notes:

This is an optional suggestion. It must be explicitly enabled with a directive enable=deprecate-which in a # shellcheck comment or .shellcheckrc

I did not know which was non-standard, but it makes sense since its intended to be a human-readable external tool that uses each user's PATH. Not really familiar with command -v since I always use which, but I read these two resources and it made sense to me:

Also you should probably add @atul as reviewer for all CI/GitHub Actions related things.

This revision is now accepted and ready to land.Aug 12 2022, 8:00 AM

What's more confusing to me is why this passed through lint-staged and Buildkite, but failed on GitHub Actions. They should be running the same shellcheck -x -P SCRIPTDIR [FILES...] command...

This revision now requires review to proceed.Aug 12 2022, 8:01 AM

Ah, investigated with @atul and figured out why this is happening. ShellCheck on lint-staged is installed by homebrew (locally, by developers), ShellCheck on Buildkite is installed using apt-get, and ShellCheck on GitHub Actions is preinstalled using ubuntu-latest. Because this check of using command -v over which is optional in versions of ShellCheck 0.7.1+ (which the homebrew and apt-get installations of ShellCheck are), this was not caught by those CI builds, but was caught by GitHub Actions since the version of ShellCheck on ubuntu-latest is 0.7.0, which is < 0.7.1. To fix this, we're bumping the ShellCheck job on GitHub Actions to run off ubuntu-22.04 instead of ubuntu-latest (which is 20.04). This is because the version of ShellCheck that comes with ubuntu-22.04 is 0.8.0-2, which is 0.7.1+.

Also, we could install ShellCheck through apt-get on GitHub Actions to keep it consistent with Buildkite. To do this, we'd just have to revert D4756. Edit: it's been done, see D4828.

More context from the ShellCheck wiki for this error:

This check is opt-in only in 0.7.1+, and you may choose to ignore it in earlier versions. which is very common, and some prefer its executable-or-nothing behavior over command -v's handling of builtins, functions and aliases.

I did not know which was non-standard

It's usually a small bash script that a distro provides. There's not a canonical "unix which"

This revision is now accepted and ready to land.Aug 12 2022, 9:39 AM
This revision was automatically updated to reflect the committed changes.