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.

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

windows: Set CREATE_NO_WINDOW for commands #18447

Merged
merged 14 commits into from
Nov 21, 2024

Conversation

JunkuiZhang
Copy link
Contributor

@JunkuiZhang JunkuiZhang commented Sep 27, 2024

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Sep 27, 2024
@CharlesChen0823
Copy link
Contributor

CharlesChen0823 commented Sep 28, 2024

I think the best method should be introduce an file name command in util crate, in here, create new_std_command and new_smol_command with set CREATE_NO_WINDOW. we can use the two function everywhere, don't need hacky everywhere.

@JunkuiZhang
Copy link
Contributor Author

I think the best method should be introduce an file name command in util crate, in here, create new_std_command and new_smol_command with set CREATE_NO_WINDOW. we can use the two function everywhere, don't need hacky everywhere.

That's my next move. The idea of this draft PR is that making a quick demo to see if it could fix the issue. If so, then I'm gonna refactor the codes.

@SomeoneToIgnore
Copy link
Contributor

SomeoneToIgnore commented Oct 13, 2024

Is it really the only way we can enable window-less command invocations in Windows?
Feels very fragile, as every other plugin that forgets about Windows and this behavior, will bring the pop-ups back.

@CharlesChen0823
Copy link
Contributor

Is it really the only way we can enable window-less command invocations in Windows? Feels very fragile, as every other plugin that forgets about Windows and this behavior, will bring the pop-ups back.

I think this is only way

@SomeoneToIgnore
Copy link
Contributor

This is very very odd, but then I think #19018 is not that bad if it's a documented single entry for all such plugin invocations.

I'll leave the review (and the choice) to the person who knows things better than me though.

@JunkuiZhang
Copy link
Contributor Author

JunkuiZhang commented Oct 13, 2024

Is it really the only way we can enable window-less command invocations in Windows? Feels very fragile, as every other plugin that forgets about Windows and this behavior, will bring the pop-ups back.

I'm afraid this is indeed the case. std::process::Command::new(...).spawn() internally calls CreateProcess, as seen here:

cvt(c::CreateProcessW(
    program.as_ptr(),
    cmd_str.as_mut_ptr(),
    ptr::null_mut(),
    ptr::null_mut(),
    c::TRUE,
    flags,
    envp,
    dirp,
    si_ptr,
    &mut pi,
))

According to Microsoft's documentation, if you don't want the window to appear, you can set dwCreationFlags to include CREATE_NO_WINDOW, or configure other settings. But in any case, we always need to set some variables.

Moreover, in Windows programming, whether a window is displayed by default when calling CreateProcess depends on the parent process. If the parent process already has a console window, then the call to CreateProcess will not create a new window but will reuse the existing one. This explains the behavior when compiling Zed in debug mode—Zed is treated as a console app and thus has a console window. As a result, std::process::Command does not create a new window. In release mode, however, Zed doesn't have a console window, so std::process::Command always creates a new one.

@JunkuiZhang JunkuiZhang force-pushed the windows/no-cmd-window branch from ca2910a to c44a5d8 Compare October 13, 2024 13:48
@JunkuiZhang
Copy link
Contributor Author

I referenced VSCode's code here to introduce util::command::new_std_command and util::command::new_smol_command. If we decide not to proceed with this approach, we can easily revert to the previous commit.

Additionally, since VSCode's code is under the MIT license and the crate util uses the APACHE license, would there be any issue?

use std::ffi::OsStr;

#[cfg(target_os = "windows")]
const CREATE_NO_WINDOW: u32 = 134217728u32;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const CREATE_NO_WINDOW: u32 = 134217728u32;
const CREATE_NO_WINDOW: u32 = 0x08000000;

Small nitpick, but I believe this to be the more readable and understandable representation for the flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thx!

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries, thank you for all your awesome contributions for the community!

@teknalb

This comment was marked as off-topic.

@ddanielsantos

This comment was marked as off-topic.

@teknalb

This comment was marked as off-topic.

@kelvinchin12070811
Copy link

Just let them do their work, seems like they are too busy at ssh remote dev and no time for other issues. JunkuiZhang seems to be the only Windows dev that they have which is currently active and he's human too, and there's still a lot of issues for him to fix.

@eternalphane
Copy link

Can we have this merged please, so annoying to have those terminal popups exploding like fireworks on my face every time I use Zed

@teknalb You can cherry-pick this PR and build it by yourself if you're really concerned

@teknalb
Copy link

teknalb commented Nov 2, 2024

You can cherry-pick this PR and build it by yourself

@eternalphane Yep, that's exactly what I ended up doing.

@shenjackyuanjie
Copy link
Contributor

It looks like we need tokio::process::Command wrapper @JunkuiZhang

@JunkuiZhang
Copy link
Contributor Author

It looks like we need tokio::process::Command wrapper @JunkuiZhang

I'll to continue with this PR once the Zed team has decided on which approach to take.

@shenjackyuanjie
Copy link
Contributor

for anyone who want to use this branch's work, check out https://github.com/shenjackyuanjie/zed-win-build/releases
and I am maintaining a temporary branch https://github.com/shenjackyuanjie/zed/tree/temp/cmdfix

@xgfone

This comment has been minimized.

@JunkuiZhang JunkuiZhang force-pushed the windows/no-cmd-window branch from 3edff29 to a10373d Compare November 19, 2024 13:47
Copy link

@codecooker1 codecooker1 left a comment

Choose a reason for hiding this comment

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

Seems cool. There doesn't seems to be any malicious code hidden (I think) :P

@mikayla-maki
Copy link
Contributor

Thank you!

@mikayla-maki mikayla-maki merged commit 95ace03 into zed-industries:main Nov 21, 2024
11 checks passed
@JunkuiZhang JunkuiZhang deleted the windows/no-cmd-window branch November 21, 2024 15:13
mikayla-maki pushed a commit that referenced this pull request Nov 21, 2024
…solution (#20991)

After #18447 was merged, I reviewed the PR code as usual. During this
review, I realized that some code was unintentionally removed when I was
resolving merge conflicts in #18447.

Sorry!

Release Notes:

- N/A
Anthony-Eid pushed a commit to Anthony-Eid/zed that referenced this pull request Nov 22, 2024
Anthony-Eid pushed a commit to Anthony-Eid/zed that referenced this pull request Nov 22, 2024
…e-conflicts resolution (zed-industries#20991)

After zed-industries#18447 was merged, I reviewed the PR code as usual. During this
review, I realized that some code was unintentionally removed when I was
resolving merge conflicts in zed-industries#18447.

Sorry!

Release Notes:

- N/A
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Windows] Multiple open and closed terminals at startup