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 the script when sourced from zsh #208

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wilkmar
Copy link

@wilkmar wilkmar commented Jul 25, 2024

When the novarc file is sourced from the Zsh, it fails due to a few Bash specifics:

  • $BASH_SOURCE - does not exist in Zsh
  • readarray - does not exist in Zsh

Resolves issue #176

@dosaboy
Copy link
Member

dosaboy commented Jul 25, 2024

this looks like it might now work in bash?

@wilkmar
Copy link
Author

wilkmar commented Jul 25, 2024

this looks like it might now work in bash?

I tested it in both bash and zsh, both when sourced or executed directly (however don't see the use case for the latter one). Also ran ./configure with sources novarc and haven't noticed any issues

@pponnuvel
Copy link
Member

pponnuvel commented Jul 25, 2024

There are a number of differences between zsh and bash. So it's going to be difficult for a script to work in both shells.

A simple workaround is (assuming user starts in zsh):

bash # switch to bash shell
source novarc # do it
zsh # new zsh shell; exported vars will be available in the subshell (zsh)
## will have to exit twice of course

If that's not acceptable, and zsh support is absolutely required, perhaps we can make two: novarc.bash and novarc.zsh?

@wilkmar
Copy link
Author

wilkmar commented Jul 25, 2024

There are a number of differences between zsh and bash. So it's going to be difficult for a script to work in both shells.

A simple workaround is (assuming user starts in zsh):

bash # switch to bash shell
source novarc # do it
zsh # new zsh shell; exported vars will be available in the subshell (zsh)
## will have to exit twice of course

If that's not acceptable, and zsh support is absolutely required, perhaps we can make two: novarc.bash and novarc.zsh?

IMHO only the changes I am proposing in the PR are needed to make it work on both bash and zsh. I have tested this for both shells and haven't noticed problems but if more testing is needed I am happy to do that.
Unfortunately, the proposed workaround is not an option, since the novarc is sourced from a number of other scripts.

@pponnuvel
Copy link
Member

IMHO only the changes I am proposing in the PR are needed to make it work on both bash and zsh. I have tested this for both shells and haven't noticed problems but if more testing is needed I am happy to do that.

I don't see any issues - just saying that keeping script written in X working in Y is always going to be hard when X & Y aren't feature compatible. If we go with the PR as is, a comment might be useful so that future travellers know they should keep it compatible with zsh.

Unfortunately, the proposed workaround is not an option, since the novarc is sourced from a number of other scripts.

That shouldn't be an issue as every script that sources it is a bash script. The only issue here is that sourcing novarc from a user's shell is the problem because user's shell is zsh. So zsh -> bash -> zsh should work.


Alternatively, novarc can be modified to produce those variables and that can be sourced in any shell. Example:

bash_cmd='
list=(V1 V2 V3)

V1="111 A B C"
V2="222"
V3="333"

for v in "${list[@]}"; do
    printf "export $v=\"${!v}\"\n"
done
'

eval $(bash -c "$bash_cmd")

When the novarc file is sourced from the Zsh, it fails due to a few
Bash specifics:
- $BASH_SOURCE - does not exist in Zsh
- readarray - does not exist in Zsh

Resolves issue canonical#176

Signed-off-by: Marcin Wilk <[email protected]>
@wilkmar
Copy link
Author

wilkmar commented Jul 26, 2024

Hi @pponnuvel I got your point about the need to maintain the script for both shells. I have added the comment as you suggested.
Unfortunately, not all scripts use bash in the shebang. The .configure uses #!/bin/sh -u, and that is causing issues too. Maybe it's worth to change that to bash at the same time?

@pponnuvel
Copy link
Member

Thanks @wilkmar . Yes, it makes sense to change the shebang in openstack/configure to bash. In practice, it doesn't cause any issues because when sourcing a script, the shebang has no meaning. But for consistency/clarity, it makes sense to change it to bash.

There's also one more in jaas/configure.

@wilkmar
Copy link
Author

wilkmar commented Jul 26, 2024

jaas/configure

the openstack/configure is meant to be executed directly (not sourced), which is an issue because indirectly it sources novarc. Not sure what about the jaas/configure (I haven't used it so far) but I suspect the same.

@pponnuvel
Copy link
Member

jaas/configure

the openstack/configure is meant to be executed directly (not sourced), which is an issue because indirectly it sources novarc. Not sure what about the jaas/configure (I haven't used it so far) but I suspect the same.

Regardless, you can change the shebang in both.

@wilkmar
Copy link
Author

wilkmar commented Jul 26, 2024

ACK but rather I will make it a separate PR

Copy link
Member

@pponnuvel pponnuvel left a comment

Choose a reason for hiding this comment

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

LGTM

@wilkmar
Copy link
Author

wilkmar commented Jul 26, 2024

Created PR #209

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.

3 participants