Fix Windows 11 identification in wails doctor
#3891
Conversation
WalkthroughThe pull request introduces enhancements to the Windows operating system information retrieval in the Wails project. New functions have been added to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
v2/internal/system/operatingsystem/os_windows.go (1)
29-36: Consider improvements to the Windows version detection logicWhile the current implementation correctly identifies Windows 11, consider these improvements:
- Define
22000as a named constant for better maintainability- Add a comment about Windows Server handling
- Consider more robust product name replacement
//go:build windows package operatingsystem +const ( + // Build number that distinguishes Windows 11 from Windows 10 + // https://en.wikipedia.org/wiki/List_of_Microsoft_Windows_versions#Personal_computer_versions + windows11MinBuildNumber = 22000 +) + // ... existing imports ... func platformInfo() (*OS, error) { // ... existing code ... // Windows 10 and 11 both declare the product name as "Windows 10" // We determine the difference using the build number: // https://en.wikipedia.org/wiki/List_of_Microsoft_Windows_versions#Personal_computer_versions + // Note: This logic applies to consumer Windows versions. Windows Server versions may need different handling. if buildNum, err := strconv.Atoi(currentBuild); err == nil { - if buildNum >= 22000 { + if buildNum >= windows11MinBuildNumber { productName = strings.Replace(productName, "Windows 10", "Windows 11", -1) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
v2/internal/system/operatingsystem/os_windows.go(2 hunks)website/src/pages/changelog.mdx(1 hunks)
🔇 Additional comments (3)
v2/internal/system/operatingsystem/os_windows.go (2)
7-8: LGTM: Required imports added
The new imports are necessary for the Windows 11 detection logic: strconv for build number conversion and strings for product name manipulation.
29-36: Verify Windows version detection across different editions
The current implementation needs verification across different Windows editions and versions.
✅ Verification successful
Let me gather more information about the Windows version detection implementation and tests.
Windows version detection implementation is robust and well-structured
The implementation is actually quite comprehensive and reliable:
- Uses official Windows registry keys from
SOFTWARE\Microsoft\Windows NT\CurrentVersion - Has proper build number detection (22000+) for Windows 11 distinction
- Includes a dedicated
WindowsVersionInfostruct with version comparison capabilities - Handles error cases gracefully with fallback values
- Uses multiple version identifiers (build number, release ID, display version) for accurate detection
The code follows best practices and official Microsoft documentation for Windows version detection. No issues found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing Windows version detection tests
echo "Searching for Windows version detection tests..."
rg -l "TestPlatformInfo|Windows.*detection|build.*number" --type go
# Look for other Windows registry key usages
echo "Checking other Windows registry key usages..."
ast-grep --pattern 'registry.OpenKey($_, $_, $_)'
# Check for potential version detection patterns
echo "Checking for version detection patterns..."
rg "Windows (10|11|Server)" --type go
Length of output: 2394
Script:
#!/bin/bash
# Check the implementation details of version detection
echo "Checking version detection implementation..."
cat v2/internal/system/operatingsystem/os_windows.go
cat v2/internal/system/operatingsystem/version_windows.go
# Look for test files
echo "Checking for test files..."
fd "_test.go" --type f --exec grep -l "Windows" {}
Length of output: 3098
website/src/pages/changelog.mdx (1)
27-27: LGTM! Well-documented changelog entry.
The changelog entry follows best practices by:
- Clearly describing what was fixed
- Attributing the change to the contributor
- Being placed in the correct "Fixed" section
|
Thanks for taking the time to open this. If it's a big concern, I'd suggest back porting the code that's in v3 which picks up the DisplayVersion key. Would be easier than managing version numbers which doesn't always work unfortunately. |
|
Hi, thanks for the quick reply! It's really not a big deal, and I see that in v3 alpha there's a "Branding" entry which pretty much does the trick. In reality I was just looking for a relatively easy first issue to start contributing to the project :-) I'm closing this PR. Thanks again! |
|
I'm happy for this PR to go ahead! I was just pointing out that there's a reg key that will save you calculating versions 😃 |
|
Whoops, sorry for misunderstanding. Will fix soon and commit for review :-) |
|
Hi @ronen25 - what's the status of this? Thanks 👍 |
|
@leaanthony Apologies, life got in the way :-) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
v2/internal/system/operatingsystem/os_windows.go (1)
15-23: Simplify the stripNulls function.The current implementation can be simplified using
strings.ReplaceAll.func stripNulls(str string) string { - // Split the string into substrings at each null character - substrings := strings.Split(str, "\x00") - - // Join the substrings back into a single string - strippedStr := strings.Join(substrings, "") - - return strippedStr + return strings.ReplaceAll(str, "\x00", "") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
v2/cmd/wails/doctor.go(2 hunks)v2/internal/system/operatingsystem/os.go(1 hunks)v2/internal/system/operatingsystem/os_windows.go(2 hunks)
🔇 Additional comments (4)
v2/internal/system/operatingsystem/os.go (1)
5-8: LGTM! Clean addition of the Branding field.The addition of the
Brandingfield to theOSstruct is well-structured and aligns with the PR objectives.v2/internal/system/operatingsystem/os_windows.go (2)
64-64: LGTM! Clean integration of branding information.The addition of branding information to the
platformInfofunction is straightforward and well-integrated.
34-43: Document the usage of %WINDOWS_LONG% format string.The
getBrandingfunction uses a hardcoded format string without documentation explaining its purpose or possible values.Add a comment explaining the purpose and possible values of the format string:
func getBranding() string { + // %WINDOWS_LONG% is a format string used by winbrand.dll to retrieve the full Windows branding + // It returns strings like "Windows 11 Pro" or "Windows 10 Enterprise" var modBranding = syscall.NewLazyDLL("winbrand.dll") var brandingFormatString = modBranding.NewProc("BrandingFormatString")Also, consider error handling for the DLL and procedure calls:
✅ Verification successful
Document the format string but error handling is optional
The suggestion to document the
%WINDOWS_LONG%format string is valid and will improve code maintainability. However, the error handling suggestion can be safely ignored as:
- The current implementation follows the codebase's pattern for non-critical UI operations
- winbrand.dll is a core Windows component, similar to user32.dll
- The function is used for display purposes only
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify if winbrand.dll is consistently available across Windows versions rg -g "*.go" "winbrand\.dll" .Length of output: 137
Script:
#!/bin/bash # Check error handling patterns in Windows DLL calls ast-grep --pattern 'NewLazyDLL($$$)' # Look for similar Windows API documentation patterns rg -g "*.go" -B 2 "syscall\.NewLazyDLL" . # Check for other Windows-specific error handling rg -g "*_windows.go" "_, err :=" .Length of output: 6773
v2/cmd/wails/doctor.go (1)
82-82: LGTM! Clean addition of branding information to diagnostics.The addition of the branding information to the system diagnostics output is well-placed and consistent with the existing format.
| func mustStringToUTF16Ptr(input string) *uint16 { | ||
| input = stripNulls(input) | ||
| result, err := syscall.UTF16PtrFromString(input) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| return result | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider error handling instead of panic.
The mustStringToUTF16Ptr function panics on error, which could make the application unstable. Consider handling the error gracefully.
-func mustStringToUTF16Ptr(input string) *uint16 {
+func stringToUTF16Ptr(input string) (*uint16, error) {
input = stripNulls(input)
result, err := syscall.UTF16PtrFromString(input)
- if err != nil {
- panic(err)
- }
- return result
+ return result, err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func mustStringToUTF16Ptr(input string) *uint16 { | |
| input = stripNulls(input) | |
| result, err := syscall.UTF16PtrFromString(input) | |
| if err != nil { | |
| panic(err) | |
| } | |
| return result | |
| } | |
| func stringToUTF16Ptr(input string) (*uint16, error) { | |
| input = stripNulls(input) | |
| result, err := syscall.UTF16PtrFromString(input) | |
| return result, err | |
| } |
|
Thanks 🙏 Please could you add an entry to the changelog located at |
|
@leaanthony Done |
Description
Fixes #3890
This PR fixes a small issue in
wails doctor: Windows 11 installations are reported as "Windows 10".The
SOFTWARE\Microsoft\Windows NT\CurrentVersion\ProductNamekey which Wails uses to identify the Windows version does indeed report "Windows 10" on Windows 11 systems.In this case, we have no choice but to rely on the build number to correctly identify the system. I've used a Wikipedia list (documented in the PR) to determine that any consumer version of Windows with a build number greater than
22000is Windows 11.Note: Windows Server versions are probably incorrectly reported as well, but at time of writing I don't have any VMs to test with. Anyways, this PR will lay some small groundwork for fixing that as well if the need arises.
It's really not a big deal, and more of an oddity on Windows' side, and I've seen this misreporting happen in many programs.
Type of change
Please select the option that is relevant.
How Has This Been Tested?
Tested by running
wails doctorand seeing the correct OS name reported on every test system.Tested on:
I have access to a Mac system, could test for regressions although I don't think it's necessary...
If you checked Linux, please specify the distro and version.
Test Configuration
Checklist:
website/src/pages/changelog.mdxwith details of this PRSummary by CodeRabbit
New Features
Improvements
wails doctor.