-
Notifications
You must be signed in to change notification settings - Fork 14.5k
[lldb][mcp] Fix unix domain socket protocol server addresses #146603
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
Conversation
@llvm/pr-subscribers-lldb Author: Alexandre Perez (aperez) ChangesWhen starting an MCP protocol server that uses unix sockets as the transport, a local
This change makes it so that the URI path is used if the socket protocol is
Full diff: https://github.com/llvm/llvm-project/pull/146603.diff 1 Files Affected:
diff --git a/lldb/source/Commands/CommandObjectProtocolServer.cpp b/lldb/source/Commands/CommandObjectProtocolServer.cpp
index 55bd42ed1a533..4cafd2266e235 100644
--- a/lldb/source/Commands/CommandObjectProtocolServer.cpp
+++ b/lldb/source/Commands/CommandObjectProtocolServer.cpp
@@ -75,9 +75,12 @@ class CommandObjectProtocolServerStart : public CommandObjectParsed {
ProtocolServer::Connection connection;
connection.protocol = protocol_and_mode->first;
- connection.name =
- formatv("[{0}]:{1}", uri->hostname.empty() ? "0.0.0.0" : uri->hostname,
- uri->port.value_or(0));
+ if (connection.protocol == Socket::SocketProtocol::ProtocolUnixDomain)
+ connection.name = uri->path;
+ else
+ connection.name = formatv(
+ "[{0}]:{1}", uri->hostname.empty() ? "0.0.0.0" : uri->hostname,
+ uri->port.value_or(0));
if (llvm::Error error = server->Start(connection)) {
result.AppendErrorWithFormatv("{0}", llvm::fmt_consume(std::move(error)));
|
Shall we add a test for this? |
@kusmour @JDevlieghere |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment, otherwise LGTM
Test if we can start an MCP protocol-server accepting unix sockets | ||
""" | ||
|
||
temp_directory = tempfile.TemporaryDirectory() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One note, for unix sockets there is often a character limit of around 104~108 chars depending on the platform.
This will often fetch a directory using TEMP_DIR, which may be overridden by say a CI system to something pretty long.
We may either want to skip the test if the tempdir is to long or try to find a shorter directory like "/tmp", if it exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I've pushed a change to make sure we skip if the socket path is over that limit
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/197/builds/6596 Here is the relevant piece of the build log for the reference
|
It looks like #146603 broke the [lldb-remote-linux-win](https://lab.llvm.org/buildbot/#/builders/197) build bot because `MCPUnixSocketCommandTestCase` is trying to start a protocol-server via unix domain sockets on windows. This change makes it so the test is skipped if it is remote.
When starting an MCP protocol server that uses unix sockets as the transport, a local
'[0.0.0.0]:0'
file is used instead of the supplied socket path, e.g:This change makes it so that the URI path is used if the socket protocol is
ProtocolUnixDomain
: