Skip to content

check_for_upgrade.sh not executable with /bin/sh #1856

Closed
wants to merge 4 commits into from

5 participants

@MatthiasLohr

check_for_upgrade.sh is not executable with /bin/sh while using bash syntax, so the execution shell should be set to /bin/bash.

@urevic
urevic commented Jun 3, 2013

Probably it is better to change it to zsh instead of bash, as we can't be sure that bash is installed everywhere?

@MatthiasLohr

Yes, of course, my fault.

Also the env command should be used, because the zsh binary is not always in the same place across different systems (e.g. /usr/bin/zsh is the location on my notebook).

@mcornella
Collaborator

Hi @MatthiasLohr, this is a cool PR, but I want to run a few tests first. Be aware though that check_for_upgrade.sh is executed from upgrade_oh_my_zsh function, which bypasses the shebang. Also, the script is fully compatible with /bin/sh now.
If you want to proceed with this PR, could you also add the appropriate shebang for the install.sh script (#1623) since both are very similar issues?

@MatthiasLohr

Done.

@mcornella
Collaborator

Cool!

@mcornella mcornella referenced this pull request Jul 11, 2014
Closed

Closable issues hunting #2568

@mcornella
Collaborator

Hi @MatthiasLohr, we're gonna proceed with this PR, I need a couple of last favors from you:

  • Add the statement Closes #1623 in your description so that one is automatically closed, and add its modifications to the README file too: there are sh in lines 17, 21 and 31. Also, in line 31 the URL should be http://install.ohmyz.sh too, like the rest of them.
  • Change the PR's title to something more general; we're using zsh everywhere now.
  • Also rebase against the current changes too.

Can we get some testers here? /cc @ncanceill @Kriechi

@ncanceill

Will test shortly.

@MatthiasLohr please remember to rebase against current upstream, and take the opportunity to squash your commits in order to keep the history clean and atomic.

@mcornella
Collaborator

Hey @MatthiasLohr, can you get some time to rebase? The change in the README file from sh to zsh need to happen too. See my previous comment too: #1856 (comment)

@MatthiasLohr

Hey @mcornella, i'll try that, but at the moment i'm very busy, sry.

@mcornella
Collaborator
@mcornella
Collaborator

Hi @MatthiasLohr, any luck on finishing this up?

@apjanke
apjanke commented Feb 28, 2015

Couple questions:
Is #!/usr/bin/env zsh the right shebang line for install.sh? That is run under sh during the install process, not zsh. So it needs to be kept to just sh-compatible syntax. Unless you're planning on changing the install process to run it through zsh instead?

And how about renaming check_for_upgrade.sh to check_for_upgrade.zsh? Same for theme_chooser.sh to theme_chooser.zsh, since it has a zsh shebang. That way all the file extensions and shebang lines are all consistent with the shell flavor the files are targeting.

@mcornella
Collaborator

Sorry for the late late response, this fell off the tracks. I'm going to build upon this PR to make the transition to a full-zsh codebase, but for the moment i'm closing this as there are many things to take into account.

Again, sorry for losing track of this PR and thank you for your all your work.

@mcornella mcornella closed this Dec 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.