-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Fix Get-EnvironmentInformation IsCoreCLR logic #5592
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
Conversation
build.psm1
Outdated
| { | ||
| $environment = @{} | ||
| # Use the .NET Core APIs to determine the current platform. | ||
| # If a runtime exception is thrown, we are on Windows PowerShell, not PowerShell Core, |
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.
The line confuse me. I think it is better to re-write all the comment.
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.
I don't understand what's confusing about it. It's accurate.
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.
comment completely removed as it is no longer relevant.
|
How about changing the check to the following? if ($PSVersionTable.ContainsKey("PSEdition") -and $PSVersionTable.PSEdition -eq "Core") {
$environment += @{'IsCoreCLR' = $true}
$environment += @{'IsLinux' = $IsLinux}
$environment += @{'IsMacOS' = $IsMacOS}
$environment += @{'IsWindows' = $IsWindows}
} else {
$environment += @{'IsCoreCLR' = $false}
$environment += @{'IsLinux' = $false}
$environment += @{'IsMacOS' = $false}
$environment += @{'IsWindows' = $true}
} |
|
We can remove |
|
@daxian-dbw I thought the entire point of this was to not rely on |
|
@markekraus If you break powershell, how would you trust the type resolution in powershell still works? So |
|
@iSazonov We can remove |
|
I think maybe |
|
¯\_(ツ)_/¯ |
| $Runtime = [System.Runtime.InteropServices.RuntimeInformation] | ||
| $OSPlatform = [System.Runtime.InteropServices.OSPlatform] | ||
|
|
||
| if ($PSVersionTable.ContainsKey("PSEdition") -and "Core" -eq $PSVersionTable.PSEdition) { |
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.
maybe a comment about we don't expect anyone to build on Pre-1709 nanoserver. Post-1709, nanoserver user powershell core, but you can probably just say nanoserver for now.
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.
added.
* Fix Get-EnvironmentInformation IsCoreCLR logic
Replace $Runtime = [System.Runtime.InteropServices.RuntimeInformation] and $OSPlatform = [System.Runtime.InteropServices.OSPlatform] with ($PSVersionTable.ContainsKey("PSEdition") -and $PSVersionTable.PSEdition -eq "Core").
* Fix Get-EnvironmentInformation IsCoreCLR logic
Replace $Runtime = [System.Runtime.InteropServices.RuntimeInformation] and $OSPlatform = [System.Runtime.InteropServices.OSPlatform] with ($PSVersionTable.ContainsKey("PSEdition") -and $PSVersionTable.PSEdition -eq "Core").
closes #5581
.NET 4.7.1 adds the APIs
Get-EnvironmentInformationwas using to determine if it is on CoreCLR or not. This is not$IsCoreCLR, but a part of the environment gathering used in the build and packaging.