[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.

If you encounter a path length issue after this change, the registry value LongPathsEnabled must be set as described in https://learn.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation . Note that the Python runtime is already marked as a longPathAware executable.

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'

Thank you for the review.
I meant to write, "the example_dir*.news are directories" rather than "the example_dir*.news already exist".
Would "the example_dir*.news that already exist are directories." be correct?

Yes that would be fine, thanks.

kikairoya · 2025-10-22T11:10:46Z

I've verified that latest version resolves my initial problem about *.exe.
The title and the description are updated to reflect changes.

RoboTux

LGTM thanks for the work

kikairoya · 2025-10-31T11:16:17Z

@arichardson Could you take another look?

arichardson

One comment on the test otherwise LGTM.

llvm/utils/lit/lit/TestRunner.py

arichardson · 2025-10-31T16:53:46Z

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

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


Suggested change

# RUN: not mkdir %S/example_file*.input

# RUN: mkdir %S/example_dir1.new

# RUN: mkdir %S/example_dir2.new

## This mkdir should succeed (so RUN should fail) because the `example_dir*.news` that already exist are directories.

# RUN: not mkdir %S/example_dir*.new

# RUN: rm -rf %t && mkdir %t

# RUN: cp %S/example_file*.input %t

# RUN: not mkdir %t/example_file*.input

# RUN: mkdir %t/example_dir1.new

# RUN: mkdir %t/example_dir2.new

## This mkdir should succeed (so RUN should fail) because the `example_dir*.news` that already exist are directories.

# RUN: not mkdir %t/example_dir*.new

I think we need to do something like the following since we shouldn't be writing to the source directory when running tests. Might be good to have a builder that bind mounts the source dir as read-only.

Thank you for the suggestion.

In these particular cases, Inputs are copied to the build directory and run within there, so they won't try to write to the source tree.

llvm-project/llvm/utils/lit/CMakeLists.txt

Lines 15 to 20 in 523706f

add_custom_target(prepare-check-lit

COMMAND ${CMAKE_COMMAND} -E remove_directory "${CMAKE_CURRENT_BINARY_DIR}/tests"

COMMAND ${CMAKE_COMMAND} -E copy_directory "${CMAKE_CURRENT_SOURCE_DIR}/tests" "${CMAKE_CURRENT_BINARY_DIR}/tests"

COMMAND ${CMAKE_COMMAND} -E copy "${CMAKE_CURRENT_BINARY_DIR}/lit.site.cfg" "${CMAKE_CURRENT_BINARY_DIR}/tests"

COMMENT "Preparing lit tests"

)

That said, I agree that it's not good that they look like writing to the source directory.
I'd like to avoid use of another command in this test, so I've updated to include example_dirs in the source tree.

Ah I was not aware that %S is actually in the build tree. LGTM

mstorsjo · 2025-11-03T12:10:29Z

So if everyone are ok with this, I guess we can merge it?

kikairoya · 2025-11-04T09:30:56Z

Yes, please -- thank you as always!

kikairoya · 2025-11-04T10:17:10Z

https://lab.llvm.org/buildbot/#/builders/123/builds/30061
https://lab.llvm.org/buildbot/#/builders/210/builds/4853/steps/12/logs/stdio

Some bots fail in litsupport/test.py due to lit.util.mkdir_p missing, but it seems an external code.
What should I do for this? Restoring back lit.util.mkdir_p and lit.util.mkdir with delegating to pathlib is enough?
CI got back to green.

The helper function `lit.util.mkdir_p` has been removed in llvm/llvm-project#163948 . Instead of that, call `pathlib` functions directly.

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

Update glob-mkdir.txt

a37027e

kikairoya requested a review from arichardson October 22, 2025 11:11

RoboTux approved these changes Oct 22, 2025

View reviewed changes

arichardson approved these changes Oct 31, 2025

View reviewed changes

include example_dirs into source tree

cdf8adc

mstorsjo merged commit 84cc2b0 into llvm:main Nov 4, 2025
10 checks passed

kikairoya added a commit to kikairoya/llvm-test-suite that referenced this pull request Nov 4, 2025

test.py: remove use of lit.util.mkdir_p

af191d7

The helper function `lit.util.mkdir_p` has been removed in llvm/llvm-project#163948 . Instead of that, call `pathlib` functions directly.

kikairoya mentioned this pull request Nov 4, 2025

test.py: remove use of lit.util.mkdir_p llvm/llvm-test-suite#292

Merged

kikairoya deleted the cygwin-lit-mkdir-p branch November 4, 2025 12:17

jplehr pushed a commit to llvm/llvm-test-suite that referenced this pull request Nov 4, 2025

test.py: remove use of lit.util.mkdir_p (#292)

bb453ac

The helper function `lit.util.mkdir_p` has been removed in llvm/llvm-project#163948 . Instead of that, call `pathlib` functions directly.

kwk mentioned this pull request Nov 5, 2025

Snapshot for 20251105, v22.0.0, 15b19c7 (big-merge) fedora-llvm-team/llvm-snapshots#1757

Closed

kerbowa mentioned this pull request Nov 10, 2025

[AMDGPU] Verify dominance when rewriting spills to registers #167347

Open

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

	add_custom_target(prepare-check-lit
	COMMAND ${CMAKE_COMMAND} -E remove_directory "${CMAKE_CURRENT_BINARY_DIR}/tests"
	COMMAND ${CMAKE_COMMAND} -E copy_directory "${CMAKE_CURRENT_SOURCE_DIR}/tests" "${CMAKE_CURRENT_BINARY_DIR}/tests"
	COMMAND ${CMAKE_COMMAND} -E copy "${CMAKE_CURRENT_BINARY_DIR}/lit.site.cfg" "${CMAKE_CURRENT_BINARY_DIR}/tests"
	COMMENT "Preparing lit tests"
	)

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

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

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arichardson commented Oct 17, 2025

Uh oh!

kikairoya commented Oct 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RoboTux left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kikairoya commented Oct 22, 2025

Uh oh!

RoboTux left a comment

Choose a reason for hiding this comment

Uh oh!

kikairoya commented Oct 31, 2025

Uh oh!

arichardson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mstorsjo commented Nov 3, 2025

Uh oh!

kikairoya commented Nov 4, 2025

Uh oh!

Uh oh!

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

Uh oh!

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

kikairoya commented Nov 4, 2025 •

edited

Loading