Skip to content

Conversation

@markekraus
Copy link
Contributor

@markekraus markekraus commented Dec 1, 2017

closes #5581

.NET 4.7.1 adds the APIs Get-EnvironmentInformation was 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.

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,
Copy link
Collaborator

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.

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 don't understand what's confusing about it. It's accurate.

Copy link
Contributor Author

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.

@iSazonov iSazonov self-assigned this Dec 1, 2017
@daxian-dbw
Copy link
Member

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}
}

@iSazonov
Copy link
Collaborator

iSazonov commented Dec 1, 2017

We can remove $PSVersionTable.ContainsKey("PSEdition").

@markekraus
Copy link
Contributor Author

@daxian-dbw I thought the entire point of this was to not rely on pwsh for this information as much as possible. If I break PowerShell so that it reports $IsWindows and $IsLinux all kinds of trouble will happen. The environment from this function is for building and testing. It should be set apart. ideally we would using some .NET API to figure out it this is CoreCLR or not and not rely on $PSVersionTable.PSEdition. Any ideas?

@daxian-dbw
Copy link
Member

@markekraus If you break powershell, how would you trust the type resolution in powershell still works? So [System.Runtime.InteropServices.RuntimeInformation] could throw a unrelated exception and make the script believe it's in a Windows PowerShell environment 😄
I think $PSVersionTable.PSEdition and variables like $IsWindows, $IsLinux and $IsMacOS are pretty static and would be quite unlikely to be changed. Also, I think the PowerShell session you use to build powershell usually is a comparatively stable version.

@daxian-dbw
Copy link
Member

@iSazonov We can remove $PSVersionTable.ContainsKey("PSEdition") in build.psm1, but for a script that has Set-Strict -version latest at the top, this check will be needed in case it's running on PSv5 or earlier versions.

@daxian-dbw
Copy link
Member

I think maybe RuntimeInformation was used initially because of the NanoServer PowerShell, which has $PSVersionTable.PSEdition == Core but doesn't have $IsCoreCLR, $IsWindows, $IsLinux and $IsMacOS. But I don't think anyone will build powershell in a NanoServer. Also, starting from RS3, the NanoServer image doesn't have the NanoServer PowerShell in it.

@markekraus
Copy link
Contributor Author

¯\_(ツ)_/¯
updated with the suggestions.

$Runtime = [System.Runtime.InteropServices.RuntimeInformation]
$OSPlatform = [System.Runtime.InteropServices.OSPlatform]

if ($PSVersionTable.ContainsKey("PSEdition") -and "Core" -eq $PSVersionTable.PSEdition) {
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added.

@iSazonov iSazonov merged commit ec8b849 into PowerShell:master Dec 2, 2017
@TravisEz13 TravisEz13 added this to the 6.0.0-RC.2 milestone Dec 5, 2017
TravisEz13 pushed a commit to TravisEz13/PowerShell that referenced this pull request Dec 5, 2017
* 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").
TravisEz13 pushed a commit that referenced this pull request Dec 5, 2017
* 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").
@markekraus markekraus deleted the FixIsCoreClr4.7.1 branch January 19, 2018 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Get-EnvironmentInformation does not properly check for CoreCLR

4 participants