Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Apply final set of Shellcheck fixes and turn on in CI #7832

Merged
merged 16 commits into from Jun 3, 2019
Merged
Changes from 11 commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -148,6 +148,7 @@ base_linux_config: &base_linux_config
- libssl-dev
- jq
- unzip
- shellcheck
language: python
before_install:
- ./build-support/bin/install_aws_cli_for_ci.sh
@@ -42,6 +42,7 @@ if [[ "${check}" == "true" ]]; then
cmd=("${cmd[@]}" "--check")
fi

# shellcheck disable=SC2207
This conversation was marked as resolved by Eric-Arellano

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jun 1, 2019

Author Contributor

I originally tried fixing all of the SC2207 issues but after 3 or 4 of them, lost complete confidence that my solutions were working and became frustrated with how finicky it all was. You can see the commit history for what they would have looked like.

See https://github.com/koalaman/shellcheck/wiki/SC2207. I don't think we can use their solution of mapfile because command -v mapfile fails on macOS.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Jun 3, 2019

Contributor

Can we make a tracking issue and link it in the places we disable this check? Alternatively, if we don't think it's going to be worth the effort to fix at all/ever, we could perhaps note that somehow (not sure how) so nobody else falls down this rabbit hole?

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jun 3, 2019

Author Contributor

I don't think it's going to be worth the effort to fix this ever. Should we add a NB comment about it for each of the ~6 instances?

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Jun 3, 2019

Contributor

Hence the (not sure how)! We could potentially add an extended comment in shellcheck.py listing any "WONTFIX"-esque shellchecks?

This comment has been minimized.

Copy link
@blorente

blorente Jun 3, 2019

Contributor

Maybe an issue in the repo, and link to the issue from the code? Sort of a "TO_NOT_DO(#issue)"

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jun 3, 2019

Author Contributor

I will go with the style of WONTFIX with a one-line description that links to shellcheck's wiki. Will also add this for the disables that we added in prior PRs.

bad_files=(
$(
cd "${NATIVE_ROOT}" || exit "${PIPESTATUS[0]}"
@@ -6,4 +6,4 @@
# Check for copies (-C) and moves (-M), so we don't get false positives when people do
# refactorings. -l50 bounds the time git takes to search for these non-additions.
# See git-diff(1) and https://stackoverflow.com/a/2299672/2518889 for discussion of these options.
exec git diff --cached --name-only --diff-filter=A -C -M -l50
exec git --no-pager diff --cached --name-only --diff-filter=A -C -M -l50

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jun 1, 2019

Author Contributor

Depending on how the user has their git set up, this script would sometimes fail. For example, my git for some reason defaults to opening git diff through less.

Now, the script will work no matter what.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Jun 3, 2019

Contributor

I didn't know about this argument! Note that many programs (including git) will respect the PAGER env var, and I've usually done this by running the command with PAGER=cat -- but this is definitely better!

@@ -23,6 +23,7 @@ for job in ${jobs}; do
;;
"failed")
"${curl[@]}" "https://api.travis-ci.org/job/${job}/log.txt" > "logs/jobs/${job}/txt"
# shellcheck disable=SC2207
targets=("${targets[@]}" $(cat -v "logs/jobs/${job}/txt" | awk '$2 == "....." && $3 ~ /^FAILURE/ {print $1}'))
;;
*)
@@ -75,7 +75,7 @@ function bootstrap_rust() {
fi
done

ln -fs "$(dirname "${cargo_bin_dir}")/lib/rustlib/src/rust/src"
ln -fs "$(dirname "${cargo_bin_dir}")/lib/rustlib/src/rust/src" .
This conversation was marked as resolved by Eric-Arellano

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jun 1, 2019

Author Contributor

ln defaults implicitly to using .. This makes it explicit.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Jun 3, 2019

Contributor

What does this create in the current directory? Is it a symlink named src? I really prefer making it explicit what gets created by using the ln TARGET LINK_NAME form instead of the .. I may be confused.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jun 3, 2019

Author Contributor

I honestly do not know :/ I could not figure out how to fix this one so just went with what shellcheck said is easiest to keep current behavior.

Refer to https://github.com/koalaman/shellcheck/wiki/SC2226.

This comment has been minimized.

Copy link
@illicitonion

illicitonion Jun 3, 2019

Contributor

This creates a link in the current directory called src. I'm happy with any of no second argument, ., or src.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Jun 3, 2019

Contributor

I would personally prefer using src, but that's just because I personally haven't seen the . format very often. However, it appears to be very standardized and understandable enough, so making the minimum change here (using .) seems to make sense.

This comment has been minimized.

Copy link
@blorente

blorente Jun 3, 2019

Contributor

It would seem so:

$ cd path/to/pants
$ ln -fs src/python/ .
$ ls | grep python
python
Suggested change
ln -fs "$(dirname "${cargo_bin_dir}")/lib/rustlib/src/rust/src" .
ln -fs "$(dirname "${cargo_bin_dir}")/lib/rustlib/src/rust/src" "src"

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jun 3, 2019

Author Contributor

Wonderful, I like src much more. Thanks!

ln -fs cargo.sh cargo
ln -fs cargo.sh ".${cargo_versioned}"
)
@@ -74,6 +74,7 @@ do
echo "* ${subject}"

urls=()
# shellcheck disable=SC2207
urls=(
"${urls[@]}"
$(
@@ -82,6 +83,7 @@ do
sed -Ee "s|^\(#([0-9]+)\)$|https://github.com/pantsbuild/pants/pull/\1|"
)
)
# shellcheck disable=SC2207
urls=(
"${urls[@]}"
$(
@@ -228,6 +228,7 @@ function build_pants_packages() {
pants_version_set "${version}"

start_travis_section "${NAME}" "Building packages"
# shellcheck disable=SC2207
packages=(
$(run_packages_script build_and_print "${version}")
) || die "Failed to build packages at ${version}!"
@@ -315,6 +316,7 @@ function install_and_test_packages() {
export PANTS_PLUGIN_CACHE_DIR
trap 'rm -rf "${PANTS_PLUGIN_CACHE_DIR}"' EXIT

# shellcheck disable=SC2207
packages=(
$(run_packages_script list | grep '.' | awk '{print $1}')
) || die "Failed to list packages!"
@@ -515,11 +517,12 @@ function fetch_and_check_prebuilt_wheels() {
fetch_prebuilt_wheels "${check_dir}"

local missing=()
# shellcheck disable=SC2207
RELEASE_PACKAGES=(
$(run_packages_script list | grep '.' | awk '{print $1}')
) || die "Failed to get a list of packages to release!"
for PACKAGE in "${RELEASE_PACKAGES[@]}"
do
for PACKAGE in "${RELEASE_PACKAGES[@]}"; do
# shellcheck disable=SC2207
packages=($(find_pkg "${PACKAGE}" "${PANTS_UNSTABLE_VERSION}" "${check_dir}"))
if [ ${#packages[@]} -eq 0 ]; then
missing+=("${PACKAGE}")
@@ -528,8 +531,7 @@ function fetch_and_check_prebuilt_wheels() {

# Confirm that if the package is not cross platform that we have whls for two platforms.
local cross_platform=""
for package in "${packages[@]}"
do
for package in "${packages[@]}"; do
if [[ "${package}" =~ -none-any.whl ]]
then
cross_platform="true"
@@ -559,6 +561,7 @@ function adjust_wheel_platform() {
local src_plat="$1"
local dst_plat="$2"
local dir="$3"
# shellcheck disable=SC2044
This conversation was marked as resolved by Eric-Arellano

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Jun 3, 2019

Contributor

It's not clear to me why this triggers from reading https://github.com/koalaman/shellcheck/wiki/SC2204?

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Jun 3, 2019

Contributor

(so: this disable is fine!)

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jun 3, 2019

Author Contributor

Think you're looking at the wrong link! Refer to https://github.com/koalaman/shellcheck/wiki/SC2044.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Jun 3, 2019

Contributor

Ah! Well then this could be changed into:

find "$dir" -type f -name "*${src_plat}.whl" | while read -r src_whl; do
  # ...
done

perhaps?

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Jun 3, 2019

Contributor

(and I would recommend this, as for loops are generally extremely fragile unless it's from a controlled input like seq!)

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jun 3, 2019

Author Contributor

Will try out. Thanks!

for src_whl in $(find "${dir}" -name '*'"${src_plat}.whl"); do
local dst_whl=${src_whl/$src_plat/$dst_plat}
mv -f "${src_whl}" "${dst_whl}"
@@ -6,7 +6,7 @@
import subprocess
from glob import glob

from common import die, green
from common import die


def main() -> None:
@@ -37,8 +37,6 @@ def run_shellcheck() -> None:
subprocess.run(command, check=True)
except subprocess.CalledProcessError:
die("Please fix the above errors and run again.")
else:
green("./pants passed the shellcheck!")


if __name__ == "__main__":
@@ -30,22 +30,23 @@ DIRS_TO_CHECK=(
# integration test!
set -e

# You can use ("$()") with double quotes in zsh, I assume this splits by IFS...
# shellcheck disable=SC2207
ADDED_FILES=($(./build-support/bin/get_added_files.sh))
MERGE_BASE="$(git_merge_base)"

echo "* Checking packages"
# TODO: Determine the most *hygienic* way to split an array on the command line in portable bash,
# and stick to it.
./build-support/bin/check_packages.sh "${DIRS_TO_CHECK[@]}"
./build-support/bin/check_packages.sh "${DIRS_TO_CHECK[@]}" || exit 1

echo "* Checking headers"
./build-support/bin/check_header.py "${DIRS_TO_CHECK[@]}" --files-added "${ADDED_FILES[@]}"
./build-support/bin/check_header.py "${DIRS_TO_CHECK[@]}" --files-added "${ADDED_FILES[@]}" || exit 1

echo "* Checking for banned imports"
./build-support/bin/check_banned_imports.py
./build-support/bin/check_banned_imports.py || exit 1

echo "* Checking for bad shell patterns"
echo "* Checking shell scripts via shellcheck"
./build-support/bin/shellcheck.py || exit 1

echo "* Checking shell scripts via our custom linter"
./build-support/bin/check_shell.sh || exit 1

# When travis builds a tag, it does so in a shallow clone without master fetched, which
@@ -141,6 +141,7 @@ base_linux_config: &base_linux_config
- libssl-dev
- jq
- unzip
- shellcheck
language: python
before_install:
- ./build-support/bin/install_aws_cli_for_ci.sh
@@ -19,8 +19,8 @@ function pkg_scrooge_install_test() {
function pkg_buildgen_install_test() {
local version=$1
shift
local PIP_ARGS="$@"
pip install ${PIP_ARGS} "pantsbuild.pants.contrib.buildgen==${version}" && \
local PIP_ARGS=("$@")
pip install "${PIP_ARGS[@]}" "pantsbuild.pants.contrib.buildgen==${version}" && \
python -c "from pants.contrib.buildgen.build_file_manipulator import *"
}

6 pants
@@ -73,6 +73,7 @@ export PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS="${PANTS_PYTHON_SETUP_INTERPRE
PANTS_EXE="${HERE}/src/python/pants/bin/pants_loader.py"

if [[ -n "${WRAPPER_REQUIREMENTS}" ]]; then
# shellcheck disable=SC2207
REQUIREMENTS=(
$(echo "${WRAPPER_REQUIREMENTS}" | tr : ' ')
"${REQUIREMENTS[@]}"
@@ -83,18 +84,19 @@ PANTS_SRCPATH=(
"${HERE}/src/python"
)
if [[ -n "${WRAPPER_SRCPATH}" ]]; then
# shellcheck disable=SC2207
PANTS_SRCPATH=(
$(echo "${WRAPPER_SRCPATH}" | tr : ' ')
"${PANTS_SRCPATH[@]}"
)
fi
PANTS_SRCPATH="$(echo "${PANTS_SRCPATH[@]}" | tr ' ' :)"
PANTS_SRCPATH_STR="$(echo "${PANTS_SRCPATH[@]}" | tr ' ' :)"
This conversation was marked as resolved by Eric-Arellano

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jun 1, 2019

Author Contributor

It's a bad practice to reassign an array to a string, according to shellcheck, so we simply workaround this via the new variable name.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Jun 3, 2019

Contributor

I would think it's probably "bad practice" to reassign a variable with a different type regardless of the type? So this seems reasonable.

This comment has been minimized.

Copy link
@blorente

blorente Jun 3, 2019

Contributor

I'm confused, here we were assigning a String to an array, right? Just checking my understanding.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jun 3, 2019

Author Contributor

Originally, we were converting the array PANTS_SRCPATH into a string. Now, we are creating a new string variable from the array PANTS_SRCPATH.


function exec_pants_bare() {
# Redirect activation and native bootstrap to ensure that they don't interfere with stdout.
activate_pants_venv 1>&2
bootstrap_native_code 1>&2
PYTHONPATH="${PANTS_SRCPATH}:${PYTHONPATH}" \
PYTHONPATH="${PANTS_SRCPATH_STR}:${PYTHONPATH}" \
exec python "${PANTS_EXE}" "$@"
}

@@ -23,7 +23,7 @@ set -eo pipefail
# If a file "went away", this won't remove it.

root=$(
cd "$(dirname $0)"
cd "$(dirname "$0")"
/bin/pwd
)

@@ -32,7 +32,7 @@ path_within_url=$2
out=/tmp/pantsdoc.$$

# When done, clean up tmp dir:
trap "rm -fr $out" 0 1 2
trap 'rm -fr $out' 0 1 2
This conversation was marked as resolved by Eric-Arellano

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jun 1, 2019

Author Contributor

This one might be wrong.. Refer to https://github.com/koalaman/shellcheck/wiki/SC2064 and let me know what you think.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Jun 3, 2019

Contributor

I think this works! The trap quoting rules confuse me greatly in general.

This comment has been minimized.

Copy link
@illicitonion

illicitonion Jun 3, 2019

Contributor

AIUI because we don't change the value of $out, these are equivalent here.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Jun 3, 2019

Contributor

(I didn't know about trap quoting rules until last week!)

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jun 3, 2019

Author Contributor

Agreed. Thanks for the check!


mkdir -p $out
cd $out
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.