Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
125 changes: 125 additions & 0 deletions check/all
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
#!/usr/bin/env bash
#
# Copyright 2025 Google LLC
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# https://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

# Summary: run multiple checks on the OpenFermion code base.
# This is modeled in part on Cirq's check/all script.

declare -r usage="\
Usage: ${0##*/} [--help] [--only-changed-files] [--apply-format-changes] [BASE_REV]

If the first argument given on the command line is the option --help or -h,
this program prints usage information and then exits.

With no arguments, this runs multiple checks on the code base. These tests
are based on the programs in the checks/ subdirectory.

If the option --no-coverage is specified, check/pytest will be run instead of
check/pytest-and-incremental-coverage.
Comment on lines +29 to +30
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fail to see any code for --no-coverage. Either remove this paragraph or handle the option in the script. Since passing coverage is required by the CI, it seems better to just change the text and keep running pytest-and-incremental-coverage unconditionally.


If --apply-format-changes is specified, the flag --apply will be passed to
check/format-incremental to apply the format changes suggested by the
formatter.

You can specify a base git revision to compare against (i.e., to use when
determining whether or not a file is considered to have changed). If given,
the argument BASE_REV is passed on to tests that can use it, such as
check/pytest-and-incremental-coverage."

set -eo pipefail -o errtrace
shopt -s inherit_errexit

# Get the working directory to the repo root.
thisdir=$(dirname "${BASH_SOURCE[0]:?}") || exit $?
repo_dir=$(git -C "${thisdir}" rev-parse --show-toplevel) || exit $?
cd "${repo_dir}" || exit $?
Comment on lines +45 to +47
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit - remove || exit $? because it is implied by the errexit (-e) option on line 41.


function error() {
echo >&2 "ERROR: ${*}"
}

# ~~~~ Parse the CLI arguments and gather some data ~~~~

declare rev
declare apply_arg
declare only_changed
Comment on lines +55 to +57
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit - it is safer to set initial value, e.g., declare rev="", otherwise the rev value could be picked from the rev environment variable. Also, as rev and apply_arg are optional for format-incremental, it is better to declare them as arrays which may be empty (and would not trigger shellcheck):

declare -a rev=()
declare -a apply_arg=()

for arg in "$@"; do
case "${arg}" in
-h | --help)
echo "${usage}"
exit 0
;;
--apply-format-changes)
apply_arg="--apply"
shift
;;
--only-changed-files)
only_changed="true"
shift
;;
-*)
if [[ -n "${rev}" ]]; then
error "Invalid arguments."
echo "${usage}"
exit 1
fi
Comment on lines +73 to +77
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This silently skips --no-coverage option or any other unknown option if there is no rev argument. How about

Suggested change
if [[ -n "${rev}" ]]; then
error "Invalid arguments."
echo "${usage}"
exit 1
fi
error "Invalid option '${arg}'"
error "See '$0 --help' for the list of supported options."
exit 1

;;
*)
if [ "$(git cat-file -t "${arg}" 2> /dev/null)" != "commit" ]; then
error "No revision '${arg}'"
exit 1
fi
rev="${arg}"
Comment on lines +80 to +84
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will reject annotated tag wrapping a commit, because its type is "tag" (this is an issue in Cirq too). Here is a better way:

Suggested change
if [ "$(git cat-file -t "${arg}" 2> /dev/null)" != "commit" ]; then
error "No revision '${arg}'"
exit 1
fi
rev="${arg}"
if ! rev=( "$(git rev-parse --verify --end-of-options "${arg}^{commit}")" ); then
error "No revision '${arg}'"
exit 1
fi

;;
esac
done

# ~~~~ Run the tests ~~~~

declare -a errors
declare program
Comment on lines +91 to +92
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bash seems to initialize errors[0] from the environment. The program is not used on global scope.

Suggested change
declare -a errors
declare program
declare -a errors=()


function run() {
local -r program="$1"
echo "Running $program ..."
$program || errors+=( "${program} failed" )
echo
Comment on lines +95 to +98
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing the entire command line as one argument (e.g., line 102) is fragile, in case any of desired arguments contain space or a glob-like character which would get expanded. It is safer to pass the command line arguments exactly:

Suggested change
local -r program="$1"
echo "Running $program ..."
$program || errors+=( "${program} failed" )
echo
echo "Running $* ..."
"$@" || errors+=( "$* failed" )
echo

}

if [[ -n "${only_changed}" ]]; then
run "check/format-incremental ${rev} ${apply_arg}"
run "check/pylint-changed-files ${rev}"
Comment on lines +102 to +103
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With rev and apply_arg declared as arrays, we can pass arguments to the run function exactly (similar change needed on lines 105, 109 below):

Suggested change
run "check/format-incremental ${rev} ${apply_arg}"
run "check/pylint-changed-files ${rev}"
run check/format-incremental "${rev[@]}" "${apply_arg[@]}"
run check/pylint-changed-files "${rev[@]}"

else
run "check/format-incremental ${rev} ${apply_arg} --all"
run "check/pylint"
fi
run "check/mypy"
run "check/pytest-and-incremental-coverage ${rev}"
run "check/shellcheck"

# ~~~~ Summarize the results and exit with a status code ~~~~

declare exit_code=0
echo
echo "Done."
if [[ "${#errors[@]}" == 0 ]]; then
echo "All checks passed."
else
error "Some checks failed."
printf " %s\n" "${errors[@]}"
exit_code=1
fi

exit "${exit_code}"
Loading