Skip to content

Conversation

boomanaiden154
Copy link
Contributor

Lit's builtin diff command had some Python 2.7 code paths lying around
that we can probably get rid of at this point. LLVM at this point
requires Python 3.8 at minimum. Keeping lit working at a lower version
is a reasonable goal, but I think we can probably drop python 2 support
at this point given how long it has been deprecated and how long LLVM
has supported Python 3.

@llvmbot
Copy link
Member

llvmbot commented Sep 8, 2025

@llvm/pr-subscribers-testing-tools

Author: Aiden Grossman (boomanaiden154)

Changes

Lit's builtin diff command had some Python 2.7 code paths lying around
that we can probably get rid of at this point. LLVM at this point
requires Python 3.8 at minimum. Keeping lit working at a lower version
is a reasonable goal, but I think we can probably drop python 2 support
at this point given how long it has been deprecated and how long LLVM
has supported Python 3.


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

1 Files Affected:

  • (modified) llvm/utils/lit/lit/builtin_commands/diff.py (+10-31)
diff --git a/llvm/utils/lit/lit/builtin_commands/diff.py b/llvm/utils/lit/lit/builtin_commands/diff.py
index fbf4eb0e137b3..f2b5869b35889 100644
--- a/llvm/utils/lit/lit/builtin_commands/diff.py
+++ b/llvm/utils/lit/lit/builtin_commands/diff.py
@@ -59,30 +59,15 @@ def compareTwoFiles(flags, filepaths):
 
 def compareTwoBinaryFiles(flags, filepaths, filelines):
     exitCode = 0
-    if hasattr(difflib, "diff_bytes"):
-        # python 3.5 or newer
-        diffs = difflib.diff_bytes(
-            difflib.unified_diff,
-            filelines[0],
-            filelines[1],
-            filepaths[0].encode(),
-            filepaths[1].encode(),
-            n=flags.num_context_lines,
-        )
-        diffs = [diff.decode(errors="backslashreplace") for diff in diffs]
-    else:
-        # python 2.7
-        if flags.unified_diff:
-            func = difflib.unified_diff
-        else:
-            func = difflib.context_diff
-        diffs = func(
-            filelines[0],
-            filelines[1],
-            filepaths[0],
-            filepaths[1],
-            n=flags.num_context_lines,
-        )
+    diffs = difflib.diff_bytes(
+        difflib.unified_diff,
+        filelines[0],
+        filelines[1],
+        filepaths[0].encode(),
+        filepaths[1].encode(),
+        n=flags.num_context_lines,
+    )
+    diffs = [diff.decode(errors="backslashreplace") for diff in diffs]
 
     for diff in diffs:
         sys.stdout.write(to_string(diff))
@@ -230,14 +215,8 @@ def compareDirTrees(flags, dir_trees, base_paths=["", ""]):
 
 def main(argv):
     if sys.platform == "win32":
-        if hasattr(sys.stdout, "buffer"):
-            # python 3
-            sys.stdout = io.TextIOWrapper(sys.stdout.buffer, newline="\n")
-        else:
-            # python 2.7
-            import msvcrt
+        sys.stdout = io.TextIOWrapper(sys.stdout.buffer, newline="\n")
 
-            msvcrt.setmode(sys.stdout.fileno(), os.O_BINARY)
     args = argv[1:]
     try:
         opts, args = getopt.gnu_getopt(args, "wbuI:U:r", ["strip-trailing-cr"])

Copy link
Contributor

@ilovepi ilovepi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine to me, especially since it follows our written policy on version support.

Does this need any kind of release note? It's a pure dev dependency, so I'd say "no", but I could also see how a consumer would have wanted to know about it.

@boomanaiden154
Copy link
Contributor Author

Does this need any kind of release note? It's a pure dev dependency, so I'd say "no", but I could also see how a consumer would have wanted to know about it.

It doesn't hurt to add one, so I'll throw one in.

Created using spr 1.3.6

[skip ci]
Created using spr 1.3.6
@boomanaiden154 boomanaiden154 changed the base branch from users/boomanaiden154/main.lit-remove-python-27-code-paths-in-builtin-diff to main September 9, 2025 03:19
@boomanaiden154 boomanaiden154 merged commit 9bec262 into main Sep 9, 2025
12 of 17 checks passed
@boomanaiden154 boomanaiden154 deleted the users/boomanaiden154/lit-remove-python-27-code-paths-in-builtin-diff branch September 9, 2025 03:19
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request Sep 9, 2025
Lit's builtin diff command had some Python 2.7 code paths lying around
that we can probably get rid of at this point. LLVM at this point
requires Python 3.8 at minimum. Keeping lit working at a lower version
is a reasonable goal, but I think we can probably drop python 2 support
at this point given how long it has been deprecated and how long LLVM
has supported Python 3.

Reviewers: jdenny-ornl, ilovepi, petrhosek

Reviewed By: ilovepi

Pull Request: llvm/llvm-project#157558
schwitanski pushed a commit to RWTH-HPC/llvm-lit-mirror that referenced this pull request Sep 10, 2025
Lit's builtin diff command had some Python 2.7 code paths lying around
that we can probably get rid of at this point. LLVM at this point
requires Python 3.8 at minimum. Keeping lit working at a lower version
is a reasonable goal, but I think we can probably drop python 2 support
at this point given how long it has been deprecated and how long LLVM
has supported Python 3.

Reviewers: jdenny-ornl, ilovepi, petrhosek

Reviewed By: ilovepi

Pull Request: llvm/llvm-project#157558
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants