servo: Merge #12736 - Add more shell script lints (from aneeshusa:prohibit-backticks-in-shell-scripts); r=Wafflespeanut
authorAneesh Agrawal <aneeshusa@gmail.com>
Sat, 06 Aug 2016 21:57:31 -0500
changeset 339449 324e067acd6c7a79a24779001920d42bbe88c9ea
parent 339448 9603ffe5b58e52359fd6bada0f914b5ad0c04155
child 339450 3478aa8b5033c884bdb72670183941a31e3833dd
push id31307
push usergszorc@mozilla.com
push dateSat, 04 Feb 2017 00:59:06 +0000
treeherdermozilla-central@94079d43835f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersWafflespeanut
servo: Merge #12736 - Add more shell script lints (from aneeshusa:prohibit-backticks-in-shell-scripts); r=Wafflespeanut <!-- Please describe your changes on the following line: --> The "$(some_command arg1 arg2)" form is preferred to the `some_command arg1 arg2` form because it nests unambiguously. Add a lint for this to tidy. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable). <!-- Either: --> - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> Source-Repo: https://github.com/servo/servo Source-Revision: aa900b91aa9417b6aa032b4eff878f2916c480f0
servo/components/servo/fake-ld.sh
servo/etc/ci/lockfile_changed.sh
servo/etc/ci/manifest_changed.sh
servo/etc/ci/upload_docs.sh
servo/etc/ci/upload_nightly.sh
servo/ports/geckolib/gecko_bindings/tools/regen.sh
servo/ports/geckolib/gecko_bindings/tools/setup_bindgen.sh
servo/python/tidy/servo_tidy/tidy.py
servo/python/tidy/servo_tidy_tests/shell_tidy.sh
servo/python/tidy/servo_tidy_tests/test_tidy.py
--- a/servo/components/servo/fake-ld.sh
+++ b/servo/components/servo/fake-ld.sh
@@ -4,12 +4,12 @@
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 set -o errexit
 set -o nounset
 set -o pipefail
 
 TARGET_DIR="${OUT_DIR}/../../.."
-arm-linux-androideabi-gcc "$@" \
+arm-linux-androideabi-gcc "${@}" \
                           "${LDFLAGS-}" -lc -shared \
                           -o "${TARGET_DIR}/libservo.so"
 touch "${TARGET_DIR}/servo"
--- a/servo/etc/ci/lockfile_changed.sh
+++ b/servo/etc/ci/lockfile_changed.sh
@@ -4,10 +4,10 @@
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 set -o errexit
 set -o nounset
 set -o pipefail
 
 diff="$(git diff -- */*/Cargo.lock)"
-echo "$diff"
-[[ ! $diff ]]
+echo "${diff}"
+[[ -z "${diff}" ]]
--- a/servo/etc/ci/manifest_changed.sh
+++ b/servo/etc/ci/manifest_changed.sh
@@ -12,10 +12,10 @@ set -o pipefail
 # Adding "SKIP_TESTS" to skip tests, it doesn't really skip the tests.
 # It will run "run_wpt" with "'test_list': ['SKIP_TESTS']",
 # and then pass it into wptrunner, which won't be able to find any tests named
 # "SKIP_TESTS", and thus won't run any.
 # Adding "--binary=" to skip looking for a compiled servo binary.
 ./mach test-wpt --manifest-update --binary= SKIP_TESTS > /dev/null
 
 diff="$(git diff -- tests/*/MANIFEST.json)"
-echo "$diff"
-[[ ! $diff ]]
+echo "${diff}"
+[[ -z "${diff}" ]]
--- a/servo/etc/ci/upload_docs.sh
+++ b/servo/etc/ci/upload_docs.sh
@@ -7,17 +7,17 @@
 # Helper script to upload docs to doc.servo.org.
 # Requires ghp-import (from pip)
 # GitHub API token must be passed in environment var TOKEN
 
 set -o errexit
 set -o nounset
 set -o pipefail
 
-cd "$(dirname $0)/../.."
+cd "$(dirname ${0})/../.."
 
 ./mach doc
 # etc/doc.servo.org/index.html overwrites $(mach rust-root)/doc/index.html
 cp etc/doc.servo.org/* target/doc/
 
 python components/style/properties/build.py servo html
 
 OUT_DIR="$(pwd)/target/doc/servo" \
--- a/servo/etc/ci/upload_nightly.sh
+++ b/servo/etc/ci/upload_nightly.sh
@@ -22,17 +22,17 @@ upload() {
     local -r package_upload_path="${nightly_upload_dir}/${package_filename}"
     s3cmd --mime-type="application/octet-stream" \
           put "${2}" "${package_upload_path}"
     s3cmd cp "${package_upload_path}" "${nightly_upload_dir}/servo-latest.${3}"
 }
 
 
 main() {
-    if [[ "$#" != 1 ]]; then
+    if [[ "${#}" != 1 ]]; then
         usage >&2
         return 1
     fi
 
     local platform package extension
     platform="${1}"
 
     if [[ "${platform}" == "android" ]]; then
@@ -53,9 +53,9 @@ main() {
     fi
 
     # Lack of quotes on package allows glob expansion
     # Note that this is not robust in the case of embedded spaces
     # TODO(aneeshusa): make this glob robust using e.g. arrays or Python
     upload "${platform}" ${package} "${extension}"
 }
 
-main "$@"
+main "${@}"
--- a/servo/ports/geckolib/gecko_bindings/tools/regen.sh
+++ b/servo/ports/geckolib/gecko_bindings/tools/regen.sh
@@ -3,33 +3,33 @@
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 set -o errexit
 set -o nounset
 set -o pipefail
 
-if [ $# -eq 0 ]; then
-  echo "Usage: $0 /path/to/gecko/objdir [other-regen.py-flags]"
+if [[ ${#} -eq 0 ]]; then
+  echo "Usage: ${0} /path/to/gecko/objdir [other-regen.py-flags]"
   exit 1
 fi
 
 # Check for rust-bindgen
-if [ ! -d rust-bindgen ]; then
+if [[ ! -d rust-bindgen ]]; then
   echo "rust-bindgen not found. Run setup_bindgen.sh first."
   exit 1
 fi
 
 # Check for /usr/include
-if [ ! -d /usr/include ]; then
+if [[ ! -d /usr/include ]]; then
   echo "/usr/include doesn't exist." \
        "Mac users may need to run xcode-select --install."
   exit 1
 fi
 
-if [ "$(uname)" == "Linux" ]; then
-  LIBCLANG_PATH=/usr/lib/llvm-3.8/lib;
+if [[ "$(uname)" == "Linux" ]]; then
+  LIBCLANG_PATH=/usr/lib/llvm-3.8/lib
 else
-  LIBCLANG_PATH=`brew --prefix llvm38`/lib/llvm-3.8/lib;
+  LIBCLANG_PATH="$(brew --prefix llvm38)/lib/llvm-3.8/lib"
 fi
 
-./regen.py --target all "$@"
+./regen.py --target all "${@}"
--- a/servo/ports/geckolib/gecko_bindings/tools/setup_bindgen.sh
+++ b/servo/ports/geckolib/gecko_bindings/tools/setup_bindgen.sh
@@ -4,42 +4,42 @@
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 set -o errexit
 set -o nounset
 set -o pipefail
 
 # Run in the tools directory.
-cd "$(dirname $0)"
+cd "$(dirname ${0})"
 
 # Setup and build bindgen.
-if [ "$(uname)" == "Linux" ]; then
-  export LIBCLANG_PATH=/usr/lib/llvm-3.8/lib;
+if [[ "$(uname)" == "Linux" ]]; then
+  export LIBCLANG_PATH=/usr/lib/llvm-3.8/lib
 else
-  export LIBCLANG_PATH=`brew --prefix llvm38`/lib/llvm-3.8/lib;
+  export LIBCLANG_PATH="$(brew --prefix llvm38)/lib/llvm-3.8/lib"
 fi
 
 # Make sure we have llvm38.
-if [ ! -x "$(command -v clang-3.8)" ]; then
+if [[ ! -x "$(command -v clang-3.8)" ]]; then
     echo "llmv38 must be installed." \
          "Mac users should |brew install llvm38|, Linux varies by distro."
     exit 1
 fi
 
-export LD_LIBRARY_PATH=$LIBCLANG_PATH
-export DYLD_LIBRARY_PATH=$LIBCLANG_PATH
+export LD_LIBRARY_PATH="${LIBCLANG_PATH}"
+export DYLD_LIBRARY_PATH="${LIBCLANG_PATH}"
 
 # Check for multirust
-if [ ! -x "$(command -v multirust)" ]; then
+if [[ ! -x "$(command -v multirust)" ]]; then
     echo "multirust must be installed."
     exit 1
 fi
 
 # Don't try to clone twice.
-if [ ! -d rust-bindgen ]; then
+if [[ ! -d rust-bindgen ]]; then
   git clone https://github.com/servo/rust-bindgen.git
 fi
 
 cd rust-bindgen
 
 multirust override nightly
 cargo build --features llvm_stable
--- a/servo/python/tidy/servo_tidy/tidy.py
+++ b/servo/python/tidy/servo_tidy/tidy.py
@@ -313,37 +313,54 @@ def check_toml(file_name, lines):
 
 def check_shell(file_name, lines):
     if not file_name.endswith(".sh"):
         raise StopIteration
 
     shebang = "#!/usr/bin/env bash"
     required_options = {"set -o errexit", "set -o nounset", "set -o pipefail"}
 
+    did_shebang_check = False
+
     if len(lines) == 0:
         yield (0, 'script is an empty file')
-    else:
-        if lines[0].rstrip() != shebang:
-            yield (1, 'script does not have shebang "{}"'.format(shebang))
+        return
+
+    if lines[0].rstrip() != shebang:
+        yield (1, 'script does not have shebang "{}"'.format(shebang))
 
-        for idx in range(1, len(lines)):
-            stripped = lines[idx].rstrip()
+    for idx in range(1, len(lines)):
+        stripped = lines[idx].rstrip()
+        # Comments or blank lines are ignored. (Trailing whitespace is caught with a separate linter.)
+        if lines[idx].startswith("#") or stripped == "":
+            continue
 
-            # Comments or blank lines are ignored. (Trailing whitespace is caught with a separate linter.)
-            if lines[idx].startswith("#") or stripped == "":
-                continue
-            elif stripped in required_options:
+        if not did_shebang_check:
+            if stripped in required_options:
                 required_options.remove(stripped)
             else:
                 # The first non-comment, non-whitespace, non-option line is the first "real" line of the script.
                 # The shebang, options, etc. must come before this.
                 if len(required_options) != 0:
                     formatted = ['"{}"'.format(opt) for opt in required_options]
                     yield (idx + 1, "script is missing options {}".format(", ".join(formatted)))
-                    break
+                did_shebang_check = True
+
+        if "`" in stripped:
+            yield (idx + 1, "script should not use backticks for command substitution")
+
+        if " [ " in stripped or stripped.startswith("[ "):
+            yield (idx + 1, "script should use `[[` instead of `[` for conditional testing")
+
+        for dollar in re.finditer('\$', stripped):
+            next_idx = dollar.end()
+            if next_idx < len(stripped):
+                next_char = stripped[next_idx]
+                if not (next_char == '{' or next_char == '('):
+                    yield(idx + 1, "variable substitutions should use the full \"${VAR}\" form")
 
 
 def check_rust(file_name, lines):
     if not file_name.endswith(".rs") or \
        file_name.endswith(".mako.rs") or \
        file_name.endswith(os.path.join("style", "build.rs")) or \
        file_name.endswith(os.path.join("geckolib", "build.rs")) or \
        file_name.endswith(os.path.join("unit", "style", "stylesheets.rs")):
--- a/servo/python/tidy/servo_tidy_tests/shell_tidy.sh
+++ b/servo/python/tidy/servo_tidy_tests/shell_tidy.sh
@@ -1,7 +1,14 @@
 #!/bin/bash
 #
 # Tests tidy for shell scripts.
 
 set -o nounset
 
+# Talking about some `concept in backticks` # shouldn't trigger
 echo "hello world"
+some_var=`echo "command substitution"`
+another_var="$some_var"
+if [ -z "${some_var}" ]; then
+  echo "should have used [["
+fi
+[ -z "${another_var}" ]
--- a/servo/python/tidy/servo_tidy_tests/test_tidy.py
+++ b/servo/python/tidy/servo_tidy_tests/test_tidy.py
@@ -47,16 +47,20 @@ class CheckTidiness(unittest.TestCase):
         errors = tidy.collect_errors_for_files(iterFile('incorrect_license.rs'), [], [tidy.check_license], print_text=False)
         self.assertEqual('incorrect license', errors.next()[2])
         self.assertNoMoreErrors(errors)
 
     def test_shell(self):
         errors = tidy.collect_errors_for_files(iterFile('shell_tidy.sh'), [], [tidy.check_shell], print_text=False)
         self.assertEqual('script does not have shebang "#!/usr/bin/env bash"', errors.next()[2])
         self.assertEqual('script is missing options "set -o errexit", "set -o pipefail"', errors.next()[2])
+        self.assertEqual('script should not use backticks for command substitution', errors.next()[2])
+        self.assertEqual('variable substitutions should use the full \"${VAR}\" form', errors.next()[2])
+        self.assertEqual('script should use `[[` instead of `[` for conditional testing', errors.next()[2])
+        self.assertEqual('script should use `[[` instead of `[` for conditional testing', errors.next()[2])
         self.assertNoMoreErrors(errors)
 
     def test_rust(self):
         errors = tidy.collect_errors_for_files(iterFile('rust_tidy.rs'), [], [tidy.check_rust], print_text=False)
         self.assertEqual('use statement spans multiple lines', errors.next()[2])
         self.assertEqual('missing space before }', errors.next()[2])
         self.assertTrue('use statement is not in alphabetical order' in errors.next()[2])
         self.assertEqual('use statement contains braces for single import', errors.next()[2])