sebhildebrandt / systeminformation

System Information Library for Node.JS
MIT License
2.69k stars 305 forks source link

Twitter coverage #928

Open ovinusreal opened 3 weeks ago

ovinusreal commented 3 weeks ago

Recently, the package was criticized on Twitter as a dependency of the Discord native app for unnecessarily many invocations of Powershell. I'm quoting them here in case they are valid feedback:

it's using a library called systeminformation that calls powershell like "Get-WmiObject Win32_logicaldisk | select Access,Caption,FileSystem,FreeSpace,Size ${drive ? '| where -property Caption -eq ' + drive : ''} | fl" instead of doing anything properly.

https://github.com/sebhildebrandt/systeminformation/blob/6ffb79aca39796bcf14cf167873a5c3f514ce5fc/lib/filesystem.js#L197

they're executing from DriverStore, assuming System32 AND hardcoding C:\Windows

[...]

this code scans EIGHT HUNDRED different folders to try and find an nvidia-smi binary.

https://github.com/sebhildebrandt/systeminformation/blob/6ffb79aca39796bcf14cf167873a5c3f514ce5fc/lib/graphics.js#L387-L416

https://github.com/sebhildebrandt/systeminformation/blob/6ffb79aca39796bcf14cf167873a5c3f514ce5fc/lib/util.js#L38

The former appears to be already addressed in #927

They also recommend using GetACP instead of having to assume codepage 437.

https://github.com/sebhildebrandt/systeminformation/blob/6ffb79aca39796bcf14cf167873a5c3f514ce5fc/lib/util.js#L565-L595

I am not familiar with Windows development, and some of these don't provide better alternatives, so feel free to close this issue if the claims were baseless or made without context.

Nonetheless, thanks for the work you've put into this project!

sylveon commented 3 weeks ago

Most of these WMI usages can be replaced by native Win32 APIs giving the same info (for example, to get the free space on a drive, GetDiskFreeSpace).

Otherwise, if WMI is really needed, there is a native WMI API that can be used instead of forking out to PowerShell (which is really slow). There is the node-wmi module exposing this API to JavaScript. (nevermind that just calls to wmic which is deprecated)

sebhildebrandt commented 3 weeks ago

@ovinusreal thank you for pointing me to this discussion. I know (and this is also stated in the docs) that on Windows the situation is far from being optimal. This is mostly related to the absence of easy ways to gather system relevant information in Windows.

The first route I took was using WMI (same approach as we have in the node-mwi package that @sylveon mentioned. Biut WMI is deprecated so the only way is to use PowerShell to get information via command line (all other OS do provide such functionality much easier!).

I am working on a new version of this library that should make the PowerShell calls more efficient but It will still take some time to finish this new version (6.0) as there are also a lot of new functions that I am planning to finish.

My take way is still some improvements mentioned in the twitter chat that I will try to come up with a better solution.

sylveon commented 3 weeks ago

There is no need to call into WMI for most of these, there are available native APIs.

sebhildebrandt commented 3 weeks ago

@sylveon ... well yes. But this would require some external dependencies (like ffi, node-ffi, sbffi or others) plus some C code ... which I always wanted to avoid.

dongle-the-gadget commented 3 weeks ago

I don't think avoiding native code is the best choice here. WMI is already resource intensive, per se.

WamWooWam commented 3 weeks ago

hi, as the OP of that tweet i did want to apologise for anything that might've come your way as a result, did not remotely expect the reception it got and had i anticipated that i would've been much less harsh

that said, i do echo some of the sentiment here, the correct way to read this information on windows is programmatically via C-based apis, which is effectively what you're doing with powershell, just... slowly. as im sure you well know by now powershell can have a lot of launch and runtime overhead on slower or ultra-low-power processors (as was the case for me), especially when many instances are launched at once (as node's async model tends to do).

a set of native bindings would likely be a better long-term solution though i very much understand the difficulty that comes with that, maybe for a separate project?