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

doc: add Powershell oneliner to get Windows version #30289

Closed

Conversation

saitonakamura
Copy link
Contributor

Checklist

@nodejs-github-bot nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Nov 6, 2019
@@ -9,7 +9,7 @@ repo. https://github.com/nodejs/help
Please fill in as much of the template below as you're able.

Version: output of `node -v`
Platform: output of `uname -a` (UNIX), or version and 32 or 64-bit (Windows)
Platform: output of `uname -a` (UNIX), or output of `"$([Environment]::OSVersion | ForEach-Object VersionString) $(if ([Environment]::Is64BitOperatingSystem) { "x64" } else { "x86" })"` in Powershell console (Windows)
Copy link
Member

Choose a reason for hiding this comment

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

cmd /c ver is probably more concise.

cc @nodejs/platform-windows

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is for version, but it still doesn't have bitness

@joaocgreis
Copy link
Member

@saitonakamura thanks for moving this forward! Would you consider using this (same as node-gyp):

systeminfo | findstr /B /C:"OS Name" /C:"OS Version" /C:"System Type"

It works in CMD or PowerShell without changes, including for ARM64 Windows systems. The only downside is that it produces 3 lines, but that's worth the simplicity.

@tniessen tniessen added the windows Issues and PRs related to the Windows platform. label Nov 6, 2019
@BridgeAR BridgeAR added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Nov 6, 2019
@saitonakamura
Copy link
Contributor Author

@joaocgreis thanks, i'll do that

@Trott
Copy link
Member

Trott commented Nov 23, 2019

@saitonakamura Can you make the change suggested by @joaocgreis?

@BridgeAR
Copy link
Member

Ping @saitonakamura

@BridgeAR
Copy link
Member

@richardlau @joaocgreis do you think this would also already be an improvement as it is right now?

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

LGTM as it is, since it already seems to be an improvement over the current situation.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 2, 2020
@Trott
Copy link
Member

Trott commented Jan 4, 2020

it already seems to be an improvement over the current situation.

Are you sure about that? This eliminates the very simple version and 32 or 64-bit (Windows) text.

@Trott
Copy link
Member

Trott commented Jan 4, 2020

it already seems to be an improvement over the current situation.

Are you sure about that? This eliminates the very simple version and 32 or 64-bit (Windows) text.

Especially in an issue template, a big scary command string that only runs in some environments might not be desirable as a replacement for "just tell us what Windows version your using and whether its 32- or 64-bit".

@Trott
Copy link
Member

Trott commented Jan 4, 2020

(I'm on board with systeminfo | findstr /B /C:"OS Name" /C:"OS Version" /C:"System Type", though, if it runs reliably everywhere.)

@Trott Trott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 6, 2020
@BridgeAR
Copy link
Member

BridgeAR commented Jan 12, 2020

Since recently, I also have a Windows system available, so I just checked systeminfo | findstr /B /C:"OS Name" /C:"OS Version" /C:"System Type" and it seems to be language specific. It won't work on any system that is not set up in English.

@saitonakamura
Copy link
Contributor Author

Nice, catch @BridgeAR , i'll see what i can do

@saitonakamura
Copy link
Contributor Author

saitonakamura commented Feb 5, 2020

I've come up with one more variant, not sure if it's better or worse

Get-CimInstance Win32_OperatingSystem | Select-Object Caption,BuildNumber,OSArchitecture | Format-List

It gives you the output like this

Caption        : Microsoft Windows 10 Pro
BuildNumber    : 19041
OSArchitecture : 64-bit

OSArchitecture is a culture specific, but, luckily for us, it will still contain 64 or 32, so I believe it's ok

Personally I found this less scarier, but maybe it's just me. Let me know what do you like more and I'll finish it

P.S. The best option would be Get-ComputerInfo | Select-Object WindowsProductName,OsVersion,OsArchitecture | Format-List, but that's only Powershell v5+

P.S.S. I don't think we can get away with cmd and systeminfo. Pretty much every option that tries to fight it's culture-specific nature involves powershell

@HarshithaKP
Copy link
Member

@saitonakamura, this needs a rebase.

@saitonakamura
Copy link
Contributor Author

@HarshithaKP , k, will do

@saitonakamura
Copy link
Contributor Author

done

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:19
@tniessen
Copy link
Member

Thank you for the PR, @saitonakamura. I am -0 on this. I am not sure if adding such complexity to an otherwise simple template will actually help, or rather drive people away from using the template. I personally avoid PowerShell, but I guess it's universally available by now.

Most Node.js issues are not OS-specific, but people usually at least mention "Windows", even with the current template. Version and processor architecture are often unknown, but they are also rarely helpful or relevant. Windows 10 is now the only officially supported version, both from Microsoft's side and ours. In 2010, ten years ago, Microsoft reported that 50% of Windows users were already using 64 bit processors, and by now, that has probably gone up to 95% or so. I suspect the information gain will be very small.

@saitonakamura
Copy link
Contributor Author

@tniessen that's a thoughtful inside. Let me clarify why I did this change in the first place: when I was doing code&learn I wanted to create an issue and saw this template. I thought that filling all this fields is mandatory and two problems arised\

  1. It's not clear what is meant by version: major number (7, 8, 10), build number, edition (Home, Pro, Enterprise), type (Server, Server Core) ?
  2. It's not clear where to get them. I mean, I happened to know those env variables since I worked with windows apis, but I feel like a lot of people can't do this from the top of their head

So essentially the issue was I felt an urge to fill the fields, but I didn't have the uname -r at hand
If you're saying that this information is probably not very useful in practice on windows, maybe we can somehow mark it as optional?

As for the powershell and desire not to use it: it's universally available since windows 7, the code in the PR will work on all versions and editions
I feel the same way against cmd and bat scripts, but if i'm about to be given an option to execute a cmd command or just go through various gui options and menus, I'd prefer cmd
And I still consider the command readable and therefore, "trustable"

@DerekNonGeneric
Copy link
Contributor

I agree that this is a change that needs to be made. As someone who has both reported bugs with Node.js on Windows and someone who has worked on these issues templates in the past, I can tell you (and as @saitonakamura points out above), asking someone what version of Windows they are using is an extremely ambiguous question. Allow me to explain.

If you run the following command in PowerShell.

Get-ComputerInfo | select WindowsProductName, WindowsVersion, OsHardwareAbstractionLayer

The output is something like the following.

WindowsProductName           WindowsVersion OsHardwareAbstractionLayer
------------------           -------------- --------------------------
Windows Server 2019 Standard 1809           10.0.17763.1131

It reports than my Windows Version is 1809. Now, if you run the following command.

(Get-CimInstance Win32_OperatingSystem)

The output is something like the following.

SystemDirectory     Organization BuildNumber RegisteredUser SerialNumber            Version
---------------     ------------ ----------- -------------- ------------            -------
C:\Windows\system32              17763       Windows User   XXXXXXXXXXXXXX       10.0.17763

It reports that my Windows Version is 10.0.17763. So, which is it?

I am not sure if adding such complexity to an otherwise simple template will actually help, or rather drive people away from using the template.

I have actually avoided reporting bugs with Node.js on Windows because of the ambiguity of this request.


Although I don't know what the solution is to this (since I don't triage these bugs), please do fix this. +1

@tniessen
Copy link
Member

Replying to @saitonakamura:

maybe we can somehow mark it as optional?

and @DerekNonGeneric:

I have actually avoided reporting bugs with Node.js on Windows because of the ambiguity of this request.

Note that the template already says "Please fill in as much of the template below as you're able" (emphasis added). If this is ambigious, maybe the wording should be changed.

It's not clear what is meant by version: major number (7, 8, 10), build number, edition (Home, Pro, Enterprise), type (Server, Server Core) ?

@saitonakamura Fair point. From my perspective, version implies either the major number or build number (both would be okay), but that might be a translation issue.

It reports that my Windows Version is 10.0.17763. So, which is it?

@DerekNonGeneric I don't think it matters much. As far as I know, the BuildNumber, the WindowsVersion, the OsHardwareAbstractionLayer, and the WindowsProductName can all be used to identify the major Windows version. The exact build number rarely matters, and the distinction between desktop OS and server OS is often also not important.


(To be clear, I am not formally objecting. I did not block this PR and also won't do that in the future.)

@DerekNonGeneric
Copy link
Contributor

DerekNonGeneric commented Jul 4, 2020

In 2010, ten years ago, Microsoft reported that 50% of Windows users were already using 64 bit processors, and by now, that has probably gone up to 95% or so. I suspect the information gain will be very small.

Perhaps we should take Windows Terminal's lead on this. Their bug report template simply says…

run `[Environment]::OSVersion` for powershell, or `ver` for cmd

I also happened to stumble upon an authoritative page published my Microsoft with instructions on how to determine version.

Check your Windows version by selecting the Windows logo key + R, type winver, select OK. (Or enter the ver command in Windows Command Prompt).


Or maybe some combination of the two?

run [Environment]::OSVersion for PowerShell or ver for Windows Command Prompt


If this PR is expected to land, would it be too much to ask to update the other issue templates that request Windows version? Also, the following document would need the same.

https://github.com/nodejs/node/blob/master/doc/guides/contributing/issues.md

Thanks in advance!

@DerekNonGeneric
Copy link
Contributor

Given that this is much more challenging on Windows than on other systems (what isn't?), maybe we should just add a link?

It's not that it's challenging, it's just that the current issue templates aren't being specific enough about exactly what information they are requesting. This PR makes a good effort at summarizing everything into a one-liner that we can provide to users, however, it seems to me like we should re-evaluate these issue templates as a whole.

@bzoz
Copy link
Contributor

bzoz commented Aug 4, 2020

Why just not use Node to get the data?

node -e "const os=require('os'); console.log(os.platform(), os.version(), os.release(), os.arch(), os.freemem(), os.totalmem(), process.arch, process.version)"

We don't really have "Node is not working at all" issues, and this will get all the data that we need.

@DerekNonGeneric
Copy link
Contributor

@bzoz, I like that idea a lot, but it didn't work for me in PowerShell. Have you tested it there?

@bzoz
Copy link
Contributor

bzoz commented Aug 4, 2020

@DerekNonGeneric, works on my box:

PS C:\Users\ja> node -v
v14.6.0
PS C:\Users\ja> node -e "const os=require('os'); console.log(os.platform(), os.version(), os.release(), os.arch(), os.freemem(), os.totalmem(), process.arch, process.version)"
win32 Windows 10 Pro 10.0.19041 x64 9104707584 17082380288 x64 v14.6.0

What error do you get?

@DerekNonGeneric
Copy link
Contributor

DerekNonGeneric commented Aug 5, 2020

/to @bzoz

I had tested it on an older LTS version, so perhaps that could be the reason why.

PS C:\Users\Administrator> node --version
v12.13.1
PS C:\Users\Administrator> node -e "const os=require('os'); console.log(os.platform(), os.version(), os.release(), os.arch(), os.freemem(), os.totalmem(), process.arch, process.version)"
[eval]:1
const os=require('os'); console.log(os.platform(), os.version(), os.release(), os.arch(), os.freemem(), os.totalmem(), process.arch, process.version)
                                                      ^

TypeError: os.version is not a function
    at [eval]:1:55
�[90m    at Script.runInThisContext (vm.js:116:20)�[39m
�[90m    at Object.runInThisContext (vm.js:306:38)�[39m
    at Object.<anonymous> ([eval]-wrapper:9:26)
�[90m    at Module._compile (internal/modules/cjs/loader.js:959:30)�[39m
�[90m    at evalScript (internal/process/execution.js:80:25)�[39m
�[90m    at internal/main/eval_string.js:23:3�[39m
PS C:\Users\Administrator>

However, on the latest current, it is working as expected.

PS C:\Users\Administrator> node --version
v14.7.0
PS C:\Users\Administrator> node -e "const os=require('os'); console.log(os.platform(), os.version(), os.release(), os.arch(), os.freemem(), os.totalmem(), process.arch, process.version)"
win32 Windows Server 2019 Standard 10.0.17763 x64 4401385472 14904889344 x64 v14.7.0

If this doesn't work with older versions of node, we should definitely avoid this approach.

@bzoz
Copy link
Contributor

bzoz commented Aug 5, 2020

It was added in v12.17.0, we could skip it.

@aduh95
Copy link
Contributor

aduh95 commented Nov 17, 2020

@saitonakamura GitHub cannot link the commit to your GitHub account. You can take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork for directions on how to fix that. If you are not interested in having the commit linked to your GitHub account, that's definitely OK and we can land it as is.

@saitonakamura
Copy link
Contributor Author

@aduh95 I'll check it and verify today

@saitonakamura
Copy link
Contributor Author

@aduh95 author fixed

@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 2, 2020
Co-authored-by: Michaël Zasso <targos@protonmail.com>
@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 6, 2020
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 6, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Dec 6, 2020

Landed in 2f49720...5fa7c2a

@github-actions github-actions bot closed this Dec 6, 2020
nodejs-github-bot pushed a commit that referenced this pull request Dec 6, 2020
Co-authored-by: Michaël Zasso <targos@protonmail.com>

PR-URL: #30289
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
danielleadams pushed a commit that referenced this pull request Dec 7, 2020
Co-authored-by: Michaël Zasso <targos@protonmail.com>

PR-URL: #30289
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@danielleadams danielleadams mentioned this pull request Dec 7, 2020
cjihrig pushed a commit to cjihrig/node that referenced this pull request Dec 8, 2020
Co-authored-by: Michaël Zasso <targos@protonmail.com>

PR-URL: nodejs#30289
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@saitonakamura saitonakamura deleted the issue-template-win-version branch December 9, 2020 08:15
@saitonakamura
Copy link
Contributor Author

Thanks folks! I wasn't expecting this to raise such interesting discussion 🙂

targos pushed a commit that referenced this pull request May 1, 2021
Co-authored-by: Michaël Zasso <targos@protonmail.com>

PR-URL: #30289
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@danielleadams danielleadams mentioned this pull request May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. meta Issues and PRs related to the general management of the project. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.