Add windows support to ci_setup/setup script. #22259
Conversation
💚 Build Succeeded |
|
Please review :) |
src/dev/ci_setup/setup.sh
Outdated
| @@ -30,13 +30,26 @@ fi | |||
| ### | |||
| ### download node | |||
| ### | |||
| UNAME=`uname` | |||
There was a problem hiding this comment.
Can we stick with the "$(uname)" syntax? Shellcheck recommends it, and I assume for a good reason.
src/dev/ci_setup/setup.sh
Outdated
| if [[ "$UNAME" = *"MINGW64_NT"* ]]; then | ||
| OS='win' | ||
| fi | ||
| echo "Running on OS: $OS" |
There was a problem hiding this comment.
Mind throwing -- before this log message so it matches the other messages after "Setting up node.js and yarn in $dir"?
src/dev/ci_setup/setup.sh
Outdated
| if [[ $OS == 'win' ]]; then | ||
| export PATH="$PATH:$nodeBin" | ||
| else | ||
| export PATH="$nodeBin:$PATH" |
There was a problem hiding this comment.
Why do we prepend, vs append on Linux?
There was a problem hiding this comment.
I was getting node path errors if the node bin dir it was not listed at the end of the windows path
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
src/dev/ci_setup/setup.sh
Outdated
| 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 |
There was a problem hiding this comment.
Can we if/else this like we're doing on line 62?
|
LGTM on green. |
This comment has been minimized.
This comment has been minimized.
💔 Build Failed |
|
jenkins test this |
💔 Build Failed |
|
The CI failures are not due to my change - known flaky tests |
* Add windows support to ci_setup/setup script. * Update uname syntax * Update echo statement to match rest * Update if statement for node package * Revert path variable
* Add windows support to ci_setup/setup script. * Update uname syntax * Update echo statement to match rest * Update if statement for node package * Revert path variable
Add windows support. Part of issue: #21315