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.

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

PSVersionTable should have entry for OS #1635

Closed
lzybkr opened this Issue Aug 4, 2016 · 22 comments

Comments

@lzybkr
Copy link
Member

lzybkr commented Aug 4, 2016

Steps to reproduce

$PSVersionTable.OS

Expected behavior

Something useful

Actual behavior

empty string

Environment data

> $PSVersionTable
@oising

This comment has been minimized.

Copy link
Contributor

oising commented Aug 4, 2016

Host sniffing is bad; testing for features is good. Is this something we envisage being useful information in the grand scheme things? Esp. since there's an issue somewhere else tracking a desire for a Get-OperatingSystemInfo cmdlet/function.

@lzybkr

This comment has been minimized.

Copy link
Member Author

lzybkr commented Aug 4, 2016

Good point - I was thinking more from a "how can I repro" perspective, but folks would likely start using the property in scripts thinking it's useful.

Maybe we need a Get-PowerShellReproEnvironment cmdlet that we ask folks to run instead.

@SteveL-MSFT SteveL-MSFT modified the milestone: 6.0.0-Alpha.11 Aug 4, 2016

@rkeithhill

This comment has been minimized.

Copy link
Contributor

rkeithhill commented Aug 5, 2016

And it might be helpful for such an environment to list loaded module names & version numbers as well. Might also be nice to have list the architecture (x86, x64, ARM32) of the OS and PowerShell (for those cases where x86 PS is running on x64 Windows).

@Jaykul

This comment has been minimized.

Copy link

Jaykul commented Aug 21, 2016

@oising: testing for features is good, but you don't want to test for features to decide simple things like what library to load. E.g. if I ship System.Data.SqlClient in my module, it has .Net452, .Net46, and separate .NetStandard assemblies for Windows and for Unix. Just to be able to load the right one I need to know OS and Arch. I think it would be enough to know:

  1. OS: Windows or Unix
  2. Architecture: ARM, amd64, x86
@rkeithhill

This comment has been minimized.

Copy link
Contributor

rkeithhill commented Sep 29, 2016

Just do it. It will make it easier for scripts to know where to look for environmental info and it will make it easier for you to get bug reports with that same environment info.

@joeyaiello

This comment has been minimized.

Copy link
Member

joeyaiello commented Dec 14, 2016

Yeah, I used to agree with @oising's opinion here that we shouldn't expose this because it encourages people to "do the wrong thing" when checking for certain kinds of compat at runtime.

Until I saw the kind of lengths people will go to in order to check for these sorts of things anyway. They'll do it one way or another, and at least this way it's easier to understand when and why people are checking for OS so we can mitigate it in the future. (E.g. I want to know that @Jaykul is testing for OS because of x-CLR assembly issues, so that I know we need to adopt .NET Standard 2.0).

That being said, I still agree that checking for features is generally the right thing to do, and we should absolutely have a PSSA rule to throw warnings on checks against $PSVersionTable.OS at runtime.

Oh, and of course, we still need to design this thing. Is it Windows, Mac, Linux? Or do we get down to the distro level? Nano Server? Linux kernel version? Probably needs an RFC...

@Jaykul

This comment has been minimized.

Copy link

Jaykul commented Jan 14, 2017

But please ... get rid of the top level $IsWindows, $IsLinux variables, that's ... icky.

@joeyaiello

This comment has been minimized.

Copy link
Member

joeyaiello commented Jan 23, 2017

@PowerShell/powershell-committee I agree w/ @Jaykul on this one. We should try and move to CoreFX's Environment.OSVersion when it's available (looks like 2.0 timeframe): dotnet/corefx#9851

The next question is whether or not we should propagate $PSVersionTable with Environment.OSVersion or make users search the web to learn about it (leaning towards the former, but I'm open to being told I'm wrong here).

@SteveL-MSFT

This comment has been minimized.

Copy link
Member

SteveL-MSFT commented Jan 26, 2017

@PowerShell/powershell-committee reviewed this, we agree that $psversiontable should use Environment.OSVersion to populate the new OS property. On the topic of $IsWindows/$IsLinux/$IsMac, we agree that the capability to check for the OS via a variable is needed, but should consider implemented in a different way (ie, a prefix to avoid collision or make them members of a feature check variable like $PSFeature.IsWindows). Recommendation is we should have a RFC for the OS check variable. @joeyaiello to author.

@iSazonov

This comment has been minimized.

Copy link
Collaborator

iSazonov commented Mar 8, 2017

I looked CoreFX and didn't found results of Unix tests for Environment.OSVersion under code coverage. CoreFX parse a kernel's uname (see here and here) so we can get bad UX if it will be not tested on all supported Unix distributives.

Also we use in Build.psm1 information from /etc/os-release. It is Linux "standart". We would use this if we would map Windows properties to /etc/os-release properties.

@daxian-dbw

This comment has been minimized.

Copy link
Member

daxian-dbw commented Apr 13, 2017

System.Environment.OSVersion is back in netstandard2.0, but it's not very useful on unix platform. For example, on Ubuntu 16.04:

PS /> [System.Environment]::OSVersion | fl *                                                        

Platform      : Unix
ServicePack   : 
Version       : 4.8.0.41
VersionString : Unix 4.8.0.41

System.Runtime.InteropServices.RuntimeInformation.OSDescription may be a potential substitution:

PS /> [System.Runtime.InteropServices.RuntimeInformation]::OSDescription                            
Linux 4.8.0-41-generic #44~16.04.1-Ubuntu SMP Fri Mar 3 17:11:16 UTC 2017
@SteveL-MSFT

This comment has been minimized.

Copy link
Member

SteveL-MSFT commented Apr 13, 2017

OSDescription looks like part of the output from uname -a. Seems like the way to go.

@chunqingchen chunqingchen self-assigned this Apr 14, 2017

@chunqingchen

This comment has been minimized.

Copy link
Contributor

chunqingchen commented Apr 14, 2017

taking a look on this

@iSazonov

This comment has been minimized.

Copy link
Collaborator

iSazonov commented Apr 14, 2017

If the next what we want, then we could ask CoreFX team:

PS /> [System.Environment]::OSVersion | fl *                                                        

Platform      : Unix
ServicePack   : 
Version       : 4.8.0.41
VersionString : Ubuntu 16.04 
PS /> [System.Environment]::OSVersion | fl *                                                        

Platform      : Windows
ServicePack   : 
Version       : 10.0.15063
VersionString : Windows 10 Creator 
@joeyaiello

This comment has been minimized.

Copy link
Member

joeyaiello commented Apr 14, 2017

I think OSVersion.Platform and OSVersion.Version are both super useful as they don't require any string parsing to get at Windows v. non-Windows and the kernel version. Beyond that, I think we should expose [System.Runtime.InteropServices.RuntimeInformation]::OSDescription as our own uname -a string as well.

I know this might be ballooning $PSVersionTable, but it all feels like useful info.

@joeyaiello joeyaiello moved this from Need to validate to Priority-High in Windows PowerShell compatibility (.NET Standard 2.0) Apr 14, 2017

@chunqingchen

This comment has been minimized.

Copy link
Contributor

chunqingchen commented Apr 18, 2017

@joeyaiello could you give me a mock information $PSVersionTable.OS should give out?
for example, should $PSVersionTable.OS looks like this? (combine the osversion and osdescription)
$PSVersionTable.OS
Platform : Unix
ServicePack :
Version : 4.8.0.41
VersionString : Unix 4.8.0.41
OSDescription: Linux 4.8.0-41-generic #44~16.04.1-Ubuntu SMP Fri Mar 3 17:11:16 UTC 2017

@SteveL-MSFT

This comment has been minimized.

Copy link
Member

SteveL-MSFT commented Apr 18, 2017

@joeyaiello Do we really want Platform since we're keeping $IsUnix/$IsWindows/etc... for automation purposes? Seems like OSDescription is the only thing we really need here primarily for logging purposes.

@joeyaiello

This comment has been minimized.

Copy link
Member

joeyaiello commented Apr 19, 2017

@chunqingchen Yeah, I guess just make $PSVersionTable.OS equivalent to [System.Environment]::OSVersion for now. That's fine.

I'm not super sold on $IsUnix/$IsWindows, but I'd like to understand .NET Core 2.0's platform differentiation a little better before I form a strong opinion.

@rkeithhill

This comment has been minimized.

Copy link
Contributor

rkeithhill commented Apr 19, 2017

BTW the longer you wait, the more modules get published that rely on $Is*. :-) posh-git aleady relies on these variables but we could switch to the "preferred way" of testing for the OS platform.

@SteveL-MSFT

This comment has been minimized.

Copy link
Member

SteveL-MSFT commented Apr 19, 2017

@joeyaiello did you really mean [System.Environment]::OSVersion (which is not really useful) or [System.Runtime.InteropServices.RuntimeInformation]::OSDescription which is equivalent to uname -a? I was saying the latter makes sense

@joeyaiello

This comment has been minimized.

Copy link
Member

joeyaiello commented Apr 19, 2017

Per our discussion today, I'm on board with the recommendation from @SteveL-MSFT.

We also agreed that OSVersion.Platform should probably be included as well. It's possible that $IsWindows and its ilk should become "dumb accelerators" and query that directly.

@iSazonov

This comment has been minimized.

Copy link
Collaborator

iSazonov commented Apr 20, 2017

Is [System.Runtime.InteropServices.RuntimeInformation]::OSDescription really always different?
I mean can we distinguish between Ubuntu LTS, Release and so on? Windows Desktop, Server, Core, IoT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.