Consider removing wezterm fork from dependencies
Zed's terminal crate depends on a single struct of procinfo crate, which we retrieve by cloning the entire wezterm repo from a fork.
Such set-up makes dependency fetching slower than it could be, and seems excessive overall.
Dependency declaration: https://github.com/zed-industries/zed/blob/47bcb305af2e0a533f20b61cba98dfa5b1beb997/crates/terminal/Cargo.toml#L21 The only import in the crate: https://github.com/zed-industries/zed/blob/47bcb305af2e0a533f20b61cba98dfa5b1beb997/crates/terminal/src/terminal.rs#L37
That LocalProcessInfo type is used in multiple places, but all of them seem to use max 2 properties out of it:
https://github.com/zed-industries/zed/blob/47bcb305af2e0a533f20b61cba98dfa5b1beb997/crates/terminal/src/terminal.rs#L682
which gets (re)fetched updated when Alacritty reports back that new terminal contents is available: https://github.com/zed-industries/zed/blob/47bcb305af2e0a533f20b61cba98dfa5b1beb997/crates/terminal/src/terminal.rs#L651
Getting a name and a path of a running process by a given PID should not need a separate fork in dependencies. There seem to be a few custom commits on top of the original repo: https://github.com/wez/wezterm/compare/main...zed-industries:wezterm:main
This task requires to understand, where to move away from the current state:
-
upstream the changes (or describe why those are not needed) into original wezterm That will allow to use
procinfodirectly without cloning some git repository for building Zed, making the fork obsolete. -
add more functionality to Alacritty Zed's terminal wraps around Alacritty "backend" for all terminal-related functionality, no other alternatives exist currently. Alacritty already exposes some process bits via its API: https://github.com/alacritty/alacritty/blob/93e3de7c31ba48945d0bafca6cb6523b32abe0bc/alacritty_terminal/src/tty/unix.rs#L119
It could get a new method to return child process info (including the exit code information), then the entire procinfo dependency can be removed, making the fork obsolete and speeding up Zed's compile times.
- there could be other libc wrapper crates providing the functionality. It would be very beneficial to get the exit code among the two other properties.
Compilation footprint is of a concern here: procinfo is not a large library already https://crates.io/crates/procinfo/0.4.2/dependencies and most of the related slow things come out of cloning the git repo.
Maybe, instead we should create an internal Zed module with the related code bits implemented?
- some other way to remove the git clone of a repo at least?
Maybe, instead we should create an internal Zed module with the related code bits implemented?
I think that sounds reasonable, since all we want is cwd and name.
@SomeoneToIgnore Thanks for detailed description. I have created a PR #8998 that moved required code from wizterm to a separate crate. Have a few questions:
- At which moment is it possible to get the 'exit code' and how would that be used ?
- Should I also include the MIT license to this crate ? And where would be best to describe where it was taken from ?
- Also moved code related to linux process description to keep things the way it's right now. What you think ?
- The
exe_and_args_for_pid_sysctlfrom wezterm moves firstargvout and sets asexecutable, but linux version doesn't do that. It also seems that it expects that the first argument is theexecutableas in here. I added a commit that keepsargvas it is on macos. Does it make sense ?
🚀 🚀 🚀 Spectacular, thank you for picking this up!
-
The 'exit code' is merely a wish of mine to catch the terminal child process' state after Zed's task is run: it now spawns user commands in the terminal and Alacritty has 0 ways to know how exactly did that process finish. I've came up with https://github.com/alacritty/alacritty/pull/7794 meanwhile, so maybe it gets merged and I'll get everything what I need from Alacritty. So, maybe let's not do anything more on that front in the new crate.
-
I do not feel excited to carry another, fourth, license into the mix. We have a very permissive
Apache-2.0already in the use: https://github.com/zed-industries/zed/blob/d450fde1edac685f7a9e5ce1046d6551a0983169/crates/gpui/Cargo.toml#L8 so let's use that for the new crate either? -
Totally, whatever keeps Zed functional without wezterm depepdency.
-
Not sure, but I guess at this point, it's not the main goal and whatever happens on other platforms, can adjust the code accordingly later.
I will have a look at the PR shortly, hopefully those responses helped you 🙂
Dealt with in https://github.com/zed-industries/zed/pull/8998