Skip to content

Conversation

@kusmour
Copy link
Contributor

@kusmour kusmour commented Oct 29, 2024

API test failed for remote platform in #112657
See test detail: https://lab.llvm.org/buildbot/#/builders/195/builds/321

Previously when putting files onto remote platform, I used platform file write -d <data> which actually required a platform file open <path> first in order to obtain a file descriptor.
eg. in file TestGDBRemotePlatformFile.py
To fix this, use the platform put-file method, which is used in the redirect_stdin from this test already.

@kusmour kusmour requested a review from JDevlieghere as a code owner October 29, 2024 19:54
@llvmbot llvmbot added the lldb label Oct 29, 2024
@kusmour kusmour requested a review from jeffhammond October 29, 2024 19:55
@llvmbot
Copy link
Member

llvmbot commented Oct 29, 2024

@llvm/pr-subscribers-lldb

Author: Wanyi (kusmour)

Changes

API test failed for remote debugging in #112657

Fixing it properly


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

1 Files Affected:

  • (modified) lldb/test/API/python_api/process/io/TestProcessIO.py (+27-13)
diff --git a/lldb/test/API/python_api/process/io/TestProcessIO.py b/lldb/test/API/python_api/process/io/TestProcessIO.py
index 3b5c7c48c51f4d..24cd845904cc2c 100644
--- a/lldb/test/API/python_api/process/io/TestProcessIO.py
+++ b/lldb/test/API/python_api/process/io/TestProcessIO.py
@@ -99,31 +99,45 @@ def test_stdout_stderr_redirection(self):
     @expectedFlakeyLinux(bugnumber="llvm.org/pr26437")
     @skipIfDarwinEmbedded  # debugserver can't create/write files on the device
     def test_stdout_stderr_redirection_to_existing_files(self):
-        """Exercise SBLaunchInfo::AddOpenFileAction() for STDOUT and STDERR without redirecting STDIN to output files already exist."""
+        """Exercise SBLaunchInfo::AddOpenFileAction() for STDOUT and STDERR redirect to output files already exist."""
         self.setup_test()
         self.build()
         self.create_target()
-        self.write_file_with_placeholder(self.output_file)
-        self.write_file_with_placeholder(self.error_file)
-        self.redirect_stdout()
-        self.redirect_stderr()
-        self.run_process(True)
-        output = self.read_output_file_and_delete()
-        error = self.read_error_file_and_delete()
-        self.check_process_output(output, error)
 
-    def write_file_with_placeholder(self, target_file):
+        # Create the output and error files with placeholder
         placeholder = "This content should be overwritten."
         if lldb.remote_platform:
+            f = open(self.local_output_file, "w")
+            f.write(placeholder)
+            f.close()
+
+            f = open(self.local_error_file, "w")
+            f.write(placeholder)
+            f.close()
+            self.runCmd(
+                'platform put-file "{local}" "{remote}"'.format(
+                    local=self.local_output_file, remote=self.output_file
+                )
+            )
             self.runCmd(
-                'platform file write "{target}" -d "{data}"'.format(
-                    target=target_file, data=placeholder
+                'platform put-file "{local}" "{remote}"'.format(
+                    local=self.local_error_file, remote=self.error_file
                 )
             )
         else:
-            f = open(target_file, "w")
+            f = open(self.output_file, "w")
+            f.write(placeholder)
+            f.close()
+            f = open(self.error_file, "w")
             f.write(placeholder)
             f.close()
+            
+        self.redirect_stdout()
+        self.redirect_stderr()
+        self.run_process(True)
+        output = self.read_output_file_and_delete()
+        error = self.read_error_file_and_delete()
+        self.check_process_output(output, error)
 
     # target_file - path on local file system or remote file system if running remote
     # local_file - path on local system

@kusmour kusmour requested review from Jlalond and bulbazord and removed request for jeffhammond October 29, 2024 19:55
@github-actions
Copy link

github-actions bot commented Oct 29, 2024

✅ With the latest revision this PR passed the Python code formatter.

# Create the output and error files with placeholder
placeholder = "This content should be overwritten."
if lldb.remote_platform:
f = open(self.local_output_file, "w")
Copy link
Member

Choose a reason for hiding this comment

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

Why does this create files on your local filesystem now instead of doing the previous platform file write commands? Was there something wrong with the previous approach?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we are going to make local files the Save core tests put them in the build directory as an example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I double checked the platform file write it will require platform file open first and obtain the file descriptor so that we can write.

The put method however won't require extra steps and is used in the same test already.

# Create the output and error files with placeholder
placeholder = "This content should be overwritten."
if lldb.remote_platform:
f = open(self.local_output_file, "w")
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are going to make local files the Save core tests put them in the build directory as an example.

@kusmour kusmour force-pushed the lldb-file-action-open-fix branch from dfa3252 to a1b19df Compare October 29, 2024 23:19
@kusmour kusmour merged commit f7c36d2 into llvm:main Oct 30, 2024
7 checks passed
@kusmour kusmour deleted the lldb-file-action-open-fix branch October 31, 2024 18:20
smallp-o-p pushed a commit to smallp-o-p/llvm-project that referenced this pull request Nov 3, 2024
API test failed for remote platform in
[llvm#112657](llvm#112657)

Previously when putting files onto remote platform, I used `platform
file write -d <data>` which actually required a `platform file open
<path>` first in order to obtain a file descriptor.
eg. in file
[TestGDBRemotePlatformFile.py](https://github.com/llvm/llvm-project/blob/94e7d9c0bfe517507ea08b00fb00c32fb2837a82/lldb/test/API/functionalities/gdb_remote_client/TestGDBRemotePlatformFile.py#L24-L32)
To fix this, use the `platform put-file` method, which is used in the
`redirect_stdin` from this test already.
NoumanAmir657 pushed a commit to NoumanAmir657/llvm-project that referenced this pull request Nov 4, 2024
API test failed for remote platform in
[llvm#112657](llvm#112657)

Previously when putting files onto remote platform, I used `platform
file write -d <data>` which actually required a `platform file open
<path>` first in order to obtain a file descriptor.
eg. in file
[TestGDBRemotePlatformFile.py](https://github.com/llvm/llvm-project/blob/94e7d9c0bfe517507ea08b00fb00c32fb2837a82/lldb/test/API/functionalities/gdb_remote_client/TestGDBRemotePlatformFile.py#L24-L32)
To fix this, use the `platform put-file` method, which is used in the
`redirect_stdin` from this test already.
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.

4 participants