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

Fix Windows home directory error #2509

Merged
merged 1 commit into from
Oct 31, 2023

Conversation

danepowell
Copy link
Contributor

Based on downstream user reports (acquia/cli#1611), there's a bug in home directory detection on Windows.

It seems like Terminus doesn't officially support native Windows terminals (CMD, PowerShell, etc), in which case maybe this is dead code that should be removed entirely.

@danepowell danepowell requested a review from a team as a code owner October 23, 2023 20:30
@namespacebrian
Copy link
Contributor

Correct, running terminus on Windows is only officially supported via Windows Subsystem for Linux. Trying to provide support for Windows environments (documenting setup, troubleshooting users' problems, etc) is too much for us to do reliably.

I don't think we want to actively prevent people from maybe using it on Windows, we just can't afford to track down every problem, or gate fixes / new features based on Windows compatibility.. and since we're not doing that, it's probably safe to assume that if it works on (non-WSL) Windows at all right now, we can't trust that it will continue to work in the future.

This change seems small enough that maybe we could merge it. If I'm reading the acquia/cli issue correctly, getenv is returning a boolean. The existing terminus code effectively assumes it's going to return null or a string. Switching to truthy/falsey seems reasonable to me. Or maybe doing one better and checking if it's a string of non-zero length.. since the code seems to assume it's going to be a string...

@namespacebrian namespacebrian merged commit 79e40cb into pantheon-systems:3.x Oct 31, 2023
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.

2 participants