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.

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

SDK issues with 'set -eu' #597

Open
andretadeu opened this issue Aug 20, 2017 · 5 comments · May be fixed by #1269
Open

SDK issues with 'set -eu' #597

andretadeu opened this issue Aug 20, 2017 · 5 comments · May be fixed by #1269

Comments

@andretadeu
Copy link

Hi!

At my job I am maintaining several shell scripts created to help software developers to be more productive and I was evaluating the use of SDKMAN to save some work. In order to clean up those scripts, I am using set -eu (and other flags that are not relevant at the moment) to be sure that the scripts have all variables it uses and avoid problems with outdated variables being called in some old scripts that might do something unexpected. However, I am facing some difficulties in using sdk commands because they rely on unbound variables. For example:

sdkman-sdk-list-fail

Now, let us try installing Kotlin:

sdkman-sdk-install-kotlin-fail

PS: I am working on this fix, as this commit in my fork https://github.com/andretadeu/sdkman-cli/commit/fd26629e2135c55facc2d31f60f7b9ad64809151 shows. However I need to ensure that the tests are correct. If I don't set set -eu, all the tests passes, however if I do so, I get some errors and some skipped tests, therefore I am not confident about creating a Pull Request.

PS2: If you have time, could you give an explanation on how these tests works?

Thanks in advance,

André Tadeu de Carvalho
@marc0der
Copy link
Member

I've personally never used the -u flag for the set builtin, but this is undoubtedly what is breaking your shell. Is there a reason why you really need this? Reading the man page:

-u Treat unset variables and parameters other than the special parameters ‘@’ or ‘*’ as an error when performing parameter expansion.

This is not default shell behaviour and something that we rely on heavily when passing function parameters. In the context of our CLI, I am not really comfortable building in such defensive code, especially since this should never happen under normal circumstances. Hope this makes sense to you.

Regarding the tests, we have a mixture of cucumber specifications (which has step defs acting upon a small bash environment), as well as some Spock specifications for more fine-grained testing. It's quite complex, so I suggest working your way through the test code from the top down.

@marc0der
Copy link
Member

Not heard anything back after my previous reply so closing this issue for now. If further discussion is required, please raise it in our Gitter user-issues chat room.

@steinybot
Copy link

@marc0der I am also encountering this problem.

In my particular example I am encountering this trying to use SDKMAN with direnv. I know SDKMAN has the sdkman_auto_env=true option but I prefer to use direnv for this as I am using it for other things.

The solution seems rather straightforward, replace -n "$ZSH_VERSION" with -n "${ZSH_VERSION+x}" for example. Would you accept a PR for this?

@marc0der marc0der reopened this May 8, 2020
@marc0der
Copy link
Member

marc0der commented May 8, 2020 •
edited
Loading

Yes, PRs are always welcome. We just need to make sure that it doesn't trip up shellcheck.

@rgoldberg
Copy link
Contributor

rgoldberg commented May 8, 2020 •
edited
Loading

Any shell, not just zsh, can have a variable named ZSH_VERSION, so checking for the presence of a ZSH_VERSION variable as a shell detection heuristic is flawed.

A better heuristic would be seeing if bash recognizes a certain command as a builtin, but zsh doesn’t. If so, then it’s bash; if not, it’s zsh, since those are the only supported shells for now.

I have a commit to change the sdkman shell detection heuristic to usingshopt as the bash, not zsh, builtin, but it’s in a later commit in my queue.

My queue also contains fixes for types of issues other people have been reporting recently, too.

This was referenced Jan 10, 2024
Merged
Closed
AlexanderYastrebov added a commit to AlexanderYastrebov/sdkman-cli that referenced this issue Jan 11, 2024
@AlexanderYastrebov
Support set -eu
0bf8782 
Do not fail when parent script has `set -u` option.

See
* https://www.gnu.org/software/bash/manual/html_node/The-Set-Builtin.html
* https://www.gnu.org/software/bash/manual/html_node/Shell-Parameter-Expansion.html
* http://mywiki.wooledge.org/BashFAQ/112

Fixes sdkman#597
@AlexanderYastrebov AlexanderYastrebov linked a pull request Jan 11, 2024 that will close this issue
Open
2 tasks
AlexanderYastrebov added a commit to AlexanderYastrebov/sdkman-cli that referenced this issue Jan 11, 2024
@AlexanderYastrebov
Support set -u in parent script
9bb3774 
Do not fail when parent script has `set -u` option.

See
* https://www.gnu.org/software/bash/manual/html_node/The-Set-Builtin.html
* https://www.gnu.org/software/bash/manual/html_node/Shell-Parameter-Expansion.html
* http://mywiki.wooledge.org/BashFAQ/112

Fixes sdkman#597
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Support set -u in parent script AlexanderYastrebov/sdkman-cli
4 participants
@marc0der @rgoldberg @andretadeu @steinybot