Add linting for shell scripts #12295

jimberlage · 2016-07-06T20:18:02Z

This changes tidy to check shell scripts for the proper shebang and
options. It does not check that variables are formatted correctly. It
also adds a check for the MPL 2.0 license in shell scripts.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #12158 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2016-07-06T20:18:07Z

Heads up! This PR modifies the following files:

@wafflespeanut: python/tidy/servo_tidy_tests/test_tidy.py, python/tidy/servo_tidy/licenseck.py, python/tidy/servo_tidy/tidy.py, python/tidy/servo_tidy_tests/shell_tidy.sh
@aneeshusa: etc/ci/check_no_unwrap.sh, etc/ci/manifest_changed.sh, etc/ci/upload_docs.sh, etc/ci/upload_nightly.sh, etc/ci/lockfile_changed.sh

jimberlage · 2016-07-06T20:21:00Z

I'd appreciate it if someone took a particularly close look at my changes to components/servo/fake-ld.sh; I don't have the bash expertise to tell if those changes are totally correct.

jimberlage · 2016-07-10T17:00:06Z

@Ms2ger could you re-run tests on this? They failed the first time because tidy was changed in between my initial checkout and this PR to test a few more files, and that exposed some out-of-spec shell scripts. That's since been fixed in #12299.

Ms2ger · 2016-07-10T21:42:18Z

R? @aneeshusa

aneeshusa · 2016-07-18T03:15:57Z

components/servo/fake-ld.sh

+TARGET_DIR="${OUT_DIR}/../../.."
+arm-linux-androideabi-gcc "$@" \
+                          "${LDFLAGS}" \
+                          -lc \


Let's put the LDFLAGS, lc and shared flags on the same line since they're all linker arguments.

aneeshusa · 2016-07-18T03:16:43Z

components/servo/fake-ld.sh

+                          "${LDFLAGS}" \
+                          -lc \
+                          -o "${TARGET_DIR}/libservo.so" \
+                          -shared && touch "${TARGET_DIR}/servo"


The touch can be on a separate line as a separate command (without the && in between), as the set -o errexit will handle not running the command if gcc fails.

aneeshusa · 2016-07-18T03:17:07Z

etc/ci/check_no_unwrap.sh

 # Make sure listed files do not contain "unwrap"

 set -o errexit
 set -o nounset
 set -o pipefail

-cd "$(git rev-parse --show-toplevel)" # cd into repo root so make sure paths works in any case
+# cd into repo root to make sure paths works in any case


grammar: works -> work

aneeshusa · 2016-07-18T03:19:26Z

python/tidy/servo_tidy/tidy.py

+
+    for idx, line in enumerate(lines):
+        if idx == 0:
+            if not line.startswith(shebang):


I think we could use != instead of not and startswith here.

aneeshusa · 2016-07-18T03:19:44Z

python/tidy/servo_tidy/tidy.py

+    for idx, line in enumerate(lines):
+        if idx == 0:
+            if not line.startswith(shebang):
+                yield (idx + 1, 'script does not start with "{}"'.format(shebang))


does not start with -> does not have shebang

aneeshusa · 2016-07-18T03:25:08Z

python/tidy/servo_tidy/tidy.py

+                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.
+                    found_script_statement = True


I think you can get rid of found_script_statement and just break after the if len(required_options) != 0 block, instead of doing it on the next iteration.

aneeshusa · 2016-07-18T03:27:48Z

python/tidy/servo_tidy/tidy.py

+    found_script_statement = False
+
+    for idx, line in enumerate(lines):
+        if idx == 0:


I think it would be cleaner to check the first line outside (before) the loop; that takes away a level of nesting inside the for and means we don't need enumerate anymore.

aneeshusa · 2016-07-18T03:30:08Z

python/tidy/servo_tidy/tidy.py

+                        formatted = []
+
+                        for opt in required_options:
+                            formatted.append('"{}"'.format(opt))


Use a list comprehension to inline the loop.


        Fixes up shell linting in tidy in response to feedback

This makes the changes suggested in servo#12295. The python code to lint shell scripts is cleans up generally and some shell scripts are changed to be more readable.

KiChjang · 2016-07-22T02:09:55Z

No merge commits, please. Use rebase instead.

jimberlage · 2016-07-22T04:06:27Z

Cool, I've gone ahead and rebased.

aneeshusa · 2016-07-22T14:57:59Z

etc/ci/upload_docs.sh

@@ -16,7 +20,8 @@ cp etc/doc.servo.org/* target/doc/

 python components/style/properties/build.py servo html

-OUT_DIR="`pwd`/target/doc/servo" make -f makefile.cargo -C components/script dom_docs
+OUT_DIR="`pwd`/target/doc/servo" \


Can you update this to use $(pwd) per the style guide?

aneeshusa · 2016-07-22T14:59:52Z

python/tidy/servo_tidy/tidy.py

+    shebang = "#!/usr/bin/env bash"
+    required_options = {"set -o errexit", "set -o nounset", "set -o pipefail"}
+
+    if lines[0].strip() != shebang:


Handle the case of an empty file (i.e. lines is empty).

Also, use rstrip() instead of strip().

aneeshusa · 2016-07-22T15:04:08Z

python/tidy/servo_tidy/tidy.py

+            # The shebang, options, etc. must come before this.
+            if len(required_options) != 0:
+                formatted = ['"{}"'.format(opt) for opt in required_options]
+


Remove the empty line.

bors-servo · 2016-07-22T15:58:48Z

⌛ Testing commit 8800af2 with merge 7b209e8...


        Auto merge of #12295 - jimberlage:12158/shell-linting, r=aneeshusa

Add linting for shell scripts  This changes tidy to check shell scripts for the proper shebang and options. It does not check that variables are formatted correctly. It also adds a check for the MPL 2.0 license in shell scripts. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #12158 (github issue number if applicable).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12295)

bors-servo · 2016-07-22T16:10:22Z

💔 Test failed - android

aneeshusa · 2016-07-22T16:43:46Z

components/servo/fake-ld.sh

+
+TARGET_DIR="${OUT_DIR}/../../.."
+arm-linux-androideabi-gcc "$@" \
+                          "${LDFLAGS}" -lc -shared \


LDFLAGS is not always set, so we need to give it a default value if it isn't. A bash parameter expansion should do the trick.

The default value can just be the empty string.


        Add linting for shell scripts

This changes tidy to check shell scripts for the proper shebang and options. It does not check that variables are formatted correctly. It also adds a check for the MPL 2.0 license in shell scripts.

KiChjang · 2016-07-22T22:37:07Z

@bors-servo r=aneeshusa

bors-servo · 2016-07-22T22:37:08Z

📌 Commit 7952bd0 has been approved by aneeshusa

bors-servo · 2016-07-22T22:37:10Z

⌛ Testing commit 7952bd0 with merge a8c41b7...


        Auto merge of #12295 - jimberlage:12158/shell-linting, r=aneeshusa

Add linting for shell scripts  This changes tidy to check shell scripts for the proper shebang and options. It does not check that variables are formatted correctly. It also adds a check for the MPL 2.0 license in shell scripts. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #12158 (github issue number if applicable).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12295)

bors-servo · 2016-07-22T22:48:58Z

💔 Test failed - android

aneeshusa · 2016-08-04T16:15:12Z

@bors-servo retry

Logs are gone (probably removed by the old-log-cleanup cron job)

bors-servo · 2016-08-04T17:31:18Z

⌛ Testing commit 7952bd0 with merge 4bc629b...


        Auto merge of #12295 - jimberlage:12158/shell-linting, r=aneeshusa

Add linting for shell scripts  This changes tidy to check shell scripts for the proper shebang and options. It does not check that variables are formatted correctly. It also adds a check for the MPL 2.0 license in shell scripts. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #12158 (github issue number if applicable).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12295)

bors-servo · 2016-08-04T18:32:35Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev

highfive assigned Ms2ger Jul 6, 2016

highfive added the S-awaiting-review label Jul 6, 2016

jimberlage mentioned this pull request Jul 6, 2016

Fix up shell scripts #12299

Merged

3 of 5 tasks complete

Ms2ger closed this Jul 10, 2016

Ms2ger reopened this Jul 10, 2016

aneeshusa assigned aneeshusa and unassigned Ms2ger Jul 11, 2016

aneeshusa reviewed Jul 18, 2016
View changes

python/tidy/servo_tidy/tidy.py Outdated

for idx, line in enumerate(lines):

if idx == 0:

if not line.startswith(shebang):

This comment has been minimized.

Sign in to view

aneeshusa Jul 18, 2016
Member

I think we could use != instead of not and startswith here.

aneeshusa reviewed Jul 18, 2016
View changes

python/tidy/servo_tidy/tidy.py Outdated

formatted = []

for opt in required_options:

formatted.append('"{}"'.format(opt))

This comment has been minimized.

Sign in to view

aneeshusa Jul 18, 2016
Member

Use a list comprehension to inline the loop.

aneeshusa added S-needs-code-changes and removed S-awaiting-review labels Jul 18, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Jul 22, 2016

jimberlage force-pushed the jimberlage:12158/shell-linting branch from fe73796 to 2206ee6 Jul 22, 2016

aneeshusa reviewed Jul 22, 2016
View changes

python/tidy/servo_tidy/tidy.py Outdated

# The shebang, options, etc. must come before this.

if len(required_options) != 0:

formatted = ['"{}"'.format(opt) for opt in required_options]

This comment has been minimized.

Sign in to view

aneeshusa Jul 22, 2016
Member

Remove the empty line.

highfive added the S-awaiting-merge label Jul 22, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Jul 22, 2016

aneeshusa reviewed Jul 22, 2016
View changes

Add linting for shell scripts

Loading status checks…

7952bd0

This changes tidy to check shell scripts for the proper shebang and options. It does not check that variables are formatted correctly. It also adds a check for the MPL 2.0 license in shell scripts.

jimberlage force-pushed the jimberlage:12158/shell-linting branch from 8800af2 to 7952bd0 Jul 22, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Jul 22, 2016

highfive removed the S-awaiting-review label Jul 22, 2016

highfive added the S-awaiting-merge label Jul 22, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Jul 22, 2016

aneeshusa mentioned this pull request Jul 22, 2016

Unncessary use of $LDFLAGS in components/servo/fake-ld.sh? #12562

Closed

highfive added S-awaiting-merge and removed S-tests-failed labels Aug 4, 2016

bors-servo merged commit 7952bd0 into servo:master Aug 4, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Aug 4, 2016

bors-servo mentioned this pull request Aug 4, 2016

Rename check_no_unwrap.sh to check_no_panic.sh #10942

Merged

servo / servo

Add linting for shell scripts #12295

Add linting for shell scripts #12295

jimberlage commented Jul 6, 2016 •

edited

highfive commented Jul 6, 2016

jimberlage commented Jul 6, 2016

jimberlage commented Jul 10, 2016

Ms2ger commented Jul 10, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

KiChjang commented Jul 22, 2016

jimberlage commented Jul 22, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Jul 22, 2016

bors-servo commented Jul 22, 2016

This comment has been minimized.

This comment has been minimized.

KiChjang commented Jul 22, 2016

bors-servo commented Jul 22, 2016

bors-servo commented Jul 22, 2016

bors-servo commented Jul 22, 2016

aneeshusa commented Aug 4, 2016

bors-servo commented Aug 4, 2016

bors-servo commented Aug 4, 2016

servo / servo

Join GitHub today

Add linting for shell scripts #12295

Add linting for shell scripts #12295

Conversation

jimberlage commented Jul 6, 2016 • edited

highfive commented Jul 6, 2016

jimberlage commented Jul 6, 2016

jimberlage commented Jul 10, 2016

Ms2ger commented Jul 10, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

KiChjang commented Jul 22, 2016

jimberlage commented Jul 22, 2016

This comment has been minimized.

This comment has been minimized.

aneeshusa Jul 22, 2016 • edited

This comment has been minimized.

This comment has been minimized.

bors-servo commented Jul 22, 2016

bors-servo commented Jul 22, 2016

This comment has been minimized.

This comment has been minimized.

KiChjang commented Jul 22, 2016

bors-servo commented Jul 22, 2016

bors-servo commented Jul 22, 2016

bors-servo commented Jul 22, 2016

aneeshusa commented Aug 4, 2016

bors-servo commented Aug 4, 2016

bors-servo commented Aug 4, 2016

jimberlage commented Jul 6, 2016 •

edited

aneeshusa Jul 22, 2016 •

edited