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
Add zip
extract support for Windows
#11156
Add zip
extract support for Windows
#11156
Conversation
01db39c
to
5c526a5
Compare
110db34
to
67868eb
Compare
5471989
to
1d5b131
Compare
Now, Node.js runtime has fixed and Copilot works. 2024-04-30.223822.mp4 |
10957ab
to
54af09b
Compare
Node runtime have been passed test. 2024-04-30.232507.mp4 |
I think I have done of all about Windows install zip and Node.js runtime install, we can review and testing now. I have to do a lot of file change, because there are too many issues on Windows. They are mostly issues that have different path on Windows. |
zip
extract supports for Windowszip
extract support for Windows
Release Notes: - N/A --- Follow #11156, to make sure extensions install on window. https://github.com/tamasfe/taplo/releases/tag/0.8.1 The Taplo have `gz` for windows, so we can just use `gz`. --------- Co-authored-by: Marshall Bowers <elliott.codes@gmail.com>
Anything I can do to help with this? Was trying to solve the same issue and found this PR 🙂 Those force-pushes are killing me trying to work out what's going on with the current state of the PR so thought it better I just ask 😅 |
} | ||
|
||
#[cfg(windows)] | ||
command.creation_flags(windows::Win32::System::Threading::CREATE_NO_WINDOW.0); |
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.
@maxdeviant I'd like to improve the Command::new
in entire of the Zed, there have a lot of place need to do this.
For example:
crates\languages\src\go.rs
So, can I add a helper in util::process:command
in util
crate to create smol::process:Command
in shortly with Windows special creation_flags
? Then other place can just use this method to create command.
This is not related to this PR, I think we can split a new PR to do this.
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.
After multiple attempts to review this PR, I think it's touching a few too many parts.
I think we should refocus on just fixing Node runtime support on Windows and leave the other bits untouched.
We can inline the zip extraction into the node_runtime
crate. We don't need to add a new archive
crate or touch any of the tar
or tar.gz
-related code.
futures::pin_mut!(body); | ||
self.host.fs.create_file_with(&zip_path, body).await?; | ||
|
||
let unzip_status = std::process::Command::new("unzip") |
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.
How about the part?
I make extract_zip
method before , because here it requires the new zip extraction method, the unzip
command is not working on Windows. And node_runtime and more places also need to.
ecc3ba4
to
91e00bd
Compare
I have updated:
|
91e00bd
to
5131e1a
Compare
Reverted this copilot change. The https://github.com/zed-industries/zed/pull/11591/files#diff-9aed0f58926aee71638164c95da3800a90460d525d736a354c71490e333c8d88R581 is a better way. |
5131e1a
to
6a6e6f3
Compare
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, much better!
@maxdeviant For other parts of the zip decompression, you can follow up to see how to deal with it. Currently, there are still problems using the However, the parts that have been merged so far and the panic problem caused by Copilot's root_path unwrap will be fixed. Then the most Node.js related ones can already be run. The HTTP Proxy issue has not been resolved yet. I don’t know if it is a problem with my proxy software. I have tested it before, but it was strange today, so I canceled it first. I'll submit another one after it's resolved. |
this should fixed, currently open immediaterly crashed. |
This reverts commit 5e06ce4.
This reverts commit 5e06ce4.
Release Notes: - [x] Fixed install Node.js runtime and NPM lsp installation on Windows. - [x] Update Node runtime command to execute on Windows with no window popup. Ref zed-industries#9619, zed-industries#9424 --------- Co-authored-by: Marshall Bowers <elliott.codes@gmail.com>
Release Notes:
Ref #9619, #9424