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.

Sign up for GitHub

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

Add windows support to ci_setup/setup script. #22259

Merged
merged 5 commits into from
Aug 31, 2018
Merged

Add windows support to ci_setup/setup script. #22259

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c80fc25
Add windows support to ci_setup/setup script.
Aug 22, 2018
f163d52
Update uname syntax
liza-mae Aug 30, 2018
c8bcf32
Update echo statement to match rest
liza-mae Aug 30, 2018
efea0fc
Update if statement for node package
liza-mae Aug 30, 2018
6f1bdf8
Revert path variable
liza-mae Aug 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 28 additions & 4 deletions src/dev/ci_setup/setup.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,26 @@ fi
###
### download node
###
UNAME=`uname`
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we stick with the "$(uname)" syntax? Shellcheck recommends it, and I assume for a good reason.

OS='linux'
if [[ "$UNAME" = *"MINGW64_NT"* ]]; then
OS='win'
fi
echo "Running on OS: $OS"
Copy link
Contributor

Choose a reason for hiding this comment

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

Mind throwing -- before this log message so it matches the other messages after "Setting up node.js and yarn in $dir"?


nodeVersion="$(cat $dir/.node-version)"
nodeUrl="https://nodejs.org/download/release/v$nodeVersion/node-v$nodeVersion-linux-x64.tar.gz"
nodeDir="$cacheDir/node/$nodeVersion"
nodeBin="$nodeDir/bin"
nodeUrl="https://nodejs.org/download/release/v$nodeVersion/node-v$nodeVersion-linux-x64.tar.gz"
if [[ $OS == 'win' ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we if/else this like we're doing on line 62?

nodeBin="$nodeDir"
nodeUrl="https://nodejs.org/download/release/v$nodeVersion/node-v$nodeVersion-win-x64.zip"
fi

echo " -- node: version=v${nodeVersion} dir=$nodeDir"

echo " -- setting up node.js"
if [ -x "$nodeDir/bin/node" ] && [ "$($nodeDir/bin/node --version)" == "v$nodeVersion" ]; then
if [ -x "$nodeBin/node" ] && [ "$($nodeBin/node --version)" == "v$nodeVersion" ]; then
echo " -- reusing node.js install"
else
if [ -d "$nodeDir" ]; then
Expand All @@ -46,14 +59,25 @@ else

echo " -- downloading node.js from $nodeUrl"
mkdir -p "$nodeDir"
curl --silent "$nodeUrl" | tar -xz -C "$nodeDir" --strip-components=1
if [[ $OS == 'win' ]]; then
nodePkg="$nodeDir/${nodeUrl##*/}"
curl --silent -o $nodePkg $nodeUrl
unzip -jqo $nodePkg -d $nodeDir
else
curl --silent "$nodeUrl" | tar -xz -C "$nodeDir" --strip-components=1
fi

fi


###
### "install" node into this shell
###
export PATH="$nodeDir/bin:$PATH"
if [[ $OS == 'win' ]]; then
export PATH="$PATH:$nodeBin"
else
export PATH="$nodeBin:$PATH"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we prepend, vs append on Linux?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was getting node path errors if the node bin dir it was not listed at the end of the windows path

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not seeing any errors with setting it first - if anything I would expect an issue if there was a node executable already in your path. Setting it first would give it precedence. What is the exact error you're seeing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure why :) It was this type of error:
Error: 'module' is undefined
Code: 800A1391
Source: Microsoft JScript runtime error

It is possible I had another node version installed when the problem occurred, I can't recall right now. I will remove it for now and if the problem comes up again we can fix it at that time.

fi
hash -r


Expand Down