Merge 948e84fcd2 into acb598e5dd

2025-12-24 20:31:27 +00:00 · 2025-10-31 12:28:38 -07:00 · 2025-10-31 12:28:38 -07:00 · f74a34cecb
parent acb598e5dd 948e84fcd2
commit f74a34cecb
8 changed files with 211 additions and 49 deletions
--- a/.github/workflows/shellcheck.yml
+++ b/.github/workflows/shellcheck.yml
@ -0,0 +1,28 @@
+name: ShellCheck
+
+on:
+  pull_request:
+    types: [opened, synchronize]
+  push:
+    branches:
+      - master
+      - dev
+      - staging
+
+permissions:
+  contents: read
+
+jobs:
+  shellcheck:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout code
+        uses: actions/checkout@v4
+
+      - name: Run ShellCheck
+        uses: ludeeus/action-shellcheck@master
+        with:
+          scandir: '.'
+          format: gcc
+          severity: style
+
--- a/.shellcheckrc
+++ b/.shellcheckrc
@ -0,0 +1,31 @@
+# ShellCheck configuration file
+# See https://github.com/koalaman/shellcheck/wiki/Directives for more information
+
+# Enable all optional checks
+enable=all
+
+# Disable specific warnings that may be acceptable in this project
+# SC1090: Can't follow non-constant source. Use a path to a file that can be checked
+#         (disabled because we use dynamic sourcing with script_dir)
+disable=SC1090
+
+# SC1091: Not following: <file> was not specified as input
+#         (disabled because lib.sh is sourced dynamically)
+disable=SC1091
+
+# SC2310: Function invoked in && condition (set -e disabled)
+#         (acceptable pattern for conditional execution)
+disable=SC2310
+
+# SC2311: Bash implicitly disabled set -e in command substitution
+#         (acceptable pattern - we want to capture output even if function fails)
+disable=SC2311
+
+# SC2312: Consider invoking command separately to avoid masking return value
+#         (many of these are acceptable patterns in command substitutions)
+disable=SC2312
+
+# Exclude external files that we don't control
+exclude-dir=.git
+exclude-dir=testlogs
+
--- a/README.md
+++ b/README.md
@ -154,3 +154,84 @@ For more context and information see [issue #57](https://github.com/awalsh128/ca
 ### Cache Limits

 A repository can have up to 5GB of caches. Once the 5GB limit is reached, older caches will be evicted based on when the cache was last accessed. Caches that are not accessed within the last week will also be evicted. To get more information on how to access and manage your actions's caches, see [GitHub Actions / Using workflows / Cache dependencies](https://docs.github.com/en/actions/using-workflows/caching-dependencies-to-speed-up-workflows#viewing-cache-entries).
+
+## Development
+
+### Prerequisites
+
+- **Go 1.20+** (for building the `apt_query` binary) - version specified in `go.mod`
+  - Install from [golang.org](https://golang.org/dl/) or via package manager
+  - Verify installation: `go version`
+- **ShellCheck** (for linting shell scripts) - install via:
+  - macOS: `brew install shellcheck`
+  - Linux: `sudo apt-get install shellcheck` or see [shellcheck installation guide](https://github.com/koalaman/shellcheck#installing)
+  - Windows: Available via [scoop](https://scoop.sh/) or [chocolatey](https://chocolatey.org/)
+
+### Building
+
+The project includes Go binaries (`apt_query-arm64` and `apt_query-x86`) that are used by the shell scripts to query APT package information.
+
+**Build all packages:**
+```bash
+go build -v ./...
+```
+
+**Build for specific architecture:**
+```bash
+# For ARM64 (Apple Silicon, ARM servers)
+GOARCH=arm64 go build -o apt_query-arm64 ./src/cmd/apt_query
+
+# For x86_64 (Intel/AMD)
+GOARCH=amd64 go build -o apt_query-x86 ./src/cmd/apt_query
+```
+
+**Run tests:**
+```bash
+go test -v ./...
+```
+
+### Linting
+
+This project uses [ShellCheck](https://github.com/koalaman/shellcheck) to ensure shell script quality and catch common errors. The configuration is stored in `.shellcheckrc`.
+
+**Run ShellCheck locally:**
+```bash
+shellcheck *.sh
+```
+
+**IDE Integration:**
+
+Many IDEs and editors can automatically run ShellCheck:
+
+- **VS Code**: Install the [ShellCheck extension](https://marketplace.visualstudio.com/items?itemName=timonwong.shellcheck)
+- **Vim/Neovim**: Use [ALE](https://github.com/dense-analysis/ale) or [coc-shellcheck](https://github.com/josa42/coc-shellcheck)
+- **IntelliJ/CLion**: ShellCheck is integrated in recent versions
+- **Sublime Text**: Install [SublimeLinter-shellcheck](https://github.com/SublimeLinter/SublimeLinter-shellcheck)
+
+**Go Linting:**
+
+This project uses [golangci-lint](https://golangci-lint.run/) for Go code quality checks.
+
+**Run golangci-lint locally:**
+```bash
+# Install golangci-lint (if not already installed)
+# macOS: brew install golangci-lint
+# Linux: See https://golangci-lint.run/usage/install/
+
+golangci-lint run
+```
+
+**IDE Integration:**
+
+- **VS Code**: Install the [Go extension](https://marketplace.visualstudio.com/items?itemName=golang.go) for syntax highlighting, auto-completion, and built-in linting
+- **IntelliJ/GoLand**: Built-in Go support with linting and formatting
+- **Vim/Neovim**: Use [vim-go](https://github.com/fatih/vim-go) for Go development
+
+### CI/CD
+
+The GitHub Actions workflows will automatically:
+- **Build and test** Go code on pull requests
+- **Run ShellCheck** on shell scripts (blocks PRs on failures)
+- **Run golangci-lint** on Go code (blocks PRs on failures)
+
+All checks run on pull requests and pushes to `master`, `dev`, and `staging` branches.
--- a/install_and_cache_pkgs.sh
+++ b/install_and_cache_pkgs.sh
@ -19,7 +19,7 @@ cache_dir="${1}"
 add_repository="${3}"

 # List of the packages to use.
-input_packages="${@:4}"
+input_packages="${*:4}"

 if ! apt-fast --version > /dev/null 2>&1; then
  log "Installing apt-fast for optimized installs..."
@ -31,7 +31,7 @@ if ! apt-fast --version > /dev/null 2>&1; then
 fi

 # Add custom repositories if specified
-if [ -n "${add_repository}" ]; then
+if [[ -n "${add_repository}" ]]; then
  log "Adding custom repositories..."
  for repository in ${add_repository}; do
    log "- Adding repository: ${repository}"
@ -76,7 +76,10 @@ install_log_filepath="${cache_dir}/install.log"

 log "Clean installing ${package_count} packages..."
 # Zero interaction while installing or upgrading the system via apt.
-sudo DEBIAN_FRONTEND=noninteractive apt-fast --yes install ${packages} > "${install_log_filepath}"
+# Note: sudo doesn't affect redirects, but we want the output in the file anyway
+# shellcheck disable=SC2024
+# We intentionally redirect output here; the redirect happens as the current user which is fine
+sudo DEBIAN_FRONTEND=noninteractive apt-fast --yes install "${packages}" > "${install_log_filepath}"
 log "done"
 log "Installation log written to ${install_log_filepath}"

@ -86,7 +89,7 @@ installed_packages=$(get_installed_packages "${install_log_filepath}")
 log "Installed package list:"
 for installed_package in ${installed_packages}; do
  # Reformat for human friendly reading.  
-  log "- $(echo ${installed_package} | awk -F\= '{print $1" ("$2")"}')"
+  log "- $(echo "${installed_package}" | awk -F= '{print $1" ("$2")"}')"
 done

 log_empty_line
@ -98,7 +101,7 @@ for installed_package in ${installed_packages}; do

  # Sanity test in case APT enumerates duplicates.
  if test ! -f "${cache_filepath}"; then
-    read package_name package_ver < <(get_package_name_ver "${installed_package}")
+    read -r package_name package_ver < <(get_package_name_ver "${installed_package}")
    log "  * Caching ${package_name} to ${cache_filepath}..."

    # Pipe all package files (no folders), including symlinks, their targets, and installation control data to Tar.
@ -109,9 +112,9 @@ for installed_package in ${installed_packages}; do
      while IFS= read -r f; do
        if test -f "${f}" -o -L "${f}"; then
          get_tar_relpath "${f}"
-          if [ -L "${f}" ]; then
+          if [[ -L "${f}" ]]; then
            target="$(readlink -f "${f}")"
-            if [ -f "${target}" ]; then
+            if [[ -f "${target}" ]]; then
              get_tar_relpath "${target}"
            fi
          fi
@ -125,7 +128,7 @@ for installed_package in ${installed_packages}; do
  # Comma delimited name:ver pairs in the all packages manifest.
  manifest_all="${manifest_all}${package_name}=${package_ver},"
 done
-log "done (total cache size $(du -h ${cache_dir} | tail -1 | awk '{print $1}'))"
+log "done (total cache size $(du -h "${cache_dir}" | tail -1 | awk '{print $1}'))"

 log_empty_line

--- a/lib.sh
+++ b/lib.sh
@ -17,14 +17,16 @@ set +e
 #   Filepath of the install script, otherwise an empty string.
 ###############################################################################
 function execute_install_script {
-  local package_name=$(basename ${2} | awk -F\= '{print $1}')  
-  local install_script_filepath=$(\
+  local package_name
+  package_name=$(basename "${2}" | awk -F= '{print $1}')  
+  local install_script_filepath
+  install_script_filepath=$(\
    get_install_script_filepath "${1}" "${package_name}" "${3}")
  if test ! -z "${install_script_filepath}"; then
    log "- Executing ${install_script_filepath}..."
    # Don't abort on errors; dpkg-trigger will error normally since it is
    # outside its run environment.
-    sudo sh -x ${install_script_filepath} ${4} || true
+    sudo sh -x "${install_script_filepath}" "${4}" || true
    log "  done"
  fi
 }
@ -40,9 +42,17 @@ function execute_install_script {
 ###############################################################################
 function get_install_script_filepath {
  # Filename includes arch (e.g. amd64).
-  local filepath="$(\
-    ls -1 ${1}var/lib/dpkg/info/${2}*.${3} 2> /dev/null \
-    | grep -E ${2}'(:.*)?.'${3} | head -1 || true)"
+  local filepath
+  # Use glob expansion instead of ls|grep for better handling of non-alphanumeric filenames
+  # Use nullglob to prevent literal match when no files found
+  shopt -s nullglob
+  for f in "${1}"var/lib/dpkg/info/"${2}"*."${3}"; do
+    if [[ -f "${f}" ]] && [[ "${f}" =~ ${2}'(:.*)?.'"${3}" ]]; then
+      filepath="${f}"
+      break
+    fi
+  done
+  shopt -u nullglob
  test "${filepath}" && echo "${filepath}"
 }

@ -66,7 +76,7 @@ function get_installed_packages {
      log_err "Unable to parse package name and version from \"${line}\""
      exit 2
    fi
-  done < <(grep "^Unpacking " ${install_log_filepath})
+  done < <(grep "^Unpacking " "${install_log_filepath}")
  if test -n "${dep_packages}"; then
    echo "${dep_packages:0:-1}"  # Removing trailing space.
  else
@ -83,13 +93,13 @@ function get_installed_packages {
 ###############################################################################
 function get_package_name_ver {
  local ORIG_IFS="${IFS}"
-  IFS=\= read name ver <<< "${1}"
+  IFS='=' read -r name ver <<< "${1}"
  IFS="${ORIG_IFS}"
  # If version not found in the fully qualified package value.
  if test -z "${ver}"; then
    # This is a fallback and should not be used any more as its slow.
    log_err "Unexpected version resolution for package '${name}'"
-    ver="$(apt-cache show ${name} | grep '^Version:' | awk '{print $2}')"
+    ver="$(apt-cache show "${name}" | grep '^Version:' | awk '{print $2}')"
  fi
  echo "${name}" "${ver}"  
 }
@ -105,16 +115,19 @@ function get_normalized_package_list {
  # Remove commas, and block scalar folded backslashes,
  # extraneous spaces at the middle, beginning and end
  # then sort.
-  local packages=$(echo "${1}" \
+  local packages
+  packages=$(echo "${1}" \
    | sed 's/[,\]/ /g; s/\s\+/ /g; s/^\s\+//g; s/\s\+$//g' \
    | sort -t' ')
-  local script_dir="$(dirname -- "$(realpath -- "${0}")")"
+  local script_dir
+  script_dir="$(dirname -- "$(realpath -- "${0}")")"

-  local architecture=$(dpkg --print-architecture)
-  if [ "${architecture}" == "arm64" ]; then
-    ${script_dir}/apt_query-arm64 normalized-list ${packages}
+  local architecture
+  architecture=$(dpkg --print-architecture)
+  if [[ "${architecture}" == "arm64" ]]; then
+    "${script_dir}"/apt_query-arm64 normalized-list "${packages}"
  else
-    ${script_dir}/apt_query-x86 normalized-list ${packages}
+    "${script_dir}"/apt_query-x86 normalized-list "${packages}"
  fi
 }

@ -127,8 +140,8 @@ function get_normalized_package_list {
 #   The relative filepath to archive.
 ###############################################################################
 function get_tar_relpath {
-  local filepath=${1}
-  if test ${filepath:0:1} = "/"; then
+  local filepath="${1}"
+  if test "${filepath:0:1}" = "/"; then
    echo "${filepath:1}"
  else
    echo "${filepath}"
@ -153,7 +166,7 @@ function validate_bool {
  if test "${1}" != "true" -a "${1}" != "false"; then
    log "aborted"
    log "${2} value '${1}' must be either true or false (case sensitive)."
-    exit ${3}
+    exit "${3}"
  fi
 }

@ -167,12 +180,12 @@ function validate_bool {
 #   Log lines from write.
 ###############################################################################
 function write_manifest {  
-  if [ ${#2} -eq 0 ]; then 
+  if [[ ${#2} -eq 0 ]]; then 
    log "Skipped ${1} manifest write. No packages to install."
  else
    log "Writing ${1} packages manifest to ${3}..."
    # 0:-1 to remove trailing comma, delimit by newline and sort.
-    echo "${2:0:-1}" | tr ',' '\n' | sort > ${3}
+    echo "${2:0:-1}" | tr ',' '\n' | sort > "${3}"
    log "done"
  fi
 }
--- a/post_cache_action.sh
+++ b/post_cache_action.sh
@ -29,12 +29,12 @@ test "${debug}" = "true" && set -x
 add_repository="${6}"

 # List of the packages to use.
-packages="${@:7}"
+packages="${*:7}"

 if test "${cache_hit}" = "true"; then
-  ${script_dir}/restore_pkgs.sh "${cache_dir}" "${cache_restore_root}" "${execute_install_scripts}" "${debug}"
+  "${script_dir}"/restore_pkgs.sh "${cache_dir}" "${cache_restore_root}" "${execute_install_scripts}" "${debug}"
 else
-  ${script_dir}/install_and_cache_pkgs.sh "${cache_dir}" "${debug}" "${add_repository}" ${packages}
+  "${script_dir}"/install_and_cache_pkgs.sh "${cache_dir}" "${debug}" "${add_repository}" "${packages}"
 fi

 log_empty_line
--- a/pre_cache_action.sh
+++ b/pre_cache_action.sh
@ -10,7 +10,7 @@ source "${script_dir}/lib.sh"
 # Setup first before other operations.
 debug="${4}"
 validate_bool "${debug}" debug 1
-test ${debug} == "true" && set -x
+test "${debug}" == "true" && set -x

 # Directory that holds the cached packages.
 cache_dir="${1}"
@ -28,7 +28,8 @@ debug="${4}"
 add_repository="${5}"

 # List of the packages to use.
-input_packages="${@:6}"
+# Use * instead of @ to concatenate array elements into a single string
+input_packages="${*:6}"

 # Trim commas, excess spaces, and sort.
 log "Normalizing package list..."
@ -36,7 +37,7 @@ packages="$(get_normalized_package_list "${input_packages}")"
 log "done"

 # Create cache directory so artifacts can be saved.
-mkdir -p ${cache_dir}
+mkdir -p "${cache_dir}"

 log "Validating action arguments (version='${version}', packages='${packages}')...";
 if grep -q " " <<< "${version}"; then
@ -47,7 +48,9 @@ fi

 # Is length of string zero?
 if test -z "${packages}"; then
-  case "$EMPTY_PACKAGES_BEHAVIOR" in
+  # shellcheck disable=SC2154
+  # EMPTY_PACKAGES_BEHAVIOR is an environment variable passed from GitHub Actions
+  case "${EMPTY_PACKAGES_BEHAVIOR}" in
    ignore)
      exit 0
      ;;
@ -66,7 +69,7 @@ fi
 validate_bool "${execute_install_scripts}" execute_install_scripts 4

 # Basic validation for repository parameter
-if [ -n "${add_repository}" ]; then
+if [[ -n "${add_repository}" ]]; then
  log "Validating repository parameter..."
  for repository in ${add_repository}; do
    # Check if repository format looks valid (basic check)
@ -100,13 +103,13 @@ log "- CPU architecture is '${cpu_arch}'."
 value="${packages} @ ${version} ${force_update_inc}"

 # Include repositories in cache key to ensure different repos get different caches
-if [ -n "${add_repository}" ]; then
+if [[ -n "${add_repository}" ]]; then
  value="${value} ${add_repository}"
  log "- Repositories '${add_repository}' added to value."
 fi

 # Don't invalidate existing caches for the standard Ubuntu runners
-if [ "${cpu_arch}" != "x86_64" ]; then
+if [[ "${cpu_arch}" != "x86_64" ]]; then
  value="${value} ${cpu_arch}"
  log "- Architecture '${cpu_arch}' added to value."
 fi
@ -119,5 +122,5 @@ log "- Value hashed as '${key}'."
 log "done"

 key_filepath="${cache_dir}/cache_key.md5"
-echo ${key} > ${key_filepath}
+echo "${key}" > "${key_filepath}"
 log "Hash value written to ${key_filepath}"
--- a/restore_pkgs.sh
+++ b/restore_pkgs.sh
@ -6,7 +6,7 @@ set -e
 # Debug mode for diagnosing issues.
 # Setup first before other operations.
 debug="${4}"
-test ${debug} == "true" && set -x
+test "${debug}" == "true" && set -x

 # Include library.
 script_dir="$(dirname -- "$(realpath -- "${0}")")"
@ -18,30 +18,33 @@ cache_dir="${1}"
 # Root directory to untar the cached packages to.
 # Typically filesystem root '/' but can be changed for testing.
 cache_restore_root="${2}"
-test -d ${cache_restore_root} || mkdir ${cache_restore_root}
+test -d "${cache_restore_root}" || mkdir "${cache_restore_root}"

 # Cache and execute post install scripts on restore.
 execute_install_scripts="${3}"

-cache_filepaths="$(ls -1 "${cache_dir}" | sort)"
-log "Found $(echo ${cache_filepaths} | wc -w) files in the cache."
+# Use find instead of ls to better handle non-alphanumeric filenames
+cache_filepaths="$(find "${cache_dir}" -mindepth 1 -maxdepth 1 -type f -o -type d | sort)"
+file_count=$(echo "${cache_filepaths}" | wc -w)
+log "Found ${file_count} files in the cache."
 for cache_filepath in ${cache_filepaths}; do
-  log "- "$(basename ${cache_filepath})""
+  log "- $(basename "${cache_filepath}")"
 done

 log_empty_line

 log "Reading from main requested packages manifest..."
-for logline in $(cat "${cache_dir}/manifest_main.log" | tr ',' '\n' ); do
+while IFS= read -r logline; do
  log "- $(echo "${logline}" | tr ':' ' ')"
-done
+done < <(tr ',' '\n' < "${cache_dir}/manifest_main.log")
 log "done"

 log_empty_line

 # Only search for archived results. Manifest and cache key also live here.
-cached_filepaths=$(ls -1 "${cache_dir}"/*.tar 2>/dev/null | sort)
-cached_filecount=$(echo ${cached_filepaths} | wc -w)
+# Use find instead of ls to better handle non-alphanumeric filenames
+cached_filepaths=$(find "${cache_dir}" -maxdepth 1 -name "*.tar" -type f 2>/dev/null | sort)
+cached_filecount=$(echo "${cached_filepaths}" | wc -w)

 log "Restoring ${cached_filecount} packages from cache..."
 for cached_filepath in ${cached_filepaths}; do
@ -51,7 +54,7 @@ for cached_filepath in ${cached_filepaths}; do
  log "  done"

  # Execute install scripts if available.    
-  if test ${execute_install_scripts} == "true"; then
+  if test "${execute_install_scripts}" == "true"; then
    # May have to add more handling for extracting pre-install script before extracting all files.
    # Keeping it simple for now.
    execute_install_script "${cache_restore_root}" "${cached_filepath}" preinst install