Reapply "[lit] Implement builtin umask (#94621)" #155850
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This reverts commit faa4e35. This was originally reverted because it was using a Python 3.9 feature (umask in subprocess.Popen) when LLVM only requires Python 3.8. This patch uses os.umask instead, which has been around for longer.
|
@llvm/pr-subscribers-debuginfo @llvm/pr-subscribers-llvm-binary-utilities Author: Aiden Grossman (boomanaiden154) ChangesThis reverts commit faa4e35. This was originally reverted because it was using a Python 3.9 feature (umask in subprocess.Popen) when LLVM only requires Python 3.8. This patch uses os.umask instead, which has been around for longer. Full diff: https://github.com/llvm/llvm-project/pull/155850.diff 9 Files Affected:
diff --git a/llvm/test/tools/llvm-dwarfutil/ELF/X86/mirror-permissions-unix.test b/llvm/test/tools/llvm-dwarfutil/ELF/X86/mirror-permissions-unix.test
index a95d1c0aafa21..fdcba4dcd666b 100644
--- a/llvm/test/tools/llvm-dwarfutil/ELF/X86/mirror-permissions-unix.test
+++ b/llvm/test/tools/llvm-dwarfutil/ELF/X86/mirror-permissions-unix.test
@@ -3,7 +3,6 @@
## Setting the umask to 0 ensures deterministic permissions across
## test environments.
# UNSUPPORTED: system-windows
-# REQUIRES: shell
# RUN: touch %t
# RUN: chmod 0777 %t
diff --git a/llvm/test/tools/llvm-objcopy/ELF/mirror-permissions-unix.test b/llvm/test/tools/llvm-objcopy/ELF/mirror-permissions-unix.test
index 8f4993f4f3d29..66a481a2230d1 100644
--- a/llvm/test/tools/llvm-objcopy/ELF/mirror-permissions-unix.test
+++ b/llvm/test/tools/llvm-objcopy/ELF/mirror-permissions-unix.test
@@ -6,7 +6,6 @@
## Setting the umask to 0 ensures deterministic permissions across
## test environments.
# UNSUPPORTED: system-windows
-# REQUIRES: shell
# RUN: touch %t
# RUN: chmod 0777 %t
diff --git a/llvm/test/tools/llvm-objcopy/ELF/respect-umask.test b/llvm/test/tools/llvm-objcopy/ELF/respect-umask.test
index 376e33a217819..02e9b93f5376f 100644
--- a/llvm/test/tools/llvm-objcopy/ELF/respect-umask.test
+++ b/llvm/test/tools/llvm-objcopy/ELF/respect-umask.test
@@ -1,10 +1,8 @@
## This tests that the umask is respected when
## assigning permissions of output files.
-## Windows has no umask so this test makes no sense, nor would
-## it work because there is no umask(1) in a Windows environment
+## Windows has no umask so this test makes no sense.
# UNSUPPORTED: system-windows
-# REQUIRES: shell
# RUN: rm -f %t
# RUN: touch %t
diff --git a/llvm/utils/lit/lit/TestRunner.py b/llvm/utils/lit/lit/TestRunner.py
index 31e3ed679d431..36c19c1c86c75 100644
--- a/llvm/utils/lit/lit/TestRunner.py
+++ b/llvm/utils/lit/lit/TestRunner.py
@@ -98,9 +98,10 @@ class ShellEnvironment(object):
we maintain a dir stack for pushd/popd.
"""
- def __init__(self, cwd, env):
+ def __init__(self, cwd, env, umask=-1):
self.cwd = cwd
self.env = dict(env)
+ self.umask = umask
self.dirStack = []
def change_dir(self, newdir):
@@ -582,6 +583,20 @@ class SHFILEOPSTRUCTW(Structure):
return ShellCommandResult(cmd, "", stderr.getvalue(), exitCode, False)
+def executeBuiltinUmask(cmd, shenv):
+ """executeBuiltinUmask - Change the current umask."""
+ if os.name != "posix":
+ raise InternalShellError(cmd, "'umask' not supported on this system")
+ if len(cmd.args) != 2:
+ raise InternalShellError(cmd, "'umask' supports only one argument")
+ try:
+ # Update the umask in the parent environment.
+ shenv.umask = int(cmd.args[1], 8)
+ except ValueError as err:
+ raise InternalShellError(cmd, "Error: 'umask': %s" % str(err))
+ return ShellCommandResult(cmd, "", "", 0, False)
+
+
def executeBuiltinColon(cmd, cmd_shenv):
"""executeBuiltinColon - Discard arguments and exit with status 0."""
return ShellCommandResult(cmd, "", "", 0, False)
@@ -736,6 +751,7 @@ def _executeShCmd(cmd, shenv, results, timeoutHelper):
"popd": executeBuiltinPopd,
"pushd": executeBuiltinPushd,
"rm": executeBuiltinRm,
+ "umask": executeBuiltinUmask,
":": executeBuiltinColon,
}
# To avoid deadlock, we use a single stderr stream for piped
@@ -757,7 +773,7 @@ def _executeShCmd(cmd, shenv, results, timeoutHelper):
# env FOO=1 llc < %s | env BAR=2 llvm-mc | FileCheck %s
# env FOO=1 %{another_env_plus_cmd} | FileCheck %s
if cmd_shenv is shenv:
- cmd_shenv = ShellEnvironment(shenv.cwd, shenv.env)
+ cmd_shenv = ShellEnvironment(shenv.cwd, shenv.env, shenv.umask)
args = updateEnv(cmd_shenv, args)
if not args:
# Return the environment variables if no argument is provided.
@@ -902,6 +918,13 @@ def _executeShCmd(cmd, shenv, results, timeoutHelper):
args = quote_windows_command(args)
try:
+ # TODO(boomanaiden154): We currently wrap the subprocess.Popen with
+ # os.umask as the umask argument in subprocess.Popen is not
+ # available before Python 3.9. Once LLVM requires at least Python
+ # 3.9, this code should be updated to use umask argument.
+ old_umask = -1
+ if cmd_shenv.umask != -1:
+ old_umask = os.umask(cmd_shenv.umask)
procs.append(
subprocess.Popen(
args,
@@ -916,6 +939,8 @@ def _executeShCmd(cmd, shenv, results, timeoutHelper):
errors="replace",
)
)
+ if old_umask != -1:
+ os.umask(old_umask)
proc_not_counts.append(not_count)
# Let the helper know about this process
timeoutHelper.addProcess(procs[-1])
diff --git a/llvm/utils/lit/tests/Inputs/shtest-umask/lit.cfg b/llvm/utils/lit/tests/Inputs/shtest-umask/lit.cfg
new file mode 100644
index 0000000000000..52e18b1d5c2b3
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/shtest-umask/lit.cfg
@@ -0,0 +1,7 @@
+import lit.formats
+
+config.name = "shtest-umask"
+config.suffixes = [".txt"]
+config.test_format = lit.formats.ShTest(execute_external=False)
+if sys.platform.startswith("win") or sys.platform.startswith("cygwin"):
+ config.available_features.add("system-windows")
diff --git a/llvm/utils/lit/tests/Inputs/shtest-umask/umask-bad-arg.txt b/llvm/utils/lit/tests/Inputs/shtest-umask/umask-bad-arg.txt
new file mode 100644
index 0000000000000..639bfd4b7b4f3
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/shtest-umask/umask-bad-arg.txt
@@ -0,0 +1 @@
+# RUN: umask bad
diff --git a/llvm/utils/lit/tests/Inputs/shtest-umask/umask-ok.txt b/llvm/utils/lit/tests/Inputs/shtest-umask/umask-ok.txt
new file mode 100644
index 0000000000000..9d43efbddf2cf
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/shtest-umask/umask-ok.txt
@@ -0,0 +1,11 @@
+## Windows has no umask so this test makes no sense.
+# UNSUPPORTED: system-windows
+
+# RUN: touch %t
+# RUN: chmod 644 %t && ls -l %t | cut -f 1 -d ' ' > %t.644
+# RUN: chmod 600 %t && ls -l %t | cut -f 1 -d ' ' > %t.600
+# RUN: chmod 666 %t && ls -l %t | cut -f 1 -d ' ' > %t.666
+
+# RUN: umask 022 && rm %t && touch %t && ls -l %t | cut -f 1 -d ' ' | cmp - %t.644
+# RUN: umask 177 && rm %t && touch %t && ls -l %t | cut -f 1 -d ' ' | cmp - %t.600
+# RUN: umask 000 && rm %t && touch %t && ls -l %t | cut -f 1 -d ' ' | cmp - %t.666
diff --git a/llvm/utils/lit/tests/Inputs/shtest-umask/umask-too-many-args.txt b/llvm/utils/lit/tests/Inputs/shtest-umask/umask-too-many-args.txt
new file mode 100644
index 0000000000000..0b7461b5bc3fe
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/shtest-umask/umask-too-many-args.txt
@@ -0,0 +1 @@
+# RUN: umask 0 0
diff --git a/llvm/utils/lit/tests/shtest-umask.py b/llvm/utils/lit/tests/shtest-umask.py
new file mode 100644
index 0000000000000..daec3cf479fc7
--- /dev/null
+++ b/llvm/utils/lit/tests/shtest-umask.py
@@ -0,0 +1,18 @@
+# Check the umask command
+
+# RUN: not %{lit} -a -v %{inputs}/shtest-umask | FileCheck -match-full-lines %s
+
+# CHECK: -- Testing: 3 tests{{.*}}
+
+# CHECK-LABEL: FAIL: shtest-umask :: umask-bad-arg.txt ({{[^)]*}})
+# CHECK: umask bad
+# CHECK: # | Error: 'umask': invalid literal {{.*}}
+
+# CHECK-LABEL: FAIL: shtest-umask :: umask-too-many-args.txt ({{[^)]*}})
+# CHECK: umask 0 0
+# CHECK: # | 'umask' supports only one argument
+
+# CHECK: Total Discovered Tests: 3
+# CHECK: {{Passed|Unsupported}}: 1 {{\([0-9]*\.[0-9]*%\)}}
+# CHECK: Failed{{ *}}: 2 {{\([0-9]*\.[0-9]*%\)}}
+# CHECK-NOT: {{.}}
|
|
@llvm/pr-subscribers-testing-tools Author: Aiden Grossman (boomanaiden154) ChangesThis reverts commit faa4e35. This was originally reverted because it was using a Python 3.9 feature (umask in subprocess.Popen) when LLVM only requires Python 3.8. This patch uses os.umask instead, which has been around for longer. Full diff: https://github.com/llvm/llvm-project/pull/155850.diff 9 Files Affected:
diff --git a/llvm/test/tools/llvm-dwarfutil/ELF/X86/mirror-permissions-unix.test b/llvm/test/tools/llvm-dwarfutil/ELF/X86/mirror-permissions-unix.test
index a95d1c0aafa21..fdcba4dcd666b 100644
--- a/llvm/test/tools/llvm-dwarfutil/ELF/X86/mirror-permissions-unix.test
+++ b/llvm/test/tools/llvm-dwarfutil/ELF/X86/mirror-permissions-unix.test
@@ -3,7 +3,6 @@
## Setting the umask to 0 ensures deterministic permissions across
## test environments.
# UNSUPPORTED: system-windows
-# REQUIRES: shell
# RUN: touch %t
# RUN: chmod 0777 %t
diff --git a/llvm/test/tools/llvm-objcopy/ELF/mirror-permissions-unix.test b/llvm/test/tools/llvm-objcopy/ELF/mirror-permissions-unix.test
index 8f4993f4f3d29..66a481a2230d1 100644
--- a/llvm/test/tools/llvm-objcopy/ELF/mirror-permissions-unix.test
+++ b/llvm/test/tools/llvm-objcopy/ELF/mirror-permissions-unix.test
@@ -6,7 +6,6 @@
## Setting the umask to 0 ensures deterministic permissions across
## test environments.
# UNSUPPORTED: system-windows
-# REQUIRES: shell
# RUN: touch %t
# RUN: chmod 0777 %t
diff --git a/llvm/test/tools/llvm-objcopy/ELF/respect-umask.test b/llvm/test/tools/llvm-objcopy/ELF/respect-umask.test
index 376e33a217819..02e9b93f5376f 100644
--- a/llvm/test/tools/llvm-objcopy/ELF/respect-umask.test
+++ b/llvm/test/tools/llvm-objcopy/ELF/respect-umask.test
@@ -1,10 +1,8 @@
## This tests that the umask is respected when
## assigning permissions of output files.
-## Windows has no umask so this test makes no sense, nor would
-## it work because there is no umask(1) in a Windows environment
+## Windows has no umask so this test makes no sense.
# UNSUPPORTED: system-windows
-# REQUIRES: shell
# RUN: rm -f %t
# RUN: touch %t
diff --git a/llvm/utils/lit/lit/TestRunner.py b/llvm/utils/lit/lit/TestRunner.py
index 31e3ed679d431..36c19c1c86c75 100644
--- a/llvm/utils/lit/lit/TestRunner.py
+++ b/llvm/utils/lit/lit/TestRunner.py
@@ -98,9 +98,10 @@ class ShellEnvironment(object):
we maintain a dir stack for pushd/popd.
"""
- def __init__(self, cwd, env):
+ def __init__(self, cwd, env, umask=-1):
self.cwd = cwd
self.env = dict(env)
+ self.umask = umask
self.dirStack = []
def change_dir(self, newdir):
@@ -582,6 +583,20 @@ class SHFILEOPSTRUCTW(Structure):
return ShellCommandResult(cmd, "", stderr.getvalue(), exitCode, False)
+def executeBuiltinUmask(cmd, shenv):
+ """executeBuiltinUmask - Change the current umask."""
+ if os.name != "posix":
+ raise InternalShellError(cmd, "'umask' not supported on this system")
+ if len(cmd.args) != 2:
+ raise InternalShellError(cmd, "'umask' supports only one argument")
+ try:
+ # Update the umask in the parent environment.
+ shenv.umask = int(cmd.args[1], 8)
+ except ValueError as err:
+ raise InternalShellError(cmd, "Error: 'umask': %s" % str(err))
+ return ShellCommandResult(cmd, "", "", 0, False)
+
+
def executeBuiltinColon(cmd, cmd_shenv):
"""executeBuiltinColon - Discard arguments and exit with status 0."""
return ShellCommandResult(cmd, "", "", 0, False)
@@ -736,6 +751,7 @@ def _executeShCmd(cmd, shenv, results, timeoutHelper):
"popd": executeBuiltinPopd,
"pushd": executeBuiltinPushd,
"rm": executeBuiltinRm,
+ "umask": executeBuiltinUmask,
":": executeBuiltinColon,
}
# To avoid deadlock, we use a single stderr stream for piped
@@ -757,7 +773,7 @@ def _executeShCmd(cmd, shenv, results, timeoutHelper):
# env FOO=1 llc < %s | env BAR=2 llvm-mc | FileCheck %s
# env FOO=1 %{another_env_plus_cmd} | FileCheck %s
if cmd_shenv is shenv:
- cmd_shenv = ShellEnvironment(shenv.cwd, shenv.env)
+ cmd_shenv = ShellEnvironment(shenv.cwd, shenv.env, shenv.umask)
args = updateEnv(cmd_shenv, args)
if not args:
# Return the environment variables if no argument is provided.
@@ -902,6 +918,13 @@ def _executeShCmd(cmd, shenv, results, timeoutHelper):
args = quote_windows_command(args)
try:
+ # TODO(boomanaiden154): We currently wrap the subprocess.Popen with
+ # os.umask as the umask argument in subprocess.Popen is not
+ # available before Python 3.9. Once LLVM requires at least Python
+ # 3.9, this code should be updated to use umask argument.
+ old_umask = -1
+ if cmd_shenv.umask != -1:
+ old_umask = os.umask(cmd_shenv.umask)
procs.append(
subprocess.Popen(
args,
@@ -916,6 +939,8 @@ def _executeShCmd(cmd, shenv, results, timeoutHelper):
errors="replace",
)
)
+ if old_umask != -1:
+ os.umask(old_umask)
proc_not_counts.append(not_count)
# Let the helper know about this process
timeoutHelper.addProcess(procs[-1])
diff --git a/llvm/utils/lit/tests/Inputs/shtest-umask/lit.cfg b/llvm/utils/lit/tests/Inputs/shtest-umask/lit.cfg
new file mode 100644
index 0000000000000..52e18b1d5c2b3
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/shtest-umask/lit.cfg
@@ -0,0 +1,7 @@
+import lit.formats
+
+config.name = "shtest-umask"
+config.suffixes = [".txt"]
+config.test_format = lit.formats.ShTest(execute_external=False)
+if sys.platform.startswith("win") or sys.platform.startswith("cygwin"):
+ config.available_features.add("system-windows")
diff --git a/llvm/utils/lit/tests/Inputs/shtest-umask/umask-bad-arg.txt b/llvm/utils/lit/tests/Inputs/shtest-umask/umask-bad-arg.txt
new file mode 100644
index 0000000000000..639bfd4b7b4f3
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/shtest-umask/umask-bad-arg.txt
@@ -0,0 +1 @@
+# RUN: umask bad
diff --git a/llvm/utils/lit/tests/Inputs/shtest-umask/umask-ok.txt b/llvm/utils/lit/tests/Inputs/shtest-umask/umask-ok.txt
new file mode 100644
index 0000000000000..9d43efbddf2cf
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/shtest-umask/umask-ok.txt
@@ -0,0 +1,11 @@
+## Windows has no umask so this test makes no sense.
+# UNSUPPORTED: system-windows
+
+# RUN: touch %t
+# RUN: chmod 644 %t && ls -l %t | cut -f 1 -d ' ' > %t.644
+# RUN: chmod 600 %t && ls -l %t | cut -f 1 -d ' ' > %t.600
+# RUN: chmod 666 %t && ls -l %t | cut -f 1 -d ' ' > %t.666
+
+# RUN: umask 022 && rm %t && touch %t && ls -l %t | cut -f 1 -d ' ' | cmp - %t.644
+# RUN: umask 177 && rm %t && touch %t && ls -l %t | cut -f 1 -d ' ' | cmp - %t.600
+# RUN: umask 000 && rm %t && touch %t && ls -l %t | cut -f 1 -d ' ' | cmp - %t.666
diff --git a/llvm/utils/lit/tests/Inputs/shtest-umask/umask-too-many-args.txt b/llvm/utils/lit/tests/Inputs/shtest-umask/umask-too-many-args.txt
new file mode 100644
index 0000000000000..0b7461b5bc3fe
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/shtest-umask/umask-too-many-args.txt
@@ -0,0 +1 @@
+# RUN: umask 0 0
diff --git a/llvm/utils/lit/tests/shtest-umask.py b/llvm/utils/lit/tests/shtest-umask.py
new file mode 100644
index 0000000000000..daec3cf479fc7
--- /dev/null
+++ b/llvm/utils/lit/tests/shtest-umask.py
@@ -0,0 +1,18 @@
+# Check the umask command
+
+# RUN: not %{lit} -a -v %{inputs}/shtest-umask | FileCheck -match-full-lines %s
+
+# CHECK: -- Testing: 3 tests{{.*}}
+
+# CHECK-LABEL: FAIL: shtest-umask :: umask-bad-arg.txt ({{[^)]*}})
+# CHECK: umask bad
+# CHECK: # | Error: 'umask': invalid literal {{.*}}
+
+# CHECK-LABEL: FAIL: shtest-umask :: umask-too-many-args.txt ({{[^)]*}})
+# CHECK: umask 0 0
+# CHECK: # | 'umask' supports only one argument
+
+# CHECK: Total Discovered Tests: 3
+# CHECK: {{Passed|Unsupported}}: 1 {{\([0-9]*\.[0-9]*%\)}}
+# CHECK: Failed{{ *}}: 2 {{\([0-9]*\.[0-9]*%\)}}
+# CHECK-NOT: {{.}}
|
|
Thanks! LGTM but I will defer to anyone who didn't write the original patch. |
petrhosek
approved these changes
Aug 29, 2025
ilovepi
approved these changes
Aug 29, 2025
boomanaiden154
added a commit
to boomanaiden154/llvm-project
that referenced
this pull request
Aug 29, 2025
These two tests were unresolved when using lit's internal shell. In the case of tail-duplication-constant-prop, it was because they were using a echo $? line, and lit's internal echo implementation does not support $? to get the return code. The test was never actually asserting anything about the return code though, so I've removed the echo commands. In the case of permission.test, it was because umask was not supported before llvm#155850, and afterwards not without an argument. The test also was not great at capturing what it was supposed to (leaving open possibilites like the system umask and what Bolt was using happening to match), so I've rewritten the test in the style of llvm/test/tools/llvm-objcopy/ELF/respect-umask.test. This fixes llvm#102693.
boomanaiden154
added a commit
to boomanaiden154/llvm-project
that referenced
this pull request
Aug 29, 2025
These two tests were unresolved when using lit's internal shell. In the case of tail-duplication-constant-prop, it was because they were using a echo $? line, and lit's internal echo implementation does not support $? to get the return code. The test was never actually asserting anything about the return code though, so I've removed the echo commands. In the case of permission.test, it was because umask was not supported before llvm#155850, and afterwards not without an argument. The test also was not great at capturing what it was supposed to (leaving open possibilites like the system umask and what Bolt was using happening to match), so I've rewritten the test in the style of llvm/test/tools/llvm-objcopy/ELF/respect-umask.test. This fixes llvm#102693. Pull Request: llvm#156082
boomanaiden154
added a commit
that referenced
this pull request
Aug 29, 2025
These two tests were unresolved when using lit's internal shell. In the case of tail-duplication-constant-prop, it was because they were using a echo $? line, and lit's internal echo implementation does not support $? to get the return code. The test was never actually asserting anything about the return code though, so I've removed the echo commands. In the case of permission.test, it was because umask was not supported before #155850, and afterwards not without an argument. The test also was not great at capturing what it was supposed to (leaving open possibilites like the system umask and what Bolt was using happening to match), so I've rewritten the test in the style of llvm/test/tools/llvm-objcopy/ELF/respect-umask.test. This fixes #102693. Reviewers: maksfb, yota9, ayermolo, yozhu, aaupov, rafaelauler, paschalis-mpeis Reviewed By: maksfb Pull Request: #156082
|
This change breaks on Windows pipelines because the error output is: It seems like the top-level |
|
This change has been causing Windows builders to be red for the last 6 hours or so. @boomanaiden154 if you do not have a fix in progress, can we either mark the test as unsupported on Windows as suggested by @tex3d, or revert the change while you investigate/fix? |
|
If there's no other breakage, then lets get it marked unsupported for now, and @boomanaiden154 can follow up after. |
|
We ideally want to assert that we get the expected behavior on Windows. Disabled for now to (hopefully) get things unblocked. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This reverts commit faa4e35.
This was originally reverted because it was using a Python 3.9 feature (umask in subprocess.Popen) when LLVM only requires Python 3.8. This patch uses os.umask instead, which has been around for longer.