Skip to content
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.

Sign up for GitHub

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

fix empty command behavior on windows #2378

Merged
merged 4 commits into from
Jan 2, 2024
Merged

fix empty command behavior on windows #2378

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f3146a7
fix empty command behavior on windows
AlexeyBond Dec 18, 2023
9c3fb1e
do not try to start client process when command is empty
AlexeyBond Dec 19, 2023
c8bd4c7
style
rchl Jan 2, 2024
d3d3dc7
Merge branch 'main' into AlexeyBond-fix-windows-empty-command
rchl Jan 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion plugin/core/transports.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,11 +245,11 @@ def create_transport(config: TransportConfig, cwd: Optional[str],
else:
stdout = subprocess.PIPE
stdin = subprocess.PIPE
startupinfo = _fixup_startup_args(config.command)
sock = None # type: Optional[socket.socket]
process = None # type: Optional[subprocess.Popen]

def start_subprocess() -> subprocess.Popen:
startupinfo = _fixup_startup_args(config.command)
return _start_subprocess(config.command, stdin, stdout, subprocess.PIPE, startupinfo, config.env, cwd)
Copy link
Member

@predragnikolic predragnikolic Dec 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

the bug is caused by the _fixup_startup_args function

def _fixup_startup_args(args: List[str]) -> Any:
    startupinfo = None
    if sublime.platform() == "windows":
        # ... this line bellow causes the Error because, args is `[]`
        executable_arg = args[0]

start_subprocess will be called in 2 cases:

-   startupinfo = _fixup_startup_args(config.command)


    def start_subprocess() -> subprocess.Popen:
+       startupinfo = _fixup_startup_args(config.command)
        return _start_subprocess(config.command, stdin, stdout, subprocess.PIPE, startupinfo, config.env, cwd)

    if config.listener_socket:
        assert isinstance(config.tcp_port, int) and config.tcp_port > 0
        process, sock, reader, writer = _await_tcp_connection(
            config.name, config.tcp_port, config.listener_socket, start_subprocess) # <- Case 1
    else:
        if config.command:
            process = start_subprocess() # <- Case 2

In this PR the _fixup_startup_args will not be called in Case 2 anymore,
so the bug in that case will be fixed.

But in Case 1, when start_subprocess is called, the _fixup_startup_args will also be called, and the command will still be None. So we still have a bug there.

IMO, a better option would be to fix the _fixup_startup_args function:

def _fixup_startup_args(args: List[str]) -> Any:
+     if not args:
+         return None
    startupinfo = None
    if sublime.platform() == "windows":
        startupinfo = subprocess.STARTUPINFO()  # type: ignore
        startupinfo.dwFlags |= subprocess.SW_HIDE | subprocess.STARTF_USESHOWWINDOW  # type: ignore
        executable_arg = args[0]
        _, ext = os.path.splitext(executable_arg)
        if len(ext) < 1:
            path_to_executable = shutil.which(executable_arg)
            # what extensions should we append so CreateProcess can find it?
            # node has .cmd
            # dart has .bat
            # python has .exe wrappers - not needed
            for extension in ['.cmd', '.bat']:
                if path_to_executable and path_to_executable.lower().endswith(extension):
                    args[0] = executable_arg + extension
                    break
    return startupinfo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like something else is wrong with case 1: it doesn't make sense to start a subprocess with empty command. And _start_subprocess will fail when first argument is empty:

def _start_subprocess(
    args: List[str],
# ...
) -> subprocess.Popen:
    debug("starting {} in {}".format(args, cwd if cwd else os.getcwd()))
    process = subprocess.Popen( # <- Will throw if args is empty
        args=args,
        # ...
    )
    _subprocesses.add(process)
    return process

So, even with check added in _fixup_startup_args, it will still fail, but in _start_subprocess.


if config.listener_socket:
Expand Down