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

install prompt on WSL2 fails #78

Closed
sjk07 opened this issue Dec 22, 2020 · 5 comments · Fixed by #79
Closed

install prompt on WSL2 fails #78

sjk07 opened this issue Dec 22, 2020 · 5 comments · Fixed by #79

Comments

@sjk07
Copy link
Member

sjk07 commented Dec 22, 2020

seems like there is an issue with the bootstrap.sh -> install.sh variable passing.

. "$PROMPT_INSTALL_TEMP/install.sh" $PROMPT_SHELL $PROMPT_INSTALL_TEMP

here are two sample files that cause the issue:
bootstrap.sh

#!/usr/bin/env sh

set -e

DIR=$(mktemp -d)
. "./test.sh" something "$DIR"

install.sh

SCRIPT_DIR=${2:-$(CDPATH= cd -- "$(dirname -- "$0")" && pwd -P)}

echo $SCRIPT_DIR

calling sh -x ./bootstrap.sh produces:

scott@DESKTOP-AFL3UD5:~$ sh -x ./bootstrap.sh
+ set -e
+ mktemp -d
+ DIR=/tmp/tmp.od69gATykC
+ . ./install.sh something /tmp/tmp.od69gATykC
+ dirname -- ./bootstrap.sh
+ CDPATH= cd -- .
+ pwd -P
+ SCRIPT_DIR=/home/scott
+ echo /home/scott

adding the . in front of the calling command to ./install.sh seems to cause something funky to happen and the second variable does not propagate .

removing the . in . "./test.sh" something "$DIR" to "./test.sh" something "$DIR" results in the correct output:

scott@DESKTOP-AFL3UD5:~$ sh -x ./bootstrap.sh
+ set -e
+ mktemp -d
+ DIR=/tmp/tmp.7Sbl0QtwLL
+ ./install.sh something /tmp/tmp.7Sbl0QtwLL
/tmp/tmp.7Sbl0QtwLL

PR incoming; want to see if this is the same case in other dists. also wondering why this was not caught in CI

@patrickserrano
Copy link
Contributor

I wonder if this is related to the upgrade/install issue I ran into last week using WSL1. I didn't finish debugging but it looked like the value of $SCRIPTDIR in install.sh wasn't being set correctly which was causing the install to look in $HOME/templates instead of temp_dir/templates.

@sjk07
Copy link
Member Author

sjk07 commented Dec 22, 2020

@patrickserrano yup that was the exact problem 😄

PR fixes the issue. dunno if it causes problems for the other OSs

@dmccaffery
Copy link
Member

I will look at the CI logs, but it definitely seems to work just fine there. :)

@dmccaffery
Copy link
Member

🎉 This issue has been resolved in version 8.2.1-next.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@dmccaffery
Copy link
Member

@sjk07 / @patrickserrano : this was working in ci/cd because we are invoking install.sh directly:

./install.sh $SHELL_NAME
and we're not relying on the temp dir -- basically, the bootstrap script isn't invoked as it would clone the upstream rather than whatever changes are included in the PR (until after its merged).

There is no temp dir in this scenario and thus the default (current script path) is used. There isn't any good way to test the bootstrap script itself while going through the real execution path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants