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

Fix Bash4 behavior with set -o nounset #38

Merged
merged 1 commit into from
Jan 18, 2020

Conversation

mikynov
Copy link
Contributor

@mikynov mikynov commented Jan 15, 2020

This PR fixes inconsistent behavior between Bash 4.4 and lower versions while using set -o nounset.

I believe set -o nounset is very helpful for more complex runnerfiles and this PR should address two things:

  1. Unbound variables in runner_bootstrap and runner_run_task
  2. Empty task name in runner_bootstrap when called without parameters (i.e. default task should be invoked)

Example runnerfile.sh:

#!/usr/bin/env bash
cd "$(dirname "$0")" || exit
source src/runner.sh

set -o xtrace
set -o errtrace
set -o nounset
set -o pipefail

task_default() {
    runner_log "I'm default task: ${*}"
}

task_example() {
    runner_log "I'm example task: ${*}"
}

Using Bash version prior to 4.4 the runnerfile.sh was failing with either "unbound variable" or "Task '' is not defined!".

Tested in Bash 4.0, 4.1, 4.2, 4.3, 4.4 and 5.0 from https://hub.docker.com/_/bash.

Please consider merging this because Bash versions < 4.4 are still pretty common.

@stylemistake
Copy link
Owner

Looks a bit hacky. Could it all be fixed by properly initializing those vars?

@mikynov
Copy link
Contributor Author

mikynov commented Jan 16, 2020

Fine, I should have done better investigation...

I've found the SO answer about this, containing also reasoning and link to Bash changelog. It looks like "unbound variable" error is expected behavior in Bash < 4.4 while referencing an empty array with arr[@].

Suggested workaround looks "hacky" as well but works better:

Use ${arr[@]+"${arr[@]}"} instead of "${arr[@]}".

@stylemistake
Copy link
Owner

Oof. Well, I guess I'll merge it like that.

@stylemistake stylemistake merged commit 54b1353 into stylemistake:master Jan 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants