Skip to content

[lldb] Don't use NamedTemporaryFile to test compiler support #151387

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 31, 2025

Conversation

JDevlieghere
Copy link
Member

You cannot use a NamedTempFile with an external process because it may not be flushed to disk. The safest and most portable approach is to close the file, call the other process and then unlink the file manually.

Presumably this works fine on Linux, but it fails on Darwin when targeting remote-linux.

See https://bugs.python.org/issue29573

@llvmbot
Copy link
Member

llvmbot commented Jul 30, 2025

@llvm/pr-subscribers-lldb

Author: Jonas Devlieghere (JDevlieghere)

Changes

You cannot use a NamedTempFile with an external process because it may not be flushed to disk. The safest and most portable approach is to close the file, call the other process and then unlink the file manually.

Presumably this works fine on Linux, but it fails on Darwin when targeting remote-linux.

See https://bugs.python.org/issue29573


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

1 Files Affected:

  • (modified) lldb/packages/Python/lldbsuite/test/dotest.py (+19-4)
diff --git a/lldb/packages/Python/lldbsuite/test/dotest.py b/lldb/packages/Python/lldbsuite/test/dotest.py
index 24236e779e51d..20eb3154b5775 100644
--- a/lldb/packages/Python/lldbsuite/test/dotest.py
+++ b/lldb/packages/Python/lldbsuite/test/dotest.py
@@ -45,6 +45,21 @@
 from ..support import seven
 
 
+class OnDiskTempFile:
+    def __init__(self):
+        self.path = None
+
+    def __enter__(self):
+        fd, path = tempfile.mkstemp()
+        os.close(fd)
+        self.path = path
+        return self
+
+    def __exit__(self, exc_type, exc_val, exc_tb):
+        if os.path.exists(self.path):
+            os.remove(self.path)
+
+
 def is_exe(fpath):
     """Returns true if fpath is an executable."""
     if fpath is None:
@@ -780,8 +795,8 @@ def canRunLibcxxTests():
         return True, "libc++ always present"
 
     if platform == "linux":
-        with tempfile.NamedTemporaryFile() as f:
-            cmd = [configuration.compiler, "-xc++", "-stdlib=libc++", "-o", f.name, "-"]
+        with OnDiskTempFile() as f:
+            cmd = [configuration.compiler, "-xc++", "-stdlib=libc++", "-o", f.path, "-"]
             p = subprocess.Popen(
                 cmd,
                 stdin=subprocess.PIPE,
@@ -840,8 +855,8 @@ def canRunMsvcStlTests():
     if platform != "windows":
         return False, f"Don't know how to build with MSVC's STL on {platform}"
 
-    with tempfile.NamedTemporaryFile() as f:
-        cmd = [configuration.compiler, "-xc++", "-o", f.name, "-E", "-"]
+    with OnDiskTempFile() as f:
+        cmd = [configuration.compiler, "-xc++", "-o", f.path, "-E", "-"]
         p = subprocess.Popen(
             cmd,
             stdin=subprocess.PIPE,

SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

Prepares language bindings for LLDB build process. Run with --help
to see a description of the supported command line arguments.
Copy link
Member

Choose a reason for hiding this comment

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

Description of this file is inaccurate

You cannot use a NamedTempFile with an external process because it may
not be flushed to disk.  The safest and most portable approach is to
close the file, call the other process and then unlink the file
manually.

Presumably this works fine on Linux, but it fails on Darwin when
targeting remote-linux.

See https://bugs.python.org/issue29573
@JDevlieghere JDevlieghere merged commit bf7afe1 into llvm:main Jul 31, 2025
9 checks passed
@JDevlieghere JDevlieghere deleted the temp-file branch July 31, 2025 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants