Skip to content

Conversation

@boomanaiden154
Copy link
Contributor

Before this change, rm would assume that a symlink to a directory was
actually a directory and require the recursive flag to be passed,
differing from other shells. Given the change in lit is about the same
length as the test change would be (minus tests), I think it makes sense
to just support this in the internal shell.

@llvmbot
Copy link
Member

llvmbot commented Sep 14, 2025

@llvm/pr-subscribers-testing-tools

Author: Aiden Grossman (boomanaiden154)

Changes

Before this change, rm would assume that a symlink to a directory was
actually a directory and require the recursive flag to be passed,
differing from other shells. Given the change in lit is about the same
length as the test change would be (minus tests), I think it makes sense
to just support this in the internal shell.


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

4 Files Affected:

  • (modified) llvm/utils/lit/lit/TestRunner.py (+3-1)
  • (added) llvm/utils/lit/tests/Inputs/shtest-shell-symlinks/lit.cfg (+7)
  • (added) llvm/utils/lit/tests/Inputs/shtest-shell-symlinks/rm-symlink-dir.txt (+5)
  • (added) llvm/utils/lit/tests/shtest-shell-symlinks.py (+9)
diff --git a/llvm/utils/lit/lit/TestRunner.py b/llvm/utils/lit/lit/TestRunner.py
index 5daa8887e4c80..babc5361fa16b 100644
--- a/llvm/utils/lit/lit/TestRunner.py
+++ b/llvm/utils/lit/lit/TestRunner.py
@@ -516,7 +516,9 @@ def on_rm_error(func, path, exc_info):
         if force and not os.path.exists(path):
             continue
         try:
-            if os.path.isdir(path):
+            if os.path.islink(path):
+                os.remove(path)
+            elif os.path.isdir(path):
                 if not recursive:
                     stderr.write("Error: %s is a directory\n" % path)
                     exitCode = 1
diff --git a/llvm/utils/lit/tests/Inputs/shtest-shell-symlinks/lit.cfg b/llvm/utils/lit/tests/Inputs/shtest-shell-symlinks/lit.cfg
new file mode 100644
index 0000000000000..eab06f08b688c
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/shtest-shell-symlinks/lit.cfg
@@ -0,0 +1,7 @@
+import lit.formats
+
+config.name = "shtest-shell"
+config.suffixes = [".txt"]
+config.test_format = lit.formats.ShTest()
+config.test_source_root = None
+config.test_exec_root = None
diff --git a/llvm/utils/lit/tests/Inputs/shtest-shell-symlinks/rm-symlink-dir.txt b/llvm/utils/lit/tests/Inputs/shtest-shell-symlinks/rm-symlink-dir.txt
new file mode 100644
index 0000000000000..a44a07c13e4ae
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/shtest-shell-symlinks/rm-symlink-dir.txt
@@ -0,0 +1,5 @@
+# Check that we can delete a symlink to a folder without -r
+#
+# RUN: mkdir %t.dir
+# RUN: ln -s %t.dir %t.symlink
+# RUN: rm %t.symlink
diff --git a/llvm/utils/lit/tests/shtest-shell-symlinks.py b/llvm/utils/lit/tests/shtest-shell-symlinks.py
new file mode 100644
index 0000000000000..cfd72a01b6d94
--- /dev/null
+++ b/llvm/utils/lit/tests/shtest-shell-symlinks.py
@@ -0,0 +1,9 @@
+# Check that the internal shell builtins correctly handle cases involving
+# symlinks.
+
+# REQUIRES: symlinks
+# RUN: echo test
+# RUN: %{lit} -v %{inputs}/shtest-shell-symlinks | FileCheck %s
+
+# CHECK: -- Testing: 1 test{{.*}}
+# CHECK: PASS: shtest-shell :: rm-symlink-dir.txt

Created using spr 1.3.6

[skip ci]
Created using spr 1.3.6
Created using spr 1.3.6

[skip ci]
Created using spr 1.3.6
Created using spr 1.3.6

[skip ci]
Created using spr 1.3.6
boomanaiden154 added a commit to boomanaiden154/llvm-project that referenced this pull request Sep 19, 2025
Before this change, rm would assume that a symlink to a directory was
actually a directory and require the recursive flag to be passed,
differing from other shells. Given the change in lit is about the same
length as the test change would be (minus tests), I think it makes sense
to just support this in the internal shell.

Pull Request: llvm#158464
Created using spr 1.3.6

[skip ci]
Created using spr 1.3.6
Created using spr 1.3.6

[skip ci]
Created using spr 1.3.6
Created using spr 1.3.6

[skip ci]
Created using spr 1.3.6
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-add-support-for-deleting-symlinks-to-directories-without-r to main September 20, 2025 04:46
@boomanaiden154 boomanaiden154 merged commit 8326823 into main Sep 20, 2025
9 of 12 checks passed
@boomanaiden154 boomanaiden154 deleted the users/boomanaiden154/lit-add-support-for-deleting-symlinks-to-directories-without-r branch September 20, 2025 04:47
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request Sep 20, 2025
…hout -r

Before this change, rm would assume that a symlink to a directory was
actually a directory and require the recursive flag to be passed,
differing from other shells. Given the change in lit is about the same
length as the test change would be (minus tests), I think it makes sense
to just support this in the internal shell.

Reviewers: cmtice, petrhosek, ilovepi

Reviewed By: petrhosek, cmtice, ilovepi

Pull Request: llvm/llvm-project#158464
schwitanski pushed a commit to RWTH-HPC/llvm-lit-mirror that referenced this pull request Sep 21, 2025
Before this change, rm would assume that a symlink to a directory was
actually a directory and require the recursive flag to be passed,
differing from other shells. Given the change in lit is about the same
length as the test change would be (minus tests), I think it makes sense
to just support this in the internal shell.

Reviewers: cmtice, petrhosek, ilovepi

Reviewed By: petrhosek, cmtice, ilovepi

Pull Request: llvm/llvm-project#158464
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.

6 participants