v1.63.0 broken on Mac (bash 3.2)

[Chris-Softrams]

Describe the bug

https://github.com/antonbabenko/pre-commit-terraform/blob/v1.63.0/hooks/_common.sh#L30 uses declare -g which is not an option in all bash versions.

.cache/pre-commit/repo574zjemm/hooks/_common.sh: line 30: declare: -g: invalid option
declare: usage: declare [-afFirtx] [-p] [name[=value] ...]

Tried with 2 different versions of bash.

/bin/bash --version
GNU bash, version 3.2.57(1)-release (x86_64-apple-darwin20)
Copyright (C) 2007 Free Software Foundation, Inc.

and

/usr/local/bin/bash --version
GNU bash, version 5.1.16(1)-release (x86_64-apple-darwin20.6.0)
Copyright (C) 2020 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>

This is free software; you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

Version v1.62.3 works fine.

[yermulnik]

GNU bash, version 3.2.57(1)-release

This is quite ancient version of Bash, whereas newer and modern Bash is available on MacOS by means of MacPorts, Homebrew, or Fink.
I'd highly recommend to upgrade to Bash 4 at least. It ships a huge bunch of various improvements.

GNU bash, version 5.1.16(1)-release

Are you sure you had bash version 5 in your path preceding older bash binary?
What's the output of $(which bash) --version | head -1?

I've checked on several different servers with different OS (Ubuntu, FreeBSD, Amazon Linux) with different versions of Bash (4 and 5) and on all of them declare had -g option implemented. E.g.

> bash --version | head -1 && help declare | egrep -A1 '[[:space:]]-g[[:space:]]'
GNU bash, version 5.1.8(1)-release (x86_64-pc-linux-gnu)
      -g        create global variables when used in a shell function; otherwise
                ignored

and

$ bash --version | head -1 && help declare | egrep -A1 '[[:space:]]-g[[:space:]]'
GNU bash, version 4.2.46(2)-release (x86_64-redhat-linux-gnu)
      -g        create global variables when used in a shell function; otherwise
        ignored

[MaxymVlasov]

As I know, in new versions macOS use zsh by default and include bash 3.2 for compatibility reasons.
That OS never will upgrade bash to 4+, during to license issues.
https://www.theverge.com/2019/6/4/18651872/apple-macos-catalina-zsh-bash-shell-replacement-features

So looks like we need to maintain compatibility with bash 3.2 🙄

[Chris-Softrams]

@MaxymVlasov I even tried with https://formulae.brew.sh/formula/bash and setting my SHELL to /usr/local/bin/bash

[yermulnik]

So looks like we need to maintain compatibility with bash 3.2 🙄

Or include Bash >= 4 as requirement in README =)

UPD: some hooks require external deps — why wouldn't we require modern Bash also?

[yermulnik]

@MaxymVlasov I even tried with https://formulae.brew.sh/formula/bash and setting my SHELL to /usr/local/bin/bash

You'd need to update your PATH env var so that /usr/local/bin precedes /usr/bin (or which ever bash3 resides in).

[MaxymVlasov]

By the way, @Chris-Softrams can you test that it works fine with bash 3.2?

repos:
- repo: https://github.com/antonbabenko/pre-commit-terraform
  rev: 48bc03ca3f0f2f782d2f430069868019a6892062
  hooks:
    - id: terraform_fmt

[robinbowes]

https://github.com/antonbabenko/pre-commit-terraform/blob/v1.63.0/hooks/_common.sh#L30 uses declare -g which is not an option in all bash versions.

That's relatively easy to fix.

declare -g declares a variable to be global when used inside a function. To work on Bash 3.2, simply move the variable declaration outside the function - eg, move to the top of the file. Arguably, that's clearer than declaring a global var in a function anyway.

Move this to the top of the file:

  # common global arrays.
  # Populated via `common::parse_cmdline` and can be used inside hooks' functions
  declare -a ARGS=() HOOK_CONFIG=() FILES=()

[ocofaigh]

Recently ran into the same issue recently, and came across this:

# For bash 4.x, must not be in posix mode, may use temporary files
mapfile -t array < <(mycommand)

# For bash 3.x+, must not be in posix mode, may use temporary files
array=()
while IFS='' read -r line; do array+=("$line"); done < <(mycommand)

# For ksh, and bash 4.2+ with the lastpipe option enabled (may require disabling monitor mode)
array=()
mycommand | while IFS="" read -r line; do array+=("$line"); done

I had to change my scripts to use option 2 instead of option 1 in order to be compatible on mac with enforcing a bash upgrade. Suggest to do the same here.

[drewmullen]

48bc03ca3f0f2f782d2f430069868019a6892062

This worked for me, @MaxymVlasov . I was having the same error as the issue outlines when upgrading to from v1.62 to v1.64

$ uname -a
Darwin 3c22fb40eee8 19.6.0 Darwin Kernel Version 19.6.0: Thu Jan 13 01:26:33 PST 2022; root:xnu-6153.141.51~3/RELEASE_X86_64 x86_64

$ echo $SHELL
/bin/zsh

$ bash -c 'echo ${BASH_VERSION}'
3.2.57(1)-release

[yermulnik]

Just to compile main items that are required to keep compatibility with Bash v3:

• builtins/utilities missing from Bash v3 or from base OS:
  • decalare -g: code changes → drop declare -g and (possibly) use export to populate var(s) globally where applicable and if needed
  • mapfile: fallback to read -a (looking at the code, where mapfile is used, we should be able to drop use of array at all (we only need the last item from that array, which can be accomplished by using tail -1)
  • realpath:
    • not enabled in Bash by default (requires enable -f realpath realpath to enable)
    • not built and enabled in Bash less than v4 by default
    • code changes:
      • bash version is equal or greater than 4 → enable this builtin
      • bash version is less than 4 and realpath binary is missing from PATH → implement realpath emulation via function and populate all shell hooks with it via e.g. _common.sh
        • alternatively can be "enabled" by installing coreutils using package manager on Linux and/or Homebrew on MacOS and Linux
    • is already in the list of requirements for MacOS along with Homebrew (requirements installation steps for MacOS assume brew is already installed)
    • is used by us in conjunction with dirname, which is part of coreutils at least on Ubuntu (is this available from base on other Linux distros and MacOS?)
    • alternatively use quirks like cd -P -- "${PATH_TO_DIR_WITH_HOOK}" && pwd -P
• all contributors are aware of restriction to use modern bash features which are not implemented in Bash v3 (http://tiswww.case.edu/php/chet/bash/NEWS) and of restriction to rely on common core packages like coreutils
• all contributors are aware of possible behavioral differences of common builtins and utilities (http://tiswww.case.edu/php/chet/bash/NEWS)
• all contributors have an option to test their code using Bash v3 (e.g. with respective Docker container — available at https://hub.docker.com/_/bash?tab=tags)
• all PRs must be automatically verified to be compatible with Bash v3 with e.g. GitHub workflow
• PR reviewers should bear in mind the above
• what else?

My only concerns are the items apart from the very first one and the need to support quite ancient version of shell interpreter.

Alternatively we can enforce Bash v4/v5 requirement. I realize this is intrusive for MacOS folks who are sort of enforced to use pre-commit (e.g. where corporate policy puts a requirement to use pre-commit-terraform for repos with Terraform code in the Company), though yet we already require them to install a bunch of tools and having one another relatively simple step like the below should be not a big deal:

/bin/bash -c "$(curl -fsSL https://raw.githubusercontent.com/Homebrew/install/HEAD/install.sh)" && \
    brew install bash && \
    # as far as I remember Homebrew on MacOS links binaries into /usr/local/bin
    export PATH="/usr/local/bin${PATH+:$PATH}" && \
    echo 'export PATH="/usr/local/bin${PATH+:$PATH}"' >> ~/.bashrc

The curl step is not required if Homebrew was already installed.

➡️ IMHO

[artis3n]

rev 48bc03ca3f0f2f782d2f430069868019a6892062 resolved this error for me on OSX 12.2.1

- repo: https://github.com/antonbabenko/pre-commit-terraform
   rev: 48bc03ca3f0f2f782d2f430069868019a6892062
   hooks:
     - id: terraform_fmt
     - id: terraform_docs
       args:
         - '--args=--sort-by required'

[sblack4]

I was able to get around this with a brew install bash. It looks like bash 5 is in Homebrew:

➜ brew info bash                                                                    
bash: stable 5.1.16 (bottled), HEAD
Bourne-Again SHell, a UNIX command interpreter
https://www.gnu.org/software/bash/
/usr/local/Cellar/bash/5.1.16 (157 files, 10.9MB) *
  Poured from bottle on 2022-03-07 at 15:14:01
From: https://github.com/Homebrew/homebrew-core/blob/HEAD/Formula/bash.rb
License: GPL-3.0-or-later
==> Options
--HEAD
        Install HEAD version
==> Analytics
install: 25,150 (30 days), 100,366 (90 days), 331,470 (365 days)
install-on-request: 20,579 (30 days), 82,799 (90 days), 276,640 (365 days)
build-error: 33 (30 days)

[antonbabenko]

This issue has been resolved in version 1.64.1 🎉