[rust] Replace WMIC commands (deprecated) by WinAPI in Windows #15363
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||
CI Feedback 🧐(Feedback updated until commit 6243961)A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
bonigarcia
force-pushed
the
sm_powershell
branch
6 times, most recently
from
dba3035 to
3974c4d
Compare
|
It seems that the performance decline with this PR is real in Windows. In the following workflow, we can see the times that tests last in Windows: The same tests in Linux: And in macOS: And the same tests, in Windows, but just before the PR: |
|
WMIC is a wrapper, PowerShell is a wrapper. Why not to use WinAPI directly to get the version of file? |
bonigarcia
changed the title
|
Using WinAPI instead of PowerShell is a great idea, thanks a lot @nvborisenko. I have just changed this PR accordingly. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
User description
Motivation and Context
Selenium Manager executes different commands in the shell to discover the browser version. For instance, for Chrome in Linux:
This kind of command is unavailable in Windows; therefore, Selenium Manager uses WMIC (Windows Management Instrumentation Command-line) to discover browser versions in Windows (
wmic datafile where name=...). For instance:WMIC has been deprecated as of Windows 10, version 21H1, and Windows Server 2022 (see info). Microsoft has announced that it will be removed in a future version of Windows, though no specific timeline has been provided for its complete removal. Microsoft recommends transitioning to alternative tools and methods, such as PowerShell, which offers more robust and modern management capabilities.
WMIC continues working in Windows 11 currently. However, we need to stop using it in Selenium Manager. This PR implements this change, as follows:
I believe using PowerShell in Windows (instead of the regular
cmd.exe) is pretty safe since PowerShell is pre-installed starting with Windows 7. However, I detected a problem with this approach: performance. Running PowerShell commands through Selenium Manager is slower than WMIC commands. For example, in the first snippet of this PR, Selenium Manager used 131ms to run the WMIC command (19.807-19.676=0.131s). For PowerShell, the command took 683ms (28.434-27.751=0.683s). I repeated this, and indeed, it seems that a PowerShell command took around ~700ms, while WMIC required ~100ms:PowerShell:
28.434-27.751=0.683
15.239-14.563=0.676
38.777-38.106=0.671
13.743-13.064=0.679
11.833-11.080=0.753
20.733-20.051=0.682
53.445-52.742=0.703
WMIC:
19.807-19.676=0.131
53.781-53.652=0.129
25.457-25.345=0.112
28.647-28.536=0.111
This means that for any new Selenium session, SM would have an overhead of around 700ms. I suppose this figure can be superior in other Windows systems. In my opinion, this time is quite relevant, and we would need to improve it.
Luckily, I implemented a mechanism to cache the resolution (i.e., the discovered browser version using shell commands) using the old TTL for browsers. After discussing it, we decided to remove that feature, and since then, commands are always executed to discover browser versions. But with this new scenario (we need to move to PowerShell in Windows at some point), I propose to recover that feature and use TTL for commands. The value we used for this value was 3600s.
@diemol @titusfortner What do you think?
Types of changes
Checklist
PR Type
Enhancement
Description
Replaced deprecated WMIC commands with PowerShell commands in Windows.
Updated command definitions for OS architecture and MSI installation.
Modified shell execution to use PowerShell for Windows.
Adjusted related logic and constants to align with PowerShell usage.
Changes walkthrough 📝
config.rs
Switch OS architecture detection to PowerShellrust/src/config.rs
detection.
files.rs
Use PowerShell for MSI installation commandsrust/src/files.rs
lib.rs
Replace WMIC commands with PowerShell in constants and logicrust/src/lib.rs
detection.
shell.rs
Use PowerShell as default shell for Windowsrust/src/shell.rs