Respect original PATH definitions on install

[avit]

This is a solution for #1359.

[avit]

@mcornella, @robbyrussell can I get a review on this, I think it solves a few longstanding issues:

#1359
#2586
#1121

[mcornella]

Hi @avit, first of all, I'm not an "official" project mantainer, but it's an honor to be put on the same list :)
That said, of course I'll review this and give my opinion. I'm not on my Linux right now, so I cannot test it hands on; I'll update whenever I have.

My 2 cents for now:

1. I pretty much agree with @mpapis in Do not force PATH in the installer  #1359 that an automatic PATH generates more issues than those it tries to solve. If I had to decide, I'd just print a message reminding to update their PATH.
2. If you look in .bashrc and .zshrc, why not look too in a .bash_profile, or a .tcshrc, etc.. Pardon my logical fallacy here, but I think we are being too nanny-ish.
   A command line is very complex and has a lot of intricacies, we want to make it simple for new users but in the end we are just adding complexity, which is a bad thing. It's easier to see a command not found error and find out you have to set your $PATH than to figure out why a PATH is not correctly set (i.e., in a particular order or without duplicates).
3. Following up on the latter, I think a custom $PATH should be set ultimately on your .profile, so I don't think we have a responsibility to manage that.

More particularly about the PR:

1. The $PATH should be set before sourcing oh-my-zsh.sh, just in case some plugin needs an executable that is not in the default path. That has happened to me and some other people I helped resolve a X: command not found error. The current sed doesn't do that, and I think it would be a nice improvement if you could set it at the top.
2. Duplicates are easily removed: typeset -U path defines path as an array with unique values. If we set the path using path+=($PATH dir1 dir2) that will solve that. Easily said than done, but it's a suggestion.
3. Lastly, I had no idea something as ed existed. I see it is standard in UNIX systems, so I guess it's fine. Just make sure the syntax you use is platform independent.

I'll post more when I have that tested.
Thanks for your effort! 😄

[avit]

Good points, thanks. I also agree with you on this, but #1359 was closed for similar reasons so I'm trying to offer a solution. If we're going to set PATH for new users, this approach would be less broken than the current setup. I agree it can be improved to look for setenv in csh and be more thorough in other ways, but we have to draw the line somewhere.

(ed is like sed but without the "stream" part which always confuses me... I find it's just easier to use when editing a file in place.)

[mcornella]

Surprisingly, on my Debian system (testing), ed is not installed by default. The equivalent of your command in sed would be...?

[mcornella]

This outputs tools/zshrc_test: file not found. You probably meant ~/.zshrc?

[avit]

Yes, thanks. That's an oversight from testing.

[avit]

Wow, it's surprising that a system wouldn't have ed installed, it's one of the old core Unix commands... I'll rework this when I have a minute.

[mcornella]

This need to be $ORIG_PATHS

[mcornella]

Cool! With those changes and ed installed works perfectly.

[mpapis]

sed is the standard, I never seen ed to be used before

[avit]

http://wiki.bash-hackers.org/howto/edit-ed

@mpapis Not arguing, I'll change it, but ed has been around forever. Just about every system has it in /bin/ed (except debian now has it "optional" as we've learned). ed is basically vi commands without the visual part, it's easier and clearer to understand for many things. sed is... different.

[avit]

I had another look at this today but I'm not too sure of the right way to do this in sed or awk.

If we capture $ORIG_PATHS from grep as a multi-line string with a set of characters to be escaped ($``'``"``\n...), what's the most robust way to pass that into the replace string for sed? (or awk?)

# captured output with newlines:
ORIG_PATHS='export PATH="$PATH:/foo/bar/baz"
export PATH="$PATH:/qux"
'

# attempted replacement, using '|' instead of '/' for delimiters due to slashes in paths:
sed -e 's|^export PATH=.*|'"$ORIG_PATHS"'|' zshrc
# sed: 1: "s|^export PATH=|export  ...": unescaped newline inside substitute pattern

Can anyone help with the quoting/escaping please? I also tried escaping using printf '%q' "$ORIG_PATHS" but that comes with other "unterminated substitute" errors in sed. I've also read there are some differences between BSD sed (Mac OS X) vs. GNU sed for escaping newlines.

[mpapis]

try:

sed -e '/^export PATH=.*$/ c '"$ORIG_PATHS"'
' zshrc

there is supposed to be new line ... but it might work without it too

[avit]

Hm. That doesn't quite work either. c needs a backslash followed by a newline, so:

sed -e '/^export PATH/ c\
'"$ORIG_PATHS" zshrc

But then the newlines in $ORIG_PATHS break it so it still needs more escaping. Now I have this double-sed which seems to work:

sed -e '/^export PATH=/ c\
'"$(echo $ORIG_PATHS | sed -e 's/$/\\/')" zshrc

But is that a sane way to do it?

[mpapis]

sed -e '/^export PATH=/ c\
'"${ORIG_PATHS//\n/\\\\n}" zshrc

use shell variable replacement, not sure about the escapes in the second part - 3 or 4 should do it.

[avit]

@mpapis Thanks for your help, but I still can't seem to make it replace the newlines. This works with normal characters:

echo "${ORIG_PATHS//x/AAA}"

But does not catch the newlines:

echo "${ORIG_PATHS//\n/AAA}"

[mpapis]

ah yes, this will do it:

echo "${ORIG_PATHS//$'\n'/\\\\n}"

[mpapis]

but the last one will not work with dash (works fine with bash/zsh) so use it only if the script is run from zsh - you could add on top:

#!/usr/bin/env zsh

[avit]

@mpapis I found a solution for replacing newlines that works across all the shells by putting the newline into a variable. I couldn't find a better way unless you have any other suggestions.

# works in bash, sh but not in zsh
`"${ORIG_PATHS//$'\n'/\\$'\n'}"`

[mpapis]

ah yes I use the variable trick in rvm => https://github.com/wayneeseguin/rvm/blob/master/scripts/functions/support#L402-L403

[avit]

@robbyrussell This should be good to merge, can you give a review?

[ncanceill]

Not tested but looks good to me.

However, can you please squash your commits to keep the history clean and atomic?

[mcornella]

Hey @avit, just tested it again and it fails when there are no export statements or .bashrc and .zshrc.pre-oh-my-zsh don't exist. This coupled with the set -e statement at the beginning of the script make the install fail and exit before having a complete install.

I'm talking about line 28, and I was using bash. Grep returns a non-zero value when it doesn't find something, we can solve it adding || : or || true at the end.

[avit]

Thanks, I'll have another look before we merge this then.

[LandonSchropp]

I just wasted about an hour trying to figure out why my rbenv wasn't being loaded correctly before I realized Oh My Zsh was overwriting the $PATH variable. 👍 to fixing this.

[robbyrussell]

@avit Hi there, have you had a chance to update this?

[avit]

@robbyrussell The maintainer lives! Yeah, I haven't yet but I'll look at it again this week.

[mpapis]

it only required other tool to find the same bug as reported 2 years ago

[avit]

@robbyrussell I rebased this on master & fixed to handle cases for missing files or missing PATH declarations.

[mcornella]

Hi @avit, whenever I have the time I'll test this again (I don't remember what tests I tried yet)

[nerdy-sam]

To add to the sed portability discussion, I'd invite y'all to checkout my pull request to see how to accomplish your goals while still being compatible with operating systems that run standard sed, like OpenBSD or NetBSD. #3257

TL;DR - Standard sed doesn't have the "-i" option.

[avit]

Thanks for that @mitzip, I've updated my branch for this PR to include that.

[nerdy-sam]

No problem @avit, though, @mpapis pointed out a potential problem in my PR. If the install file gets over 4096 bytes (it's 1900 or so now), it would overflow sed's buffer and cause the tee output to be corrupt. The solution is using a temp file, which is what GNU sed does when passed the "-i" option. I've updated my PR, I suggest you yours as well. :-) You were just too quick on the draw, :-D

[avit]

Ha, ok, that's good to know. I was wondering why not output redirection (> ~/.zshrc) ...but I guess a file can't just overwrite itself while it's being read.

[LandonSchropp]

It seems a little wrong to copy the user's path to the .zshrc file on installation. What if the user's path changes? When I'm setting up a new system, Oh My ZSH is one of the first things I install. Then, I install a bunch of other tools and update my path. With the way this is implemented, wouldn't it break that?

Instead, why not append the additional directories to the existing path, like this?

export PATH=<ZSH PATH DIRECTORIES>:$PATH

Or am I missing something? Sorry if this is an ignorant comment. 😃

[mpapis]

@LandonSchropp this is response to rejected #1359 - a middle ground, preserve PATH from previous *rc files, do not hard code it

[ncanceill]

So what's the status here? Is it ready for merge?

[mcornella]

I think it is but, to be honest, I haven't tested the newest versions of the patch. Will do this weekend hopefully

[robbyrussell]

Appreciate all of your work on getting us this far on this. Once we get a chance to do some tests, we'll get this merged in.

[apjanke]

Looks like this fixes #3612 and #3367 as well.

[ncanceill]

@avit can we please get a rebase here ?

[apjanke]

Hmm. I don't think this use of tee works. When I ran it with an existing ~/.zshrc on OS X 10.9.5, this version of the installer spews a copy of the new zshrc to the terminal output, as pictured below. And when I ran it without a .zshrc already, I ended up with an empty one after OMZ installation. I ran it a few more times with and without an existing .zshrc, and I'm mostly ending up with an empty one after the installer runs.

This is a variation on what @mitzip said. When you do a pipeline like sed -e '...' file | tee file, you have a race condition. The shell will start up the programs in both sections of the pipeline. sed will try to read the file, and tee will try to write it (opening the file for output even before receiving any input, and truncating it). In my case, it looks like tee is often winning the race and writing ~/.zshrc before sed gets a chance to read it, so sed reads a 0-byte file.

This needs to be replaced with temp files.

eilonwy% rm .zshrc
rm: .zshrc: No such file or directory
eilonwy% cat install-avit.sh | sh
Cloning Oh My Zsh...
Cloning into '/Users/janke/.oh-my-zsh'...
remote: Counting objects: 12497, done.
remote: Total 12497 (delta 0), reused 0 (delta 0), pack-reused 12497
Receiving objects: 100% (12497/12497), 2.40 MiB | 4.37 MiB/s, done.
Resolving deltas: 100% (6851/6851), done.
Checking connectivity... done.
Looking for an existing zsh config...
Using the Oh My Zsh template file and adding it to ~/.zshrc
         __                                     __
  ____  / /_     ____ ___  __  __   ____  _____/ /_
 / __ \/ __ \   / __ `__ \/ / / /  /_  / / ___/ __ \
/ /_/ / / / /  / / / / / / /_/ /    / /_(__  ) / / /
\____/_/ /_/  /_/ /_/ /_/\__, /    /___/____/_/ /_/
                        /____/                       ....is now installed!


 Please look over the ~/.zshrc file to select plugins, themes, and options.


 p.s. Follow us at http://twitter.com/ohmyzsh.


 p.p.s. Get stickers and t-shirts at http://shop.planetargon.com.
eilonwy% cat .zshrc
eilonwy%

[apjanke]

I'm not sure this change makes the $PATH situation better than before.

On the one hand, it will preserve keeping $PATH in the path construction variable, so system-wide path setup like /etc/paths.d is respected. (Either because it chooses to not replace the original template contents, which have it, or because it will have been in the export PATH=... statements it found in the original rc files.) That's good.

But I think it's more likely to cause immediate breakage by overlooking path elements. In rc files, the path can be modified by several PATH= statements. The current "capture $PATH" behavior will grab the effects of all of them (at least how they were evaluated for this particular session). This parsing will only pick up ones that are using export ... and are flush left. The rc files may not use export on all of the PATH appending statements. Or even any—it's not needed because $PATH is already exported.

Consider this simplified example. Here are the config files before OMZ installation.

# ~/.zshrc
export JAVA_HOME=/opt/java/jdk-8u45
export PATH=$PATH:$JAVA_HOME/bin
PATH=/opt/foonly/bin:$PATH
# Use local stuff first
PATH=$HOME/bin:/usr/local/bin:$PATH
PATH="$PATH:$HOME/.gem/ruby/2.0.0/bin"
$path+=/opt/bar/bin

# ~/.bashrc
export PATH=$PATH:/opt/java/jdk-7u16/bin
PATH=/opt/foonly/bin:$PATH
# Use local stuff first
PATH=$HOME/bin:/usr/local/bin:$PATH
PATH="$PATH:$HOME/.gem/ruby/2.0.0/bin"

I end up with this in the .zshrc after OMZ installation.

# User configuration

export PATH=$PATH:/opt/java/jdk-7u16/bin
export PATH=$PATH:$JAVA_HOME/bin

• It grabbed $JAVA_HOME/bin for the path, but it didn't pick up the $JAVA_HOME initialization it's dependent on, so that entry devolves to /bin when you log in.
• Aside from that, it picked up the old Java version from this stale .bashrc first.
• It missed /opt/foonly, $HOME/bin, and the ruby gems
• It missed /usr/local/bin. Which will probably still end up in the path because the system includes it by default, but it won't be ahead of /bin or /usr/bin, so programs will run but the user might get the wrong version instead of a "not found" error message.
• It misses anything using the array style $path syntax
• It misses anything pulled in from other files that .zshrc would source, etc, etc

I appreciate the problem this is trying to solve, but I don't know that it's the right approach. It makes the path-determination-on-installation behavior more complicated than before, and also introduces a couple failure points that will cause immediate breakage upon installation for some users. This might be a step backward.

IMHO the way to get full correctness with path definitions is probably to just preserve the user's entire .zshrc contents, and add the OMZ loading code to it. $PATH construction is complicated, dynamic, and multi-staged, done with a full general-purpose programming language. Simple parsing is not going to be able to get it right.

And failing that, just slap an explicit dynamic :$PATH on the end of the PATH the installer creates, so at least it picks up additions to the systemwide path configuration.

[levous]

I'm not sure if this is the best issue to add myself to but...

After installing OMZ, rum was complaining:
Warning: PATH set to RVM ruby but GEM_HOME and/or GEM_PATH not set, see: https://github.com/wayneeseguin/rvm/issues/3212

Here is the offending line from my ~/.zshrc

# User configuration export PATH="/Users/rustyzarse/.rvm/gems/ruby-2.2.1/bin:/Users/rustyzarse/.rvm/gems/ruby-2.2.1@global/bin:/Users/rustyzarse/.rvm/rubies/ruby-2.2.1/bin:/Users/rustyzarse/.bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/Users/rustyzarse/.rvm/bin"

to fix, I simply prepended with $PATH; All Fixed :)

# User configuration export PATH="$PATH;/Users/rustyzarse/.rvm/gems/ruby-2.2.1/bin:/Users/rustyzarse/.rvm/gems/ruby-2.2.1@global/bin:/Users/rustyzarse/.rvm/rubies/ruby-2.2.1/bin:/Users/rustyzarse/.bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/Users/rustyzarse/.rvm/bin"
`