Shell formatting and linting (#281)

# Motivation
A shell script just failed because it was written in bash, had no shebang, and was executed in dash.

So I'm adding shellcheck and also shfmt, while I am about it.

# Changes
- Add shellcheck and shfmt, both as github checks and as scripts to be executed locally.
  - Note:  I have not made github automatically commit shfmt output, as reformatting shell is riskier than reformatting most things.
  - Note: I have made the indent 2 spaces rather than the 4 used in the main ic repo shfmt, to match the existing code.
  - Note: I have kept the formatting scripts simple and lightweight rather than using the more sophisticated approach used in the main ic repo.
- Combined the github actions formatting steps into a single job, otherwise one of the pushes will fail, if there are several.
  - Added `cargo fmt` to the formatting commands, alongside shfmt and prettier for ts and a special prettier for svelte.
- Removed an empty "generate-wasm.sh" file that was generating linter errors and doesn't seem to be used.
- Added `set -euo pipefail` in various places as requested.

.github/workflows/checks.yml

@@ -4,10 +4,64 @@ on:
   push:
     branches:
       - main
+      - svelte
   pull_request:
 
 jobs:
+
+  formatting:
+    runs-on: ubuntu-latest
+    env:
+      DEPLOY_ENV: mainnet
+
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v2
+
+      - name: Install shfmt
+        run: sudo snap install shfmt
+
+      - name: Install Rust
+        run: |
+          rustup update ${{ matrix.rust }} --no-self-update
+          rustup default ${{ matrix.rust }}
+          rustup component add rustfmt
+
+      - name: Format rust
+        run: cargo fmt
+
+      - name: Format shell scripts
+        run: ./scripts/fmt-sh
+
+      - name: Install ts dependencies
+        run: npm ci
+        working-directory: ./frontend/ts
+      - name: Format ts
+        run: npm run format
+        working-directory: ./frontend/ts
+
+      - name: Install svelte dependencies
+        run: npm ci
+        working-directory: ./frontend/svelte
+      - name: Format svelte
+        run: npm run format
+        working-directory: ./frontend/svelte
+
+      - name: Commit Formatting changes - will not trigger rebuild
+        uses: EndBug/[email protected]
+        if: ${{ github.event_name == 'pull_request' }}
+        with:
+          add: .
+          author_name: Formatting Committer
+          author_email: "<[email protected]>"
+          message: "Updating svelte frontend formatting"
+          # do not pull: if this branch is behind, then we might as well let
+          # the pushing fail
+          pull_strategy: "NO-PULL"
+
+
   cargo-tests:
+    needs: formatting
     runs-on: ${{ matrix.os }}
     strategy:
       matrix:
@@ -49,6 +103,7 @@ jobs:
           RUST_BACKTRACE: 1
 
   flutter-tests:
+    needs: formatting
     runs-on: ubuntu-latest
     env:
       DEPLOY_ENV: mainnet
@@ -69,21 +124,6 @@ jobs:
         run: npm run test
         working-directory: ./frontend/ts
 
-      - name: Check formatting
-        run: npm run format
-        working-directory: ./frontend/ts
-      - name: Commit Formatting changes
-        uses: EndBug/[email protected]
-        if: ${{ github.event_name == 'pull_request' }}
-        with:
-          add: .
-          author_name: Formatting Committer
-          author_email: "<[email protected]>"
-          message: "Updating frontend formatting"
-          # do not pull: if this branch is behind, then we might as well let
-          # the pushing fail
-          pull_strategy: "NO-PULL"
-
       - name: Flutter setup
         uses: subosito/flutter-action@v1
         with:
@@ -103,6 +143,7 @@ jobs:
         working-directory: ./frontend/dart
 
   svelte-tests:
+    needs: formatting
     runs-on: ubuntu-latest
     defaults:
       run:
@@ -124,17 +165,13 @@ jobs:
       - name: Run linter
         run: npm run check
 
-      - name: Check formatting
-        run: npm run format
-
-      - name: Commit Formatting changes - will not trigger rebuild
-        uses: EndBug/[email protected]
-        if: ${{ github.event_name == 'pull_request' }}
-        with:
-          add: .
-          author_name: Formatting Committer
-          author_email: "<[email protected]>"
-          message: "Updating svelte frontend formatting"
-          # do not pull: if this branch is behind, then we might as well let
-          # the pushing fail
-          pull_strategy: "NO-PULL"
+  shell-checks:
+    needs: formatting
+    name: ShellCheck
+    runs-on: ubuntu-latest
+    steps:
+    - uses: actions/checkout@v2
+    - name: Run ShellCheck
+      uses: ludeeus/action-shellcheck@master
+      env:
+        SHELLCHECK_OPTS: -e SC1090 -e SC2119 -e SC1091

.rustfmt.toml

@@ -0,0 +1 @@
+max_width=120

Dockerfile

@@ -4,6 +4,7 @@
 # docker run --rm --entrypoint cat nns-dapp /nns-dapp.wasm > nns-dapp.wasm
 
 FROM ubuntu:20.10
+SHELL ["bash", "-c"]
 
 ARG rust_version=1.54.0
 ENV NODE_VERSION=14.15.4

build.sh

@@ -4,7 +4,7 @@ set -e
 
 if ! [[ $DEPLOY_ENV = "testnet" ]] && ! [[ $DEPLOY_ENV = "mainnet" ]] && ! [[ $DEPLOY_ENV = "local" ]]; then
   echo "Which deployment environment? Set DEPLOY_ENV to 'testnet' or 'mainnet' or 'local'"
-	exit 1
+  exit 1
 fi
 
 set -x
@@ -30,8 +30,8 @@ sed -i -e 's/flutter_service_worker.js?v=[0-9]*/flutter_service_worker.js/' buil
 cd build/web/ || exit
 # Remove the assets/NOTICES file, as it's large in size and not used.
 rm assets/NOTICES
-tar cJv --mtime='2021-05-07 17:00+00' --sort=name --exclude .last_build_id -f ../../../../assets.tar.xz . || \
-gtar cJv --mtime='2021-05-07 17:00+00' --sort=name --exclude .last_build_id -f ../../../../assets.tar.xz .
+tar cJv --mtime='2021-05-07 17:00+00' --sort=name --exclude .last_build_id -f ../../../../assets.tar.xz . ||
+  gtar cJv --mtime='2021-05-07 17:00+00' --sort=name --exclude .last_build_id -f ../../../../assets.tar.xz .
 cd ../../../.. || exit
 ls -sh assets.tar.xz
 sha256sum assets.tar.xz

deploy.sh

@@ -1,4 +1,6 @@
+#!/usr/bin/env bash
+set -euo pipefail
 NETWORK=$1
 
 echo "Deploying to $NETWORK..."
-DEPLOY_ENV=$NETWORK dfx deploy --no-wallet --network $NETWORK
+DEPLOY_ENV=$NETWORK dfx deploy --no-wallet --network "$NETWORK"

frontend/svelte/nns-js/proto/update_proto.sh

@@ -1,14 +1,16 @@
+#!/usr/bin/env bash
+set -euo pipefail
 # A script for generating the JS of proto files.
 protoc \
---plugin="protoc-gen-ts=./node_modules/.bin/protoc-gen-ts" \
---ts_out="./" \
---js_out="import_style=commonjs,binary:./" \
---proto_path="./" \
-./proto/base_types.proto
+  --plugin="protoc-gen-ts=./node_modules/.bin/protoc-gen-ts" \
+  --ts_out="./" \
+  --js_out="import_style=commonjs,binary:./" \
+  --proto_path="./" \
+  ./proto/base_types.proto
 
 protoc \
---plugin="protoc-gen-ts=./node_modules/.bin/protoc-gen-ts" \
---ts_out="./proto" \
---js_out="import_style=commonjs,binary:./proto" \
---proto_path="./proto" \
-./proto/ledger.proto
+  --plugin="protoc-gen-ts=./node_modules/.bin/protoc-gen-ts" \
+  --ts_out="./proto" \
+  --js_out="import_style=commonjs,binary:./proto" \
+  --proto_path="./proto" \
+  ./proto/ledger.proto

frontend/ts/build.sh

@@ -3,7 +3,7 @@ set -e
 
 if ! [[ $DEPLOY_ENV = "testnet" ]] && ! [[ $DEPLOY_ENV = "mainnet" ]] && ! [[ $DEPLOY_ENV = "local" ]]; then
   echo "Which deployment environment? Set DEPLOY_ENV to 'testnet' or 'mainnet' or 'local'"
-	exit 1
+  exit 1
 fi
 
 npm ci
@@ -16,5 +16,5 @@ popd
 
 npx tsc
 echo "Bundling..."
-npx browserify ./build/index.js --standalone IcAgent > ../dart/assets/ic_agent.js
+npx browserify ./build/index.js --standalone IcAgent >../dart/assets/ic_agent.js
 echo "Done."

frontend/ts/src/proto/update_proto.sh

@@ -1,22 +1,24 @@
+#!/usr/bin/env bash
+set -euo pipefail
 # A script for updating the protobuf files.
 
 protoc \
---plugin="protoc-gen-ts=./node_modules/.bin/protoc-gen-ts" \
---ts_out="./src/proto" \
---js_out="import_style=commonjs,binary:./src/proto" \
---proto_path="./src/proto" \
-./src/proto/base_types.proto
+  --plugin="protoc-gen-ts=./node_modules/.bin/protoc-gen-ts" \
+  --ts_out="./src/proto" \
+  --js_out="import_style=commonjs,binary:./src/proto" \
+  --proto_path="./src/proto" \
+  ./src/proto/base_types.proto
 
 protoc \
---plugin="protoc-gen-ts=./node_modules/.bin/protoc-gen-ts" \
---ts_out="./src/proto" \
---js_out="import_style=commonjs,binary:./src/proto" \
---proto_path="./src/proto" \
-./src/proto/governance.proto
+  --plugin="protoc-gen-ts=./node_modules/.bin/protoc-gen-ts" \
+  --ts_out="./src/proto" \
+  --js_out="import_style=commonjs,binary:./src/proto" \
+  --proto_path="./src/proto" \
+  ./src/proto/governance.proto
 
 protoc \
---plugin="protoc-gen-ts=./node_modules/.bin/protoc-gen-ts" \
---ts_out="./src/proto" \
---js_out="import_style=commonjs,binary:./src/proto" \
---proto_path="./src/proto" \
-./src/proto/ledger.proto
+  --plugin="protoc-gen-ts=./node_modules/.bin/protoc-gen-ts" \
+  --ts_out="./src/proto" \
+  --js_out="import_style=commonjs,binary:./src/proto" \
+  --proto_path="./src/proto" \
+  ./src/proto/ledger.proto

frontend/ts/update_candid.sh

@@ -1,6 +1,7 @@
-didc bind src/canisters/icManagement/canister.did -t ts > ./src/canisters/icManagement/rawService.ts;
-didc bind src/canisters/icManagement/canister.did -t js > ./src/canisters/icManagement/canister.did.js
-didc bind src/canisters/governance/canister.did -t ts > ./src/canisters/governance/rawService.ts;
-didc bind src/canisters/governance/canister.did -t js > ./src/canisters/governance/canister.did.js
-didc bind ../../rs/nns-dapp.did -t ts > ./src/canisters/nnsDapp/rawService.ts
-didc bind ../../rs/nns-dapp.did -t js > ./src/canisters/nnsDapp/canister.did.js
+#!/usr/bin/env bash
+didc bind src/canisters/icManagement/canister.did -t ts >./src/canisters/icManagement/rawService.ts
+didc bind src/canisters/icManagement/canister.did -t js >./src/canisters/icManagement/canister.did.js
+didc bind src/canisters/governance/canister.did -t ts >./src/canisters/governance/rawService.ts
+didc bind src/canisters/governance/canister.did -t js >./src/canisters/governance/canister.did.js
+didc bind ../../rs/nns-dapp.did -t ts >./src/canisters/nnsDapp/rawService.ts
+didc bind ../../rs/nns-dapp.did -t js >./src/canisters/nnsDapp/canister.did.js

ic-hardware-wallet-cli/ic-hardware-wallet-cli

@@ -1,6 +1,6 @@
 #!/bin/bash
 
-pushd "$(dirname "$0")/../frontend/ts" > /dev/null # Move to the script's directory.
+pushd "$(dirname "$(realpath "$0")")/../frontend/ts" || exit >/dev/null # Move to the script's directory.
 
 if [ ! -d "node_modules" ]; then
   npm install
@@ -9,5 +9,5 @@ fi
 # This isn't needed by the CLI, but is done to keep the TS compiler happy.
 DEPLOY_ENV=mainnet ../../update_config.sh
 
-npm run ic-hardware-wallet-cli --silent -- $@
-popd > /dev/null
+npm run ic-hardware-wallet-cli --silent -- "${@}"
+popd || exit >/dev/null

rs/nns_functions_candid_gen/src/payloads.rs

@@ -241,52 +241,52 @@ pub struct UpdateNodeOperatorConfigPayload {
 #[derive(CandidType, Deserialize, Clone, PartialEq, Message)]
 pub struct UpdateNodeRewardsTableProposalPayload {
     /// Maps regions to the node reward rates in that region
-    #[prost(btree_map="string, message", tag="1")]
+    #[prost(btree_map = "string, message", tag = "1")]
     pub new_entries: ::prost::alloc::collections::BTreeMap<::prost::alloc::string::String, NodeRewardRates>,
 }
 
 #[derive(CandidType, Deserialize, Clone, PartialEq, Message)]
 pub struct NodeRewardRate {
     /// The number of 10,000ths of IMF SDR (currency code XDR) to be rewarded per
     /// node per month.
-    #[prost(uint64, tag="1")]
+    #[prost(uint64, tag = "1")]
     pub xdr_permyriad_per_node_per_month: u64,
 }
 
 #[derive(CandidType, Deserialize, Clone, PartialEq, Message)]
 pub struct NodeRewardRates {
     /// Maps node types to the reward rate for that node type
-    #[prost(btree_map="string, message", tag="1")]
+    #[prost(btree_map = "string, message", tag = "1")]
     pub rates: ::prost::alloc::collections::BTreeMap<::prost::alloc::string::String, NodeRewardRate>,
 }
 
 // https://gitlab.com/dfinity-lab/core/ic/-/blob/1e167e754b674f612e989cdee02acb79cfe40be8/rs/protobuf/def/registry/dc/v1/dc.proto#L27
 #[derive(CandidType, Deserialize, Clone, PartialEq, Message)]
 pub struct AddOrRemoveDataCentersProposalPayload {
-    #[prost(message, repeated, tag="1")]
+    #[prost(message, repeated, tag = "1")]
     pub data_centers_to_add: ::prost::alloc::vec::Vec<DataCenterRecord>,
     /// The IDs of data centers to remove
-    #[prost(string, repeated, tag="2")]
+    #[prost(string, repeated, tag = "2")]
     pub data_centers_to_remove: ::prost::alloc::vec::Vec<::prost::alloc::string::String>,
 }
 
 #[derive(CandidType, Deserialize, Clone, PartialEq, Message)]
 pub struct DataCenterRecord {
-    #[prost(string, tag="1")]
+    #[prost(string, tag = "1")]
     pub id: ::prost::alloc::string::String,
-    #[prost(string, tag="2")]
+    #[prost(string, tag = "2")]
     pub region: ::prost::alloc::string::String,
-    #[prost(string, tag="3")]
+    #[prost(string, tag = "3")]
     pub owner: ::prost::alloc::string::String,
-    #[prost(message, optional, tag="4")]
+    #[prost(message, optional, tag = "4")]
     pub gps: ::core::option::Option<Gps>,
 }
 
 #[derive(CandidType, Deserialize, Clone, PartialEq, Message)]
 pub struct Gps {
-    #[prost(float, tag="1")]
+    #[prost(float, tag = "1")]
     pub latitude: f32,
-    #[prost(float, tag="2")]
+    #[prost(float, tag = "2")]
     pub longitude: f32,
 }