Skip to content

Fixes recent scripting breeze fix to work also with zsh#14787

Merged
potiuk merged 1 commit intoapache:masterfrom
potiuk:fix-mistakenly-added-variable-name-in-breeze
Mar 15, 2021
Merged

Fixes recent scripting breeze fix to work also with zsh#14787
potiuk merged 1 commit intoapache:masterfrom
potiuk:fix-mistakenly-added-variable-name-in-breeze

Conversation

@potiuk
Copy link
Member

@potiuk potiuk commented Mar 15, 2021

The BASH variable introduced in #14579 is not set when the main shell is
zsh on MacOS (which is the default) this PR changes it so that the
output of command -v bash is used instead.

Fixes #14754


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

The BASH variable introduced in apache#14579 is not set when the main shell is
zsh on MacOS (which is the default) this PR changes it so that the
output of `command -v bash` is used instead.

Fixes apache#14754
@potiuk potiuk force-pushed the fix-mistakenly-added-variable-name-in-breeze branch from d3e8647 to 396f540 Compare March 15, 2021 02:40
@github-actions
Copy link

The PR is likely ready to be merged. No tests are needed as no important environment files, nor python files were modified by it. However, committers might decide that full test matrix is needed and add the 'full tests needed' label. Then you should rebase it to the latest master or amend the last commit of the PR, and push it with --force-with-lease.

@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Mar 15, 2021
@potiuk potiuk merged commit c613384 into apache:master Mar 15, 2021
@potiuk potiuk deleted the fix-mistakenly-added-variable-name-in-breeze branch March 15, 2021 09:58
@ashb
Copy link
Member

ashb commented Mar 15, 2021

Curious. This script should only ever be run with bash, even when ZSH is the shell because of the #!/usr/bin/env bash shebang ling.

@potiuk
Copy link
Member Author

potiuk commented Mar 15, 2021

True, in theory it should - https://tldp.org/LDP/abs/html/internalvariables.html, however I've learned not to trust MacOS environment and be a little defensive (I am not 100% sure what was the scenario that led to BASH env missing - but taking into account the large variations of Bash's there for MacOS (the built-in 3.2, any version that can be installed by brew etc.) - command -v bash is all but guaranteed to work.

But yeah - it might be an interesting question how it got there.

potiuk added a commit that referenced this pull request Mar 23, 2021
The BASH variable introduced in #14579 is not set when the main shell is
zsh on MacOS (which is the default) this PR changes it so that the
output of `command -v bash` is used instead.

Fixes #14754

(cherry picked from commit c613384)
ashb pushed a commit that referenced this pull request Apr 15, 2021
The BASH variable introduced in #14579 is not set when the main shell is
zsh on MacOS (which is the default) this PR changes it so that the
output of `command -v bash` is used instead.

Fixes #14754

(cherry picked from commit c613384)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dev-tools okay to merge It's ok to merge this PR as it does not require more tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Breeze fails to run on macOS with the old version of bash

3 participants