-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
doc: add Powershell oneliner to get Windows version #30289
Conversation
.github/ISSUE_TEMPLATE.md
Outdated
@@ -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) |
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.
cmd /c ver
is probably more concise.
cc @nodejs/platform-windows
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.
It is for version, but it still doesn't have bitness
@saitonakamura thanks for moving this forward! Would you consider using this (same as node-gyp):
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. |
@joaocgreis thanks, i'll do that |
@saitonakamura Can you make the change suggested by @joaocgreis? |
Ping @saitonakamura |
@richardlau @joaocgreis do you think this would also already be an improvement as it is right 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.
LGTM as it is, since it already seems to be an improvement over the current situation.
Are you sure about that? This eliminates the very simple |
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". |
(I'm on board with |
Since recently, I also have a Windows system available, so I just checked |
Nice, catch @BridgeAR , i'll see what i can do |
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
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 P.S.S. I don't think we can get away with |
@saitonakamura, this needs a rebase. |
@HarshithaKP , k, will do |
63c69a2
to
ea768ff
Compare
done |
8ae28ff
to
2935f72
Compare
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. |
@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\
So essentially the issue was I felt an urge to fill the fields, but I didn't have the 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 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 (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
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 |
Replying to @saitonakamura:
and @DerekNonGeneric:
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.
@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.
@DerekNonGeneric I don't think it matters much. As far as I know, the (To be clear, I am not formally objecting. I did not block this PR and also won't do that in the future.) |
Perhaps we should take Windows Terminal's lead on this. Their bug report template simply says…
I also happened to stumble upon an authoritative page published my Microsoft with instructions on how to determine version.
Or maybe some combination of the two?
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! |
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. |
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. |
@bzoz, I like that idea a lot, but it didn't work for me in PowerShell. Have you tested it there? |
@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? |
/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 |
It was added in v12.17.0, we could skip it. |
@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. |
@aduh95 I'll check it and verify today |
ea768ff
to
a5cde66
Compare
@aduh95 author fixed |
9392982
to
9e49de0
Compare
Co-authored-by: Michaël Zasso <[email protected]>
9e49de0
to
0ea67b2
Compare
Landed in 2f49720...5fa7c2a |
Co-authored-by: Michaël Zasso <[email protected]> PR-URL: #30289 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
Co-authored-by: Michaël Zasso <[email protected]> PR-URL: #30289 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
Co-authored-by: Michaël Zasso <[email protected]> PR-URL: nodejs#30289 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
Thanks folks! I wasn't expecting this to raise such interesting discussion 🙂 |
Co-authored-by: Michaël Zasso <[email protected]> PR-URL: #30289 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
Checklist