-
Notifications
You must be signed in to change notification settings - Fork 237
Fix .NET installation #1320
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
Fix .NET installation #1320
Conversation
PowerShellEditorServices.build.ps1
Outdated
$installScriptPath = Join-Path ([System.IO.Path]::GetTempPath()) $installScript | ||
Invoke-WebRequest "https://dot.net/v1/$installScript" -OutFile $installScriptPath | ||
|
||
# Download and install the different .NET channels in parallel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't appear to be parallel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch
$params = if ($script:IsUnix) | ||
{ | ||
@('-Channel', $dotnetChannel, '-InstallDir', $env:DOTNET_INSTALL_DIR, '-NoPath', '-Verbose') | ||
} | ||
else | ||
{ | ||
@{ | ||
Channel = $dotnetChannel | ||
InstallDir = $env:DOTNET_INSTALL_DIR | ||
NoPath = $true | ||
Verbose = $true | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did you do this instead of what I had before with Invoke-Expression
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Broke with my path which has spaces in it. Needing to fix it and wanting to avoid iex
I went with this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine I guess... annoying having a platform check for the same thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @daxian-dbw
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
> invoke-build build
Build build C:\Users\Robert Holt\Documents\Dev\Microsoft\PowerShellEditorServices\PowerShellEditorServices.build.ps1
Task /build/BinClean
Done /build/BinClean 00:00:00.0032695
Task /build/SetupDotNet
### Installing .NET CLI ...
ERROR: The term 'C:\Users\Robert' is not recognized as the name of a cmdlet, function, script file, or operable program.
Check the spelling of the name, or if a path was included, verify that the path is correct and try again.
At C:\Users\Robert Holt\Documents\Dev\Microsoft\PowerShellEditorServices\PowerShellEditorServices.build.ps1:76 char:5
+ Invoke-Expression "$installScriptPath $paramArr"
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
At C:\Users\Robert Holt\Documents\Dev\Microsoft\PowerShellEditorServices\PowerShellEditorServices.build.ps1:82 char:1
+ task SetupDotNet -Before Clean, Build, TestHost, TestServerWinPS, Tes …
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
At C:\Users\Robert Holt\Documents\Dev\Microsoft\PowerShellEditorServices\PowerShellEditorServices.build.ps1:211 char:1
+ task Build BinClean,{
+ ~~~~~~~~~~~~~~~~~~~~~
Build FAILED. 3 tasks, 1 errors, 0 warnings 00:00:01.7445922
Invoke-Expression: C:\Users\Robert Holt\Documents\Dev\Microsoft\PowerShellEditorServices\PowerShellEditorServices.build.ps1:76
Line |
76 | Invoke-Expression "$installScriptPath $paramArr"
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| The term 'C:\Users\Robert' is not recognized as the name of a cmdlet, function, script file, or
| operable program. Check the spelling of the name, or if a path was included, verify that the path is
| correct and try again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I totally believe you... I just had exactly this code once and wanted to not have to do this, which is why I used iex in the first place. I would guess that if we wrapped InstallDir in quotes in might work... but seems hacky.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, fixing it I didnt' want to get into a quote escaping tarpit. Ideally install-dotnet.ps1 would work cross-platform. Frankly that script could use some improvement anyway
|
||
# The install script is platform-specific | ||
$installScriptExt = if ($script:IsUnix) { "sh" } else { "ps1" } | ||
$installScript = "dotnet-install.$installScriptExt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you need this extra variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gets used twice below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
.NET install script was failing on my machine because of whitespace in the path.
I got rid of the
iex
and replaced it with some splatting.Also moved the three downloads into jobs in an attempt to parallelise them a bit, although not sure what kind of speedup there is.Not reliable, back to serialTested this on Windows and macOS, but worth another test.