[lldb][test] Set target OS for API tests in case of remote testing #96654
Conversation
|
@llvm/pr-subscribers-lldb Author: Vladislav Dzhidzhoev (dzhidzhoev) ChangesMakefile.rules uses HOST_OS and OS variables for determining host and target OSes for API tests compilation. When lldb's target is set to remote-linux, Makefile.rules script should be executed with the target OS variable set to Linux. This is useful for the case of Windows-to-Linux cross-testing. Full diff: https://github.com/llvm/llvm-project/pull/96654.diff 1 Files Affected:
diff --git a/lldb/packages/Python/lldbsuite/test/lldbplatformutil.py b/lldb/packages/Python/lldbsuite/test/lldbplatformutil.py
index 21f2095db90f8..c39d297a78f8f 100644
--- a/lldb/packages/Python/lldbsuite/test/lldbplatformutil.py
+++ b/lldb/packages/Python/lldbsuite/test/lldbplatformutil.py
@@ -56,6 +56,10 @@ def target_is_android():
return configuration.lldb_platform_name == "remote-android"
+def target_is_remote_linux():
+ return configuration.lldb_platform_name == "remote-linux"
+
+
def android_device_api():
if not hasattr(android_device_api, "result"):
assert configuration.lldb_platform_url is not None
@@ -97,6 +101,9 @@ def finalize_build_dictionary(dictionary):
dictionary = {}
dictionary["OS"] = "Android"
dictionary["PIE"] = 1
+ elif target_is_remote_linux():
+ dictionary = dictionary or {}
+ dictionary["OS"] = "Linux"
return dictionary
|
| @@ -97,6 +101,9 @@ def finalize_build_dictionary(dictionary): | |||
| dictionary = {} | |||
| dictionary["OS"] = "Android" | |||
| dictionary["PIE"] = 1 | |||
| elif target_is_remote_linux(): | |||
| dictionary = dictionary or {} | |||
There was a problem hiding this comment.
Suggestion: Pull the dictionary checking-logic out of these blocks and put it at the top of the function:
def finalize_build_dictionary(dictionary):
if dictionary is None:
dictionary = {}
if target_is_android():
# ...
| @@ -97,6 +101,9 @@ def finalize_build_dictionary(dictionary): | |||
| dictionary = {} | |||
| dictionary["OS"] = "Android" | |||
| dictionary["PIE"] = 1 | |||
| elif target_is_remote_linux(): | |||
There was a problem hiding this comment.
For consistency, I think we should do this for all linux systems, not just remote ones. (Ideally, I would do it for all systems overall, but I can understand if you're reluctant to do that.)
You can use getPlatform to get the platform/os name for both local and remote runs.
dzhidzhoev
force-pushed
the
lldb/target-os
branch
from
c1cb536 to
c107d5d
Compare
|
✅ With the latest revision this PR passed the Python code formatter. |
| def target_is_remote_linux(): | ||
| return configuration.lldb_platform_name == "remote-linux" | ||
|
|
||
|
|
There was a problem hiding this comment.
I guess this isn't used anymore.
| "windows": "Windows_NT", | ||
| } | ||
| dictionary["OS"] = platform_name_to_uname[getPlatform()] | ||
| dictionary["HOST_OS"] = platform_name_to_uname[getHostPlatform()] |
There was a problem hiding this comment.
Why is this under the else: branch? I would expect we need to set HOST_OS even if we're targetting android or darwin...
dzhidzhoev
force-pushed
the
lldb/target-os
branch
3 times, most recently
from
d47f730 to
b6e1aa2
Compare
|
Thank you! Updated it: improved code for Darwin host detection, and removed overriding of HOST_OS in Makefile. |
| # Triple is not available if we're not connected yet | ||
| if p.GetName() == "remote-linux": | ||
| return "linux" | ||
|
|
There was a problem hiding this comment.
Can you explain why was this necessary? I get the connected part, but who was asking for the platform before connecting?
There was a problem hiding this comment.
This may be relevant if _get_platform_os is called before the connection is established, or, for example, if it has failed to be established. (I think I've caught an error a couple of times when the target went down during testing without that)
But yeah, currently I'm not aware of such use cases. Should it be removed?
There was a problem hiding this comment.
Yes, I would very much prefer that, as the code you added only handles linux. To support other oses, we would need to create a whole new mapping from lldb platform names to os strings. That list could then get out of sync, etc., so it would be best to just avoid doing that.
I'm not worried about the debug target going down during testing, as that means the test results will be meaningless anyway. If we find out we regularly need the os name before connecting, then I think we ought to revisit the mechanism for determining the OS.
…esting Makefile.rules uses HOST_OS and OS variables for determining host and target OSes for API tests compilation. This commit starts moving the platform detection logic from Makefile to Python lldb test suite. When lldb's target is set to remote-linux, Makefile.rules script should be executed with the target OS variable set to Linux. This is useful for the case of Windows-to-Linux cross-testing.
dzhidzhoev
force-pushed
the
lldb/target-os
branch
from
b6e1aa2 to
016ed9e
Compare
Makefile.rules uses HOST_OS and OS variables for determining host and target OSes for API tests compilation.
When lldb's target is set to remote-linux, Makefile.rules script should be executed with the target OS variable set to Linux.
This is useful for the case of Windows-to-Linux cross-testing.