Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Respect original PATH definitions on install #2589

Open
wants to merge 1 commit into from

8 participants

@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 #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! :smile:

@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...?

tools/install.sh
@@ -25,10 +25,18 @@ fi
echo "\033[0;34mUsing the Oh My Zsh template file and adding it to ~/.zshrc\033[0m"
cp ~/.oh-my-zsh/templates/zshrc.zsh-template ~/.zshrc
-echo "\033[0;34mCopying your current PATH and adding it to the end of ~/.zshrc for you.\033[0m"
-sed -i -e "/export PATH=/ c\\
-export PATH=\"$PATH\"
-" ~/.zshrc
+ORIG_PATHS=`grep -sh '^export PATH=' ~/.bashrc ~/.zshrc.pre-oh-my-zsh`
+if [ "$ORIG_PATHS" ]
+then
+ echo "\033[0;34mCopying your current PATH and adding it to the end of ~/.zshrc for you.\033[0m"
+ ed tools/zshrc_test <<__EOT__

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

@avit
avit added a note

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@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.

tools/install.sh
@@ -25,10 +25,18 @@ fi
echo "\033[0;34mUsing the Oh My Zsh template file and adding it to ~/.zshrc\033[0m"
cp ~/.oh-my-zsh/templates/zshrc.zsh-template ~/.zshrc
-echo "\033[0;34mCopying your current PATH and adding it to the end of ~/.zshrc for you.\033[0m"
-sed -i -e "/export PATH=/ c\\
-export PATH=\"$PATH\"
-" ~/.zshrc
+ORIG_PATHS=`grep -sh '^export PATH=' ~/.bashrc ~/.zshrc.pre-oh-my-zsh`
+if [ "$ORIG_PATHS" ]
+then
+ echo "\033[0;34mCopying your current PATH and adding it to the end of ~/.zshrc for you.\033[0m"
+ ed tools/zshrc_test <<__EOT__
+g/export PATH=/
+c
+$OLD_PATHS

This need to be $ORIG_PATHS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@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'}"`
@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. :+1: 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)

@mitzip

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 avit Respect original PATH definitions on install
The exporting of PATH as a fixed string would clobber any system-wide
paths added to env after installation. Instead, we now read any existing
PATH exports from previous config files and copy those from the user's
bash config, or else default to a basic prepend-only example that
respects the system environment.
152aab6
@avit

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

@mitzip

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. :smiley:

@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 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 24, 2014
  1. @avit

    Respect original PATH definitions on install

    avit authored
    The exporting of PATH as a fixed string would clobber any system-wide
    paths added to env after installation. Instead, we now read any existing
    PATH exports from previous config files and copy those from the user's
    bash config, or else default to a basic prepend-only example that
    respects the system environment.
This page is out of date. Refresh to see the latest.
Showing with 15 additions and 6 deletions.
  1. +15 −6 tools/install.sh
View
21 tools/install.sh
@@ -23,14 +23,23 @@ fi
echo "\033[0;34mUsing the Oh My Zsh template file and adding it to ~/.zshrc\033[0m"
cp $ZSH/templates/zshrc.zsh-template ~/.zshrc
-sed -i -e "/^ZSH=/ c\\
+sed -e "/^ZSH=/ c\\
ZSH=$ZSH
-" ~/.zshrc
+" ~/.zshrc | tee ~/.zshrc
-echo "\033[0;34mCopying your current PATH and adding it to the end of ~/.zshrc for you.\033[0m"
-sed -i -e "/export PATH=/ c\\
-export PATH=\"$PATH\"
-" ~/.zshrc
+ORIG_RC_FILES=`ls ~/.bashrc ~/.zshrc.pre-oh-my-zsh 2>/dev/null` || :
+if [ "$ORIG_RC_FILES" ]
+then
+ ORIG_PATHS=`grep -sh '^export PATH=' $ORIG_RC_FILES` || :
+fi
+
+if [ "$ORIG_PATHS" ]
+then
+ echo "\033[0;34mCopying your current PATH and adding it to the end of ~/.zshrc for you.\033[0m"
+ nl=$'\n'
+ sed -e '/^export PATH=/ c\
+'"${ORIG_PATHS//$nl/\\$nl}" ~/.zshrc | tee ~/.zshrc
+fi
if [ "$SHELL" != "$(which zsh)" ]; then
echo "\033[0;34mTime to change your default shell to zsh!\033[0m"
Something went wrong with that request. Please try again.