zed icon indicating copy to clipboard operation
zed copied to clipboard

Consider removing wezterm fork from dependencies

Open SomeoneToIgnore opened this issue 1 year ago • 1 comments

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 procinfo directly 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?

SomeoneToIgnore avatar Feb 29 '24 12:02 SomeoneToIgnore

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.

mrnugget avatar Feb 29 '24 13:02 mrnugget

@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:

  1. At which moment is it possible to get the 'exit code' and how would that be used ?
  2. Should I also include the MIT license to this crate ? And where would be best to describe where it was taken from ?
  3. Also moved code related to linux process description to keep things the way it's right now. What you think ?
  4. The exe_and_args_for_pid_sysctl from wezterm moves first argv out and sets as executable, but linux version doesn't do that. It also seems that it expects that the first argument is the executable as in here. I added a commit that keeps argv as it is on macos. Does it make sense ?

dalton-oliveira avatar Mar 07 '24 10:03 dalton-oliveira

🚀 🚀 🚀 Spectacular, thank you for picking this up!

  1. 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.

  2. I do not feel excited to carry another, fourth, license into the mix. We have a very permissive Apache-2.0 already 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?

  3. Totally, whatever keeps Zed functional without wezterm depepdency.

  4. 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 🙂

SomeoneToIgnore avatar Mar 07 '24 13:03 SomeoneToIgnore

Dealt with in https://github.com/zed-industries/zed/pull/8998

SomeoneToIgnore avatar Mar 12 '24 19:03 SomeoneToIgnore