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

Tweak how the terminal is launched on Windows #814

Merged
merged 9 commits into from
Jan 29, 2025

Conversation

fredrikekelund
Copy link
Contributor

@fredrikekelund fredrikekelund commented Jan 17, 2025

Related issues

Proposed Changes

Window distinguishes between user-level and system-level environment variables. Studio launches with the system-level PATH environment variable, which is then inherited by the command prompt when users click the Open in… Terminal button in Studio (this is just how Node's child_process module works).

This can be problematic for users because launching a new command prompt from the Start menu behaves differently. In that case, the user-level PATH environment variable is used. Starting a terminal with an unexpected PATH can obviously be troublesome since you may be missing the utilities you're used to.

This PR changes the logic for how the terminal is launched on Windows to fix this issue. The new logic depends on PowerShell. AFAIU, PowerShell comes bundled with all currently supported Windows versions, so this PR should hopefully work for essentially all of our users.

After more testing, we concluded that we can't reliably reproduce this issue (see #814 (comment)). I repurposed this PR to focus on more cosmetic issues with how the terminal is launched on Windows

Testing Instructions

  1. Follow the instructions outlined in WP CLI: environment variables not set #359 (comment) to install PHP and WP-CLI on Windows
  2. Ensure that the user-level PATH envvar contains the WP-CLI path, but the system-level PATH does not (see clarification)
  3. Start Studio and have a Studio site
  4. Click the Open in… Terminal button
  5. Run wp to ensure that the default, user-level PATH envvar is loaded

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?

@fredrikekelund fredrikekelund requested review from wojtekn and a team January 17, 2025 10:40
@fredrikekelund fredrikekelund self-assigned this Jan 17, 2025
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I recommend viewing this diff with the Hide whitespace option enabled on GitHub, since a large part of my changes here is just re-indentation.

@wojtekn
Copy link
Contributor

wojtekn commented Jan 24, 2025

I tested it on my Windows machine and confirmed that with the build from this PR, wp works in the Terminal session opened from Studio, the same as in the Terminal session opened outside Studio. After removing the wp-cli path from my user environment variable, both stopped working.

However, everything still worked fine when I tested the same with 1.3.1-beta1. 🤔

@fredrikekelund
Copy link
Contributor Author

I can confirm that in 1.3.2, Studio picks up the user-level PATH environment variable, and it's inherited correctly by the terminal. I'm researching what the behavior was like in v1.0.5 (since that's when the issue was reported).

@fredrikekelund
Copy link
Contributor Author

I can't reproduce it in v1.0.5, either… I'm beginning to wonder if this was only reproducible while running the development version of Studio to begin with

@wojtekn
Copy link
Contributor

wojtekn commented Jan 28, 2025

@fredrikekelund do you mean development build? Or the app executed via npm start? If the latter, I can confirm I've never started Studio on Windows for development purposes.

@fredrikekelund
Copy link
Contributor Author

I meant when the app is started with npm start.

If the latter, I can confirm I've never started Studio on Windows for development purposes.

Interesting. Thanks for sharing that, @wojtekn.

@fredrikekelund
Copy link
Contributor Author

Here's a quick status update. The tl;dr is that I've changed the scope of this PR to fix more cosmetic issues with how the terminal is launched on Windows.

  • I can't reliably reproduce the issue that prompted this PR. AFAICT, it doesn't seem to affect built versions of Studio consistently. It's possible that the original problem was that we were modifying environment variables after Studio had launched, in which case they wouldn't get inherited by spawned shells.
  • I chose to take a look at how GitHub desktop handles this problem (see this directory) and ended up copying their solution for Windows.
    • Our original Windows solution is fundamentally very similar to theirs, but the benefit of theirs is that the title of the opened window is now Command Prompt instead of C:\Windows\system32\cmd.exe (which is the same as when you open the terminal from the Start menu).
    • I also modified the Linux solution to be more compact.

@fredrikekelund fredrikekelund changed the title Load the default user-level environment variables in Windows terminal Tweak how the terminal is launched on Windows Jan 28, 2025
@wojtekn
Copy link
Contributor

wojtekn commented Jan 29, 2025

I tested the new build:

  1. Ensured that wp-cli is in my user env variables
  2. restarted system
  3. started Studio and confirmed wp works in Terminal opened from Studio

@fredrikekelund fredrikekelund merged commit 743f7a8 into trunk Jan 29, 2025
6 checks passed
@fredrikekelund fredrikekelund deleted the f26d/load-default-terminal-envvars-on-windows branch January 29, 2025 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WP CLI: environment variables not set
2 participants