[lit] Remove support for %T

[boomanaiden154]

This patch removes support for %T from llvm-lit. For now we mark the
test unresolved and add an error message noting the substitution is
deprecated. This is exactly the same as the error handling for other
substitution failures. We intend to remove support for the nice error
message once 22 branches as users should have moved over by the they are
upgrading to v23.

[llvmbot]

@llvm/pr-subscribers-testing-tools

Author: Aiden Grossman (boomanaiden154)

Changes

This patch removes support for %T from llvm-lit. For now we mark the
test unresolved and add an error message noting the substitution is
deprecated. This is exactly the same as the error handling for other
substitution failures. We intend to remove support for the nice error
message once 22 branches as users should have moved over by the they are
upgrading to v23.

---

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

5 Files Affected:

• (modified) llvm/docs/CommandGuide/lit.rst (-6)
• (modified) llvm/docs/ReleaseNotes.md (+1)
• (modified) llvm/utils/lit/lit/TestRunner.py (+12-2)
• (added) llvm/utils/lit/tests/Inputs/shtest-shell/capital-t-error-message.txt (+2)
• (modified) llvm/utils/lit/tests/shtest-shell.py (+5-1)

diff --git a/llvm/docs/CommandGuide/lit.rst b/llvm/docs/CommandGuide/lit.rst
index 359e0c3e81d0e..6a721ebf9cad0 100644
--- a/llvm/docs/CommandGuide/lit.rst
+++ b/llvm/docs/CommandGuide/lit.rst
@@ -630,13 +630,11 @@ TestRunner.py:
  %{fs-sep}               file system path separator
  %t                      temporary file name unique to the test
  %basename_t             The last path component of %t but without the ``.tmp`` extension (deprecated, use ``%{t:stem}`` instead)
- %T                      parent directory of %t (not unique, deprecated, do not use)
  %%                      %
  %/s                     %s but ``\`` is replaced by ``/``
  %/S                     %S but ``\`` is replaced by ``/``
  %/p                     %p but ``\`` is replaced by ``/``
  %/t                     %t but ``\`` is replaced by ``/``
- %/T                     %T but ``\`` is replaced by ``/``
  %{s:basename}           The last path component of %s
  %{t:stem}               The last path component of %t but without the ``.tmp`` extension (alias for %basename_t)
  %{s:real}               %s after expanding all symbolic links and substitute drives
@@ -648,12 +646,10 @@ TestRunner.py:
  %{/S:real}              %/S after expanding all symbolic links and substitute drives
  %{/p:real}              %/p after expanding all symbolic links and substitute drives
  %{/t:real}              %/t after expanding all symbolic links and substitute drives
- %{/T:real}              %/T after expanding all symbolic links and substitute drives
  %{/s:regex_replacement} %/s but escaped for use in the replacement of a ``s@@@`` command in sed
  %{/S:regex_replacement} %/S but escaped for use in the replacement of a ``s@@@`` command in sed
  %{/p:regex_replacement} %/p but escaped for use in the replacement of a ``s@@@`` command in sed
  %{/t:regex_replacement} %/t but escaped for use in the replacement of a ``s@@@`` command in sed
- %{/T:regex_replacement} %/T but escaped for use in the replacement of a ``s@@@`` command in sed
  %:s                     On Windows, %/s but a ``:`` is removed if its the second character.
                          Otherwise, %s but with a single leading ``/`` removed.
  %:S                     On Windows, %/S but a ``:`` is removed if its the second character.
@@ -662,8 +658,6 @@ TestRunner.py:
                          Otherwise, %p but with a single leading ``/`` removed.
  %:t                     On Windows, %/t but a ``:`` is removed if its the second character.
                          Otherwise, %t but with a single leading ``/`` removed.
- %:T                     On Windows, %/T but a ``:`` is removed if its the second character.
-                         Otherwise, %T but with a single leading ``/`` removed.
  %{readfile:<filename>}  Reads the file specified.
  ======================= ==============
 
diff --git a/llvm/docs/ReleaseNotes.md b/llvm/docs/ReleaseNotes.md
index c211844c62491..0fc95aff83821 100644
--- a/llvm/docs/ReleaseNotes.md
+++ b/llvm/docs/ReleaseNotes.md
@@ -159,6 +159,7 @@ Changes to the LLVM tools
 
 * `llvm-readelf` now dumps all hex format values in lower-case mode.
 * Some code paths for supporting Python 2.7 in `llvm-lit` have been removed.
+* Support for `%T` in lit has been removed.
 
 Changes to LLDB
 ---------------------------------
diff --git a/llvm/utils/lit/lit/TestRunner.py b/llvm/utils/lit/lit/TestRunner.py
index 9ae8ac75bee08..a7e2705f609af 100644
--- a/llvm/utils/lit/lit/TestRunner.py
+++ b/llvm/utils/lit/lit/TestRunner.py
@@ -1541,8 +1541,10 @@ def regex_escape(s):
         return s
 
     path_substitutions = [
-        ("s", sourcepath), ("S", sourcedir), ("p", sourcedir),
-        ("t", tmpName), ("T", tmpDir)
+        ("s", sourcepath),
+        ("S", sourcedir),
+        ("p", sourcedir),
+        ("t", tmpName),
     ]
     for path_substitution in path_substitutions:
         letter = path_substitution[0]
@@ -1919,6 +1921,14 @@ def processLine(ln):
             # seems reasonable.
             ln = _caching_re_compile(a).sub(str(b), escapePercents(ln))
 
+        # TODO(boomanaiden154): Remove when we branch LLVM 22 so people on the
+        # release branch will have sufficient time to migrate.
+        if bool(_caching_re_compile("%T").search(ln)):
+            raise ValueError(
+                "%T is no longer supported. Please create directories with names "
+                "based on %t."
+            )
+
         # Strip the trailing newline and any extra whitespace.
         return ln.strip()
 
diff --git a/llvm/utils/lit/tests/Inputs/shtest-shell/capital-t-error-message.txt b/llvm/utils/lit/tests/Inputs/shtest-shell/capital-t-error-message.txt
new file mode 100644
index 0000000000000..e69dfee8fced8
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/shtest-shell/capital-t-error-message.txt
@@ -0,0 +1,2 @@
+# Check that we return a decent error message when someone uses %T
+# RUN: echo %T > %t
diff --git a/llvm/utils/lit/tests/shtest-shell.py b/llvm/utils/lit/tests/shtest-shell.py
index 498f6bb0adc11..38db1b75486cf 100644
--- a/llvm/utils/lit/tests/shtest-shell.py
+++ b/llvm/utils/lit/tests/shtest-shell.py
@@ -12,6 +12,10 @@
 
 # CHECK: -- Testing:
 
+# CHECK: UNRESOLVED: shtest-shell :: capital-t-error-message.txt
+# CHECK: *** TEST 'shtest-shell :: capital-t-error-message.txt' FAILED ***
+# CHECK: ValueError: %T is no longer supported. Please create directories with names based on %t.
+
 # CHECK: FAIL: shtest-shell :: colon-error.txt
 # CHECK: *** TEST 'shtest-shell :: colon-error.txt' FAILED ***
 # CHECK: :
@@ -633,5 +637,5 @@
 #      CHECK: ***
 
 # CHECK: PASS: shtest-shell :: valid-shell.txt
-# CHECK: Unresolved Tests (1)
+# CHECK: Unresolved Tests (2)
 # CHECK: Failed Tests (37)

[jh7370]

LGTM, but best wait to give others a chance to review.

[jh7370]

Suggested change

	# Check that we return a decent error message when someone uses %T
	# Check that we return a decent error message when someone uses %T.

[Abhishek-Varma]

Hi @boomanaiden154

Based on this PR I tried adapting one of our downstream projects but failed.

So, I replaced %T with %t.dir - this worked for Linux but failed on Windows.
I then tried replacing %T with %{fs-tmp-root} but to no avail.

Can you please guide on how the downstream projects should adapt to this change ?

[jh7370]

What was your downstream test trying to do with %T?

%T expanded to a directory name based on the name of the test source directory and which will probably already exist. %t expands to a name based on the path to the test source, which probably doesn't exist. %t.dir is nothing special - it's just the %t expansion with ".dir" added to the end. You'll likely need to manually create the directory yourself in this case, e.g. something like the following at the start of the test might be worth doing:

# RUN: rm -rf %t.dir && mkdir %t.dir
# RUN: some-command -o %t.dir/foo

[Abhishek-Varma]

What was your downstream test trying to do with %T?

%T expanded to a directory name based on the name of the test source directory and which will probably already exist. %t expands to a name based on the path to the test source, which probably doesn't exist. %t.dir is nothing special - it's just the %t expansion with ".dir" added to the end. You'll likely need to manually create the directory yourself in this case, e.g. something like the following at the start of the test might be worth doing:

# RUN: rm -rf %t.dir && mkdir %t.dir
# RUN: some-command -o %t.dir/foo

In the downstream project I tried doing the following :-

// RUN: mkdir -p %t.dir
// RUN: (some-command %s %t.dir)

But this works for Linux but for Windows the same lit test doesn't seem to be working and gives lot of File could not be opened, fopen Error: No such file or directory - so was trying to understand how are we supposed to go about this.

Two cases where we currently use %T are :-
CASE 1:

// RUN: some-command %s %T

CASE 2:

// RUN: some-command %s %T
// RUN: some-opt --pass-pipeline{pipeline-flag-using-path=%T} %s

For the above to cases I replaced %T with %t.dir and included a mkdir -p %t.dir but it only worked for Linux and NOT for Windows.

[jh7370]

For the above to cases I replaced %T with %t.dir and included a mkdir -p %t.dir but it only worked for Linux and NOT for Windows.

There's nothing there that is Windows/Linux specific as far as I know. I assume you can run upstream llvm tests without an issue?

The pattern is used commonly in other tests throughout the llvm tools tests. Given this, I don't think there's enough information in your response to help provide more help. Perhaps try running lit with verbose output, so that you can see that actual paths things expand to. Also, perhaps put a printf or similar in the tool around the site where the error is generated to get it to print the path that is trying to be used. That might help you triage things a bit better.

[ldionne]

I just realized that we probably forgot to remove the %{T:real} substitution as part of this patch?
Specifically, there seems to be a stray substitution in the documentation now: https://github.com/llvm/llvm-project/blob/main/llvm/docs/CommandGuide/lit.rst?plain=1#L644

[boomanaiden154]

Thanks for catching that. Not exactly sure how I missed it. Fixed in 9ef226983f3924cb4c5299c2a921654bdf28dc22.