[LIT] replace `lit.util.mkdir` with `pathlib.Path.mkdir` #163948

kikairoya · 2025-10-17T12:04:06Z

lit.util.mkdir and lit.util.mkdir_p were written during the Python 2.x era.
Since modern pathlib functions have similar functionality, we can simply use those instead.

Background:

On Cygwin, a file named file_name.exe can be accessed without the suffix, simply as file_name, as shown below:

$ echo > file_name.exe

$ file file_name.exe
file_name.exe: very short file (no magic)

$ file file_name
file_name: very short file (no magic)

In this situation, while running mkdir file_name works as intended, checking for the existence of the target before calling mkdir incorrectly reports that it already exists and thus skips the directory creation.

$ test -e file_name && echo exists
exists

$ mkdir file_name && echo ok
ok

$ file file_name
file_name: directory

Therefore, the existence pre-check should be skipped on Cygwin. Instead of add a workaround, refactored them.

llvmbot · 2025-10-17T12:04:42Z

@llvm/pr-subscribers-testing-tools

Author: Tomohiro Kashiwada (kikairoya)

Changes

On Cygwin, a file named file_name.exe can be accessed without the suffix, simply as file_name, as shown below:

$ echo &gt; file_name.exe

$ file file_name.exe
file_name.exe: very short file (no magic)

$ file file_name
file_name: very short file (no magic)

In this situation, while running mkdir file_name works as intended, checking for the existence of the target before calling mkdir incorrectly reports that it already exists and thus skips the directory creation.

$ test -e file &amp;&amp; echo exists
exists

$ mkdir file_name &amp;&amp; echo ok
ok

$ file file_name
file: directory

Therefore, the existence pre-check should be skipped on Cygwin. If the target actually already exists, such an error will be ignored anyway.

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

1 Files Affected:

(modified) llvm/utils/lit/lit/util.py (+1-1)

diff --git a/llvm/utils/lit/lit/util.py b/llvm/utils/lit/lit/util.py
index ce4c3c2df3436..a5181ab20a7e1 100644
--- a/llvm/utils/lit/lit/util.py
+++ b/llvm/utils/lit/lit/util.py
@@ -164,7 +164,7 @@ def mkdir(path):
 def mkdir_p(path):
     """mkdir_p(path) - Make the "path" directory, if it does not exist; this
     will also make directories for any missing parent directories."""
-    if not path or os.path.exists(path):
+    if not path or (sys.platform != "cygwin" and os.path.exists(path)):
         return
 
     parent = os.path.dirname(path)

kikairoya · 2025-10-17T12:06:00Z

cc: @jeremyd2019 @mstorsjo

arichardson · 2025-10-17T14:24:15Z

So does that mean any use of os.path.exists() is broken on cygwin? Maybe we could change it to is_dir?

arichardson

Or maybe now that we depend on newer python we can just use the exist_ok=True parameter for mkdir?

kikairoya · 2025-10-17T14:38:01Z

So does that mean any use of os.path.exists() is broken on cygwin? Maybe we could change it to is_dir?

I think, in general, most of such checks should be avoided (cf. TOCTOU).

Or maybe now that we depend on newer python we can just use the exist_ok=True parameter for mkdir?

Sure. I'll make a change to use it.

On Cygwin, a file named `file_name.exe` can be accessed without the suffix, simply as `file_name`, as shown below: ``` $ echo > file_name.exe $ file file_name.exe file_name.exe: very short file (no magic) $ file file_name file_name: very short file (no magic) ``` In this situation, while running `mkdir file_name` works as intended, checking for the existence of the target before calling `mkdir` incorrectly reports that it already exists and thus skips the directory creation. ``` $ test -e file && echo exists exists $ mkdir file_name && echo ok ok $ file file_name file: directory ``` Therefore, the existence pre-check should be skipped on Cygwin. If the target actually already exists, such an error will be ignored anyway.

kikairoya · 2025-10-17T16:39:30Z

mkdir_p now simply forwards to os.makedirs.
Should it be inlined? It is only called from two locations in TestRunner.py.

mkdir can also be inlined as it is now only called from one location but I'm not sure if the call to CreateDirectoryW can be replaced with just an os.mkdir.

arichardson · 2025-10-17T17:06:51Z

llvm/utils/lit/lit/util.py

-        mkdir_p(parent)
-
-    mkdir(path)
+    os.makedirs(path, exist_ok=True)


This looks much simpler, nice. Not a Windows expert but do we need to remap long paths using something like this?

Suggested change

os.makedirs(path, exist_ok=True)

if platform.system() == "Windows":

if not path.startswith(r"\\?\"):

path = r"\\?\" + path

os.makedirs(path, exist_ok=True)

Not sure - do our python script do such path mangling anywhere else?

They do in the def mkdir(path): above.

It sounds like python3.6 supports long paths if they are enabled in the registry so there should be no need to work around it here anymore:
https://bugs.python.org/issue27731 -> https://hg.python.org/cpython/rev/26601191b368

arichardson · 2025-10-17T17:09:30Z

From what I've read pathlib.Path handles long windows paths correctly, so maybe the best solution would be to migrate callers of these helpers towards pathlib?

kikairoya · 2025-10-17T22:59:30Z

OK, I'll try to replace both of mkdir and mkdir_p with direct calls to the pathlib functions.

kikairoya · 2025-10-18T09:38:19Z

llvm/utils/lit/lit/util.py

-            os.mkdir(path)
-    except OSError:
-        e = sys.exc_info()[1]
-        # ignore EEXIST, which may occur during a race condition


Previously, all EEXIST errors were ignored.
We should indeed ignore races between concurrent mkdir targets, but I don't see any reason to allow races between touch target and mkdir target.
Since pathlib's exist_ok=True behaves this way, the test shtest-glob.py has been updated to reflect the new behavior.

RoboTux

LGTM otherwise

RoboTux · 2025-10-20T09:39:07Z

llvm/utils/lit/tests/Inputs/shtest-glob/glob-mkdir.txt

+# RUN: mkdir %S/example_dir1.new
+# RUN: mkdir %S/example_dir2.new
+
+## This mkdir should succeed (so RUN should fail) because the `example_dir*.new`s are directories already exist.


Nit: remove `are'

llvmbot added llvm-lit testing-tools labels Oct 17, 2025

arichardson requested changes Oct 17, 2025

View reviewed changes

kikairoya added 2 commits October 18, 2025 01:35

use makedirs with exist_ok=True

665e97f

kikairoya force-pushed the cygwin-lit-mkdir-p branch from b36911f to 665e97f Compare October 17, 2025 16:35

arichardson reviewed Oct 17, 2025

View reviewed changes

use pathlib.Path.mkdir directly

c9fc27d

kikairoya commented Oct 18, 2025

View reviewed changes

kikairoya changed the title ~~[LIT][Cygwin] Skip pre-check for existence in mkdir-p~~ [LIT] replace lit.util.mkdir with pathlib.Path.mkdir Oct 18, 2025

fix test

10db692

RoboTux reviewed Oct 20, 2025

View reviewed changes

-    os.makedirs(path, exist_ok=True)
+    if platform.system() == "Windows":
+    if not path.startswith(r"\\?\"):
+        path = r"\\?\" + path
+    os.makedirs(path, exist_ok=True)

[LIT] replace lit.util.mkdir with pathlib.Path.mkdir #163948

Are you sure you want to change the base?

[LIT] replace lit.util.mkdir with pathlib.Path.mkdir #163948

Conversation

kikairoya commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Oct 17, 2025

Uh oh!

kikairoya commented Oct 17, 2025

Uh oh!

arichardson commented Oct 17, 2025

Uh oh!

arichardson left a comment

Choose a reason for hiding this comment

Uh oh!

kikairoya commented Oct 17, 2025

Uh oh!

kikairoya commented Oct 17, 2025

Uh oh!

arichardson Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

mstorsjo Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

arichardson Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

mstorsjo Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

arichardson commented Oct 17, 2025

Uh oh!

kikairoya commented Oct 17, 2025

Uh oh!

kikairoya Oct 18, 2025

Choose a reason for hiding this comment

Uh oh!

RoboTux left a comment

Choose a reason for hiding this comment

Uh oh!

RoboTux Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

[LIT] replace `lit.util.mkdir` with `pathlib.Path.mkdir` #163948

[LIT] replace `lit.util.mkdir` with `pathlib.Path.mkdir` #163948

kikairoya commented Oct 17, 2025 •

edited

Loading