Skip to content

Conversation

jurahul
Copy link
Contributor

@jurahul jurahul commented Oct 3, 2024

Add support for %basename_s pattern in the RUN commands to get the base name of the source file, and adopt it in a TableGen LIT test.

@jurahul jurahul marked this pull request as ready for review October 3, 2024 14:46
@jurahul jurahul requested a review from arichardson October 3, 2024 14:47
@jurahul jurahul requested a review from arsenm October 3, 2024 14:47
@llvmbot
Copy link
Member

llvmbot commented Oct 3, 2024

@llvm/pr-subscribers-testing-tools

Author: Rahul Joshi (jurahul)

Changes

Add support for %basename_s pattern in the RUN commands to get the base name of the source file, and adopt it in a TableGen LIT test.


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

3 Files Affected:

  • (modified) llvm/docs/CommandGuide/lit.rst (+1)
  • (modified) llvm/test/TableGen/anonymous-location.td (+1-1)
  • (modified) llvm/utils/lit/lit/TestRunner.py (+4-2)
diff --git a/llvm/docs/CommandGuide/lit.rst b/llvm/docs/CommandGuide/lit.rst
index c9d5baba3e2f49..9216eb223d1491 100644
--- a/llvm/docs/CommandGuide/lit.rst
+++ b/llvm/docs/CommandGuide/lit.rst
@@ -535,6 +535,7 @@ TestRunner.py:
  %{fs-tmp-root}          root component of file system paths pointing to the test's temporary directory
  %{fs-sep}               file system path separator
  %t                      temporary file name unique to the test
+ %basename_s             The last path component of %s
  %basename_t             The last path component of %t but without the ``.tmp`` extension
  %T                      parent directory of %t (not unique, deprecated, do not use)
  %%                      %
diff --git a/llvm/test/TableGen/anonymous-location.td b/llvm/test/TableGen/anonymous-location.td
index ffeba6ebcb686f..5485d2027b9fd0 100644
--- a/llvm/test/TableGen/anonymous-location.td
+++ b/llvm/test/TableGen/anonymous-location.td
@@ -1,4 +1,4 @@
-// RUN: llvm-tblgen --print-detailed-records %s | FileCheck %s -DFILE=anonymous-location.td
+// RUN: llvm-tblgen --print-detailed-records %s | FileCheck %s -DFILE=%basename_s
 
 class A<int a> {
   int Num = a;
diff --git a/llvm/utils/lit/lit/TestRunner.py b/llvm/utils/lit/lit/TestRunner.py
index a1785073547ad0..080a618572645d 100644
--- a/llvm/utils/lit/lit/TestRunner.py
+++ b/llvm/utils/lit/lit/TestRunner.py
@@ -1394,10 +1394,12 @@ def getDefaultSubstitutions(test, tmpDir, tmpBase, normalize_slashes=False):
     substitutions = []
     substitutions.extend(test.config.substitutions)
     tmpName = tmpBase + ".tmp"
-    baseName = os.path.basename(tmpBase)
+    tmpBaseName = os.path.basename(tmpBase)
+    sourceBaseName = os.path.basename(sourcepath)
 
     substitutions.append(("%{pathsep}", os.pathsep))
-    substitutions.append(("%basename_t", baseName))
+    substitutions.append(("%basename_t", tmpBaseName))
+    substitutions.append(("%basename_s", sourceBaseName))
 
     substitutions.extend(
         [

@jurahul jurahul requested review from ilovepi and jh7370 October 3, 2024 15:19
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 mostly fine, but we should probably have a test under llvm/utils/lit/tests (https://github.com/llvm/llvm-project/tree/main/llvm/utils/lit/tests), rather than just using it in a single tablegen test. Would you mind moving the test there?

@jurahul
Copy link
Contributor Author

jurahul commented Oct 3, 2024

This seems mostly fine, but we should probably have a test under llvm/utils/lit/tests (https://github.com/llvm/llvm-project/tree/main/llvm/utils/lit/tests), rather than just using it in a single tablegen test. Would you mind moving the test there?

Thanks. I'll add a new test instead of moving the TableGen test.

@jurahul
Copy link
Contributor Author

jurahul commented Oct 3, 2024

I added a separate unit test under lit.

@jurahul jurahul requested a review from ilovepi October 3, 2024 16:23
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.

LGTM, modulo one small comment and premerge checks are passing.

Add support for `%basename_s` pattern in the RUN commands to get the
base name of the source file, and adopt it in a TableGen LIT test.
@jurahul jurahul merged commit 6f20c30 into llvm:main Oct 3, 2024
9 checks passed
@jurahul jurahul deleted the lit_basename_s branch October 3, 2024 19:29
%{fs-tmp-root} root component of file system paths pointing to the test's temporary directory
%{fs-sep} file system path separator
%t temporary file name unique to the test
%basename_s The last path component of %s
Copy link
Member

Choose a reason for hiding this comment

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

I see this is consistent with the already existing basename_t, but I feel that it would make a lot more sense if this was consistent with %{s:real} and %{/s:regex_replacement} as below, i.e. %{s:basename}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me look into changing this. I agree this makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created: #111062, will start review once checks pass.

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.

4 participants