Skip to content

fix: remove pipefail from standard shell options #3269

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 28, 2022
Merged

fix: remove pipefail from standard shell options #3269

merged 1 commit into from
Jul 28, 2022

Conversation

denbeigh2000
Copy link
Contributor

Small one I found while checking out the OSS product.

Setting pipefail requires the -o flag, otherwise it has no effect.

Validation:

$ sh
sh-5.1$ set -eux pipefail
sh-5.1$ false | echo 'hello'
+ echo hello
hello
+ false
sh-5.1$ echo $?
+ echo 0
0

$ sh
sh-5.1$ set -eux -o pipefail
sh-5.1$ false | echo 'hello'
+ echo hello
hello
+ false

[sh exits, parent shell]

$ echo $?
1

@kylecarbs kylecarbs requested a review from mafredri July 28, 2022 17:45
@mafredri
Copy link
Member

mafredri commented Jul 28, 2022 •
edited
Loading

Thanks for catching this @denbeigh2000!

Actually, I think we should remove pipefail entirely, it's somewhat of a bashism and not supported in POSIX shells (we try to maintain compatibility with all shells, as much as possible).

Would you mind making the change?

@denbeigh2000
bug: remove -o from pipefail options
5f579ad 
This isn't well-supported by every POSIX shell anyways.
@denbeigh2000
Copy link
Contributor Author

@mafredri For sure! Done and done.

@denbeigh2000 denbeigh2000 changed the title bug: add -o to shell pipefail options bug: remove pipefail from shell pipefail options Jul 28, 2022
@denbeigh2000 denbeigh2000 changed the title bug: remove pipefail from shell pipefail options bug: remove pipefail from standard shell options Jul 28, 2022
kylecarbs
kylecarbs approved these changes Jul 28, 2022
View reviewed changes
Copy link
Member

@kylecarbs kylecarbs left a comment

Choose a reason for hiding this comment

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

LGTM!

mafredri
mafredri approved these changes Jul 28, 2022
View reviewed changes
Copy link
Member

@mafredri mafredri left a comment

Choose a reason for hiding this comment

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

Great, thanks again!

@mafredri mafredri changed the title bug: remove pipefail from standard shell options fix: remove pipefail from standard shell options Jul 28, 2022
@mafredri mafredri enabled auto-merge (squash) July 28, 2022 18:34
@mafredri mafredri merged commit 43b8cf0 into coder:main Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mafredri mafredri mafredri approved these changes

@kylecarbs kylecarbs kylecarbs approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@denbeigh2000 @mafredri @kylecarbs