[lldb][windows] refactor the version check in @skipIfWindows #172838

charles-zablit · 2025-12-18T11:32:27Z

This patch refactors the way we check for the windows version in the @skipIfWindows decorator.

The new logic reuses the expectedCompilerVersion method logic for the parsing and comparison of the version.

llvmbot · 2025-12-18T11:33:03Z

@llvm/pr-subscribers-platform-windows

@llvm/pr-subscribers-lldb

Author: Charles Zablit (charles-zablit)

Changes

This patch refactors the way we check for the windows version in the @skipIfWindows decorator.

The new logic reuses the expectedCompilerVersion method logic for the parsing and comparison of the version.

Full diff: https://github.com/llvm/llvm-project/pull/172838.diff

3 Files Affected:

(modified) lldb/packages/Python/lldbsuite/test/decorators.py (+10-12)
(modified) lldb/packages/Python/lldbsuite/test/lldbplatformutil.py (+52-16)
(modified) lldb/test/API/tools/lldb-dap/launch/TestDAP_launch.py (+8-8)

diff --git a/lldb/packages/Python/lldbsuite/test/decorators.py b/lldb/packages/Python/lldbsuite/test/decorators.py
index 6f388cb090f41..8126b39348090 100644
--- a/lldb/packages/Python/lldbsuite/test/decorators.py
+++ b/lldb/packages/Python/lldbsuite/test/decorators.py
@@ -781,30 +781,28 @@ def skipIfLinux(func):
     return skipIfPlatform(["linux"])(func)
 
 
-def skipIfWindows(func=None, major=None, build=None):
+def skipIfWindows(func=None, windows_version=None):
     """Decorate the item to skip tests that should be skipped on Windows."""
 
     def decorator(func):
-        if major is None and build is None:
+        if windows_version is None:
             return skipIfPlatform(["windows"])(func)
         else:
-            import platform
-            import sys
+            actual_win_version = lldbplatformutil.getWindowsVersion()
 
             def version_check():
-                check_major = 0 if major is None else major
-                check_build = 0 if build is None else build
-                if platform.system() != "Windows":
+                if actual_win_version == "unknown":
                     return False
-                win_version = sys.getwindowsversion()
-                return (
-                    win_version.major >= check_major
-                    and win_version.build >= check_build
+                operator, required_windows_version = windows_version
+                return lldbplatformutil.isExpectedVersion(
+                    actual_version=actual_win_version,
+                    required_version=required_windows_version,
+                    operator=operator,
                 )
 
             return unittest.skipIf(
                 version_check(),
-                f"Test is skipped on Windows major={major} build={build}",
+                f"Test is skipped on Windows '{actual_win_version}'",
             )(func)
 
     if func is not None:
diff --git a/lldb/packages/Python/lldbsuite/test/lldbplatformutil.py b/lldb/packages/Python/lldbsuite/test/lldbplatformutil.py
index cea6270695dc0..1e08f0715eefd 100644
--- a/lldb/packages/Python/lldbsuite/test/lldbplatformutil.py
+++ b/lldb/packages/Python/lldbsuite/test/lldbplatformutil.py
@@ -8,6 +8,7 @@
 import subprocess
 import sys
 import os
+from typing import Optional
 from packaging import version
 from urllib.parse import urlparse
 
@@ -300,6 +301,24 @@ def getCompilerVersion():
     return "unknown"
 
 
+def getWindowsVersion():
+    """Returns a string that represents the Windows version.
+    
+    The string is a concatenation of the following, eparated by a dot:
+      - The major version number.
+      - The build number.
+    
+    Example:
+      - Windows 11 version 24H2 -> "10.26100"
+      - Windows 10 version 1809 -> "10.17763"
+    """
+    import sys
+    if sys.platform != "win32":
+        return "unknown"
+    windows_version = sys.getwindowsversion()
+    return f"{windows_version.major}.{windows_version.build}"
+
+
 def getDwarfVersion():
     """Returns the dwarf version generated by clang or '0'."""
     if configuration.dwarf_version:
@@ -322,8 +341,34 @@ def getDwarfVersion():
     return "0"
 
 
+def isExpectedVersion(
+    actual_version: str, required_version: str, operator: str
+) -> bool:
+    """Returns True if actual_version matches the required_version given the operator.
+    Any operator other than the following defaults to an equality test:
+        '>', '>=', "=>", '<', '<=', '=<', '!=', "!" or 'not'
+
+    Example:
+      - actual_version='1.2.0', required_version='1.0.0', operator='>=' returns True
+    """
+    actual_version_ = version.parse(actual_version)
+    required_version_ = version.parse(required_version)
+
+    if operator == ">":
+        return actual_version_ > required_version_
+    if operator == ">=" or operator == "=>":
+        return actual_version_ >= required_version_
+    if operator == "<":
+        return actual_version_ < required_version_
+    if operator == "<=" or operator == "=<":
+        return actual_version_ <= required_version_
+    if operator == "!=" or operator == "!" or operator == "not":
+        return actual_version not in required_version
+    return actual_version in required_version
+
+
 def expectedCompilerVersion(compiler_version):
-    """Returns True iff compiler_version[1] matches the current compiler version.
+    """Returns True if compiler_version[1] matches the current compiler version.
     Use compiler_version[0] to specify the operator used to determine if a match has occurred.
     Any operator other than the following defaults to an equality test:
         '>', '>=', "=>", '<', '<=', '=<', '!=', "!" or 'not'
@@ -342,23 +387,14 @@ def expectedCompilerVersion(compiler_version):
 
     test_compiler_version_str = getCompilerVersion()
     if test_compiler_version_str == "unknown":
-        # Assume the compiler version is at or near the top of trunk.
+        # Assume the version is at or near the top of trunk.
         return operator in [">", ">=", "!", "!=", "not"]
 
-    actual_version = version.parse(version_str)
-    test_compiler_version = version.parse(test_compiler_version_str)
-
-    if operator == ">":
-        return test_compiler_version > actual_version
-    if operator == ">=" or operator == "=>":
-        return test_compiler_version >= actual_version
-    if operator == "<":
-        return test_compiler_version < actual_version
-    if operator == "<=" or operator == "=<":
-        return test_compiler_version <= actual_version
-    if operator == "!=" or operator == "!" or operator == "not":
-        return version_str not in test_compiler_version_str
-    return version_str in test_compiler_version_str
+    return isExpectedVersion(
+        actual_version=test_compiler_version_str,
+        required_version=version_str,
+        operator=operator,
+    )
 
 
 def expectedCompiler(compilers):
diff --git a/lldb/test/API/tools/lldb-dap/launch/TestDAP_launch.py b/lldb/test/API/tools/lldb-dap/launch/TestDAP_launch.py
index 2fdf1bb42ca09..422d589a11bc5 100644
--- a/lldb/test/API/tools/lldb-dap/launch/TestDAP_launch.py
+++ b/lldb/test/API/tools/lldb-dap/launch/TestDAP_launch.py
@@ -16,7 +16,7 @@
 
 
 class TestDAP_launch(lldbdap_testcase.DAPTestCaseBase):
-    @skipIfWindows(major=10, build=1809)
+    @skipIfWindows(windows_version=['>', '10.17763'])
     def test_default(self):
         """
         Tests the default launch of a simple program. No arguments,
@@ -76,7 +76,7 @@ def test_failing_console(self):
             r"unexpected value, expected 'internalConsole\', 'integratedTerminal\' or 'externalTerminal\' at arguments.console",
         )
 
-    @skipIfWindows(major=10, build=1809)
+    @skipIfWindows(windows_version=['>', '10.17763'])
     def test_termination(self):
         """
         Tests the correct termination of lldb-dap upon a 'disconnect'
@@ -210,7 +210,7 @@ def test_disableSTDIO(self):
         self.assertEqual(output, "", "expect no program output")
 
     @skipIfLinux  # shell argument expansion doesn't seem to work on Linux
-    @skipIfWindows(major=10, build=1809)
+    @skipIfWindows(windows_version=['>', '10.17763'])
     @expectedFailureAll(oslist=["freebsd", "netbsd"], bugnumber="llvm.org/pr48349")
     def test_shellExpandArguments_enabled(self):
         """
@@ -233,7 +233,7 @@ def test_shellExpandArguments_enabled(self):
                     quote_path, line, 'verify "%s" expanded to "%s"' % (glob, program)
                 )
 
-    @skipIfWindows(major=10, build=1809)
+    @skipIfWindows(windows_version=['>', '10.17763'])
     def test_shellExpandArguments_disabled(self):
         """
         Tests the default launch of a simple program with shell expansion
@@ -255,7 +255,7 @@ def test_shellExpandArguments_disabled(self):
                     quote_path, line, 'verify "%s" stayed to "%s"' % (glob, glob)
                 )
 
-    @skipIfWindows(major=10, build=1809)
+    @skipIfWindows(windows_version=['>', '10.17763'])
     def test_args(self):
         """
         Tests launch of a simple program with arguments
@@ -280,7 +280,7 @@ def test_args(self):
                 'arg[%i] "%s" not in "%s"' % (i + 1, quoted_arg, lines[i]),
             )
 
-    @skipIfWindows(major=10, build=1809)
+    @skipIfWindows(windows_version=['>', '10.17763'])
     def test_environment_with_object(self):
         """
         Tests launch of a simple program with environment variables
@@ -557,7 +557,7 @@ def test_terminate_commands(self):
         output = self.collect_console(pattern=terminateCommands[0])
         self.verify_commands("terminateCommands", output, terminateCommands)
 
-    @skipIfWindows(major=10, build=1809)
+    @skipIfWindows(windows_version=['>', '10.17763'])
     def test_version(self):
         """
         Tests that "initialize" response contains the "version" string the same
@@ -640,7 +640,7 @@ def test_stdio_redirection(self):
             )
 
     @skipIfAsan
-    @skipIfWindows(major=10, build=1809)
+    @skipIfWindows(windows_version=['>', '10.17763'])
     @skipIf(oslist=["linux"], archs=no_match(["x86_64"]))
     @skipIfBuildType(["debug"])
     def test_stdio_redirection_and_console(self):

github-actions · 2025-12-18T11:34:32Z

✅ With the latest revision this PR passed the Python code formatter.

DavidSpickett · 2025-12-18T11:49:25Z

lldb/packages/Python/lldbsuite/test/lldbplatformutil.py

+def getWindowsVersion():
+    """Returns a string that represents the Windows version.
+
+    The string is a concatenation of the following, eparated by a dot:


eparated -> separated

Fixed, thanks!

lldb/packages/Python/lldbsuite/test/lldbplatformutil.py

da-viper · 2025-12-18T13:24:10Z

lldb/test/API/tools/lldb-dap/launch/TestDAP_launch.py


 class TestDAP_launch(lldbdap_testcase.DAPTestCaseBase):
-    @skipIfWindows(major=10, build=1809)
+    @skipIfWindows(windows_version=[">", "10.17763"])


are we only enabling the test for older versions ?

I read this incorrectly, fixed, thanks!

da-viper · 2025-12-18T13:46:00Z

lldb/packages/Python/lldbsuite/test/lldbplatformutil.py

+    if operator == "<=" or operator == "=<":
+        return actual_version_ <= required_version_
+    if operator == "!=" or operator == "!" or operator == "not":
+        return actual_version not in required_version


We could use the parsed versions here as it takes care of extra spaces for the != and == cases.

I think this would lead to unexpected test failures due to #172838 (comment).
There might be some versions that do not get parsed correctly or have more than just whitespaces? Like v1.0.0.

da-viper · 2025-12-18T13:50:49Z

lldb/packages/Python/lldbsuite/test/lldbplatformutil.py

+    if sys.platform != "win32":
+        return "unknown"
+    windows_version = sys.getwindowsversion()
+    return f"{windows_version.major}.{windows_version.build}"


should this build ID match the ID in the name or Build ID column? see https://en.wikipedia.org/wiki/List_of_Microsoft_Windows_versions.

because the usage has changed.

I made sure it matches the versions of the PS command, please see: #172838 (comment)

DavidSpickett

LGTM

I never had to check one of these skips before but if I did the first question would be whether the build numbers are exact. Which is what it looks like.

This new version makes it clear what we're checking for, which I like.

[lldb][windows] refactor the version check in @skipIfWindows

20f30d6

charles-zablit requested a review from JDevlieghere as a code owner December 18, 2025 11:32

charles-zablit requested review from adrian-prantl and da-viper December 18, 2025 11:32

llvmbot added the lldb label Dec 18, 2025

charles-zablit self-assigned this Dec 18, 2025

charles-zablit added the platform:windows label Dec 18, 2025

charles-zablit mentioned this pull request Dec 18, 2025

[lldb][windows] add Windows Virtual Console support #168729

Merged

fixup! [lldb][windows] refactor the version check in @skipIfWindows

be32243

DavidSpickett reviewed Dec 18, 2025

View reviewed changes

da-viper reviewed Dec 18, 2025

View reviewed changes

address comments

eb209f0

DavidSpickett approved these changes Dec 18, 2025

View reviewed changes

da-viper approved these changes Dec 18, 2025

View reviewed changes

charles-zablit merged commit 2ec3ea7 into llvm:main Dec 18, 2025
10 of 11 checks passed

[lldb][windows] refactor the version check in @skipIfWindows #172838

[lldb][windows] refactor the version check in @skipIfWindows #172838

Uh oh!

Conversation

charles-zablit commented Dec 18, 2025

Uh oh!

llvmbot commented Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DavidSpickett Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

charles-zablit Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

da-viper Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

charles-zablit Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

da-viper Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

charles-zablit Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

da-viper Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

charles-zablit Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavidSpickett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

llvmbot commented Dec 18, 2025 •

edited

Loading

github-actions bot commented Dec 18, 2025 •

edited

Loading

charles-zablit Dec 18, 2025 •

edited

Loading